Ronald Cron | 43ffc9d | 2021-12-09 10:09:36 +0100 | [diff] [blame] | 1 | TLS 1.3 support |
| 2 | =============== |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 3 | |
| 4 | Overview |
| 5 | -------- |
| 6 | |
Ronald Cron | 43ffc9d | 2021-12-09 10:09:36 +0100 | [diff] [blame] | 7 | Mbed TLS provides a minimum viable implementation of the TLS 1.3 protocol |
| 8 | defined in the "MVP definition" section below. The TLS 1.3 support enablement |
| 9 | is controlled by the MBEDTLS_SSL_PROTO_TLS1_3 configuration option. |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 10 | |
Ronald Cron | 43ffc9d | 2021-12-09 10:09:36 +0100 | [diff] [blame] | 11 | The development of the TLS 1.3 protocol is based on the TLS 1.3 prototype |
| 12 | located at https://github.com/hannestschofenig/mbedtls. The prototype is |
| 13 | itself based on a version of the development branch that we aim to keep as |
| 14 | recent as possible (ideally the head) by merging regularly commits of the |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 15 | development branch into the prototype. The section "Prototype upstreaming |
| 16 | status" below describes what remains to be upstreamed. |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 17 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 18 | |
| 19 | MVP definition |
| 20 | -------------- |
| 21 | |
Ronald Cron | f164b6a | 2021-09-27 15:36:29 +0200 | [diff] [blame] | 22 | - Overview |
| 23 | |
| 24 | - The TLS 1.3 MVP implements only the client side of the protocol. |
| 25 | |
| 26 | - The TLS 1.3 MVP supports ECDHE key establishment. |
| 27 | |
| 28 | - The TLS 1.3 MVP does not support DHE key establishment. |
| 29 | |
| 30 | - The TLS 1.3 MVP does not support pre-shared keys, including any form of |
| 31 | session resumption. This implies that it does not support sending early |
| 32 | data (0-RTT data). |
| 33 | |
| 34 | - The TLS 1.3 MVP supports the authentication of the server by the client |
| 35 | but does not support authentication of the client by the server. In terms |
| 36 | of TLS 1.3 authentication messages, this means that the TLS 1.3 MVP |
| 37 | supports the processing of the Certificate and CertificateVerify messages |
| 38 | but not of the CertificateRequest message. |
| 39 | |
| 40 | - The TLS 1.3 MVP does not support the handling of server HelloRetryRequest |
| 41 | message. In practice, this means that the handshake will fail if the MVP |
| 42 | does not provide in its ClientHello the shared secret associated to the |
| 43 | group selected by the server for key establishement. For more information, |
| 44 | see the comment associated to the `key_share` extension below. |
| 45 | |
| 46 | - If the TLS 1.3 MVP receives a HelloRetryRequest or a CertificateRequest |
| 47 | message, it aborts the handshake with an handshake_failure closure alert |
| 48 | and the `mbedtls_ssl_handshake()` returns in error with the |
| 49 | `MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE` error code. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 50 | |
| 51 | - Supported cipher suites: depends on the library configuration. Potentially |
| 52 | all of them: |
| 53 | TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, |
| 54 | TLS_AES_128_CCM_SHA256 and TLS_AES_128_CCM_8_SHA256. |
| 55 | |
| 56 | - Supported ClientHello extensions: |
| 57 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 58 | | Extension | MVP | Prototype (1) | |
| 59 | | ---------------------------- | ------- | ------------- | |
Ronald Cron | 85e5108 | 2021-09-27 12:13:16 +0200 | [diff] [blame] | 60 | | server_name | YES | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 61 | | max_fragment_length | no | YES | |
| 62 | | status_request | no | no | |
| 63 | | supported_groups | YES | YES | |
| 64 | | signature_algorithms | YES | YES | |
| 65 | | use_srtp | no | no | |
| 66 | | heartbeat | no | no | |
| 67 | | apln | no | YES | |
| 68 | | signed_certificate_timestamp | no | no | |
| 69 | | client_certificate_type | no | no | |
| 70 | | server_certificate_type | no | no | |
| 71 | | padding | no | no | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 72 | | key_share | YES (2) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 73 | | pre_shared_key | no | YES | |
| 74 | | psk_key_exchange_modes | no | YES | |
| 75 | | early_data | no | YES | |
| 76 | | cookie | no | YES | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 77 | | supported_versions | YES (3) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 78 | | certificate_authorities | no | no | |
| 79 | | post_handshake_auth | no | no | |
| 80 | | signature_algorithms_cert | no | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 81 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 82 | (1) This is just for comparison. |
| 83 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame] | 84 | (2) The MVP sends only one shared secret corresponding to the configured |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 85 | preferred group. This could end up with connection failure if the |
| 86 | server does not support our preferred curve, as the MVP does not implement |
Paul Elliott | 89c8e09 | 2021-11-30 10:54:52 +0000 | [diff] [blame] | 87 | HelloRetryRequest. The preferred group is the group of the first curve in |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 88 | the list of allowed curves as defined by the configuration. The allowed |
| 89 | curves are by default ordered as follows: `x25519`, `secp256r1`, |
| 90 | `secp384r1` and finally `secp521r1`. Note that, in the absence of an |
| 91 | application profile standard specifying otherwise, section 9.1 of the |
| 92 | specification rather promotes curve `secp256r1` to be supported over |
Paul Elliott | cce0f5a | 2021-12-03 16:13:30 +0000 | [diff] [blame] | 93 | curve `x25519`. The MVP would, however, rather keep the preference order |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 94 | currently promoted by Mbed TLS as this applies to TLS 1.2 as well, and |
| 95 | changing the order only for TLS1.3 would be potentially difficult. |
| 96 | In the unlikely event a server does not support curve `x25519` but does |
| 97 | support curve `secp256r1`, curve `secp256r1` can be set as the preferred |
| 98 | curve through the `mbedtls_ssl_conf_curves()` API. |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 99 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame] | 100 | (3) The MVP proposes only TLS 1.3 and does not support version negotiation. |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 101 | Out-of-protocol fallback is supported though if the Mbed TLS library |
| 102 | has been built to support both TLS 1.3 and TLS 1.2: just set the |
| 103 | maximum of the minor version of the SSL configuration to |
| 104 | MBEDTLS_SSL_MINOR_VERSION_3 (`mbedtls_ssl_conf_min_version()` API) and |
| 105 | re-initiate a server handshake. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 106 | |
| 107 | - Supported groups: depends on the library configuration. |
Ronald Cron | 8ee9ed6 | 2021-09-28 14:46:43 +0200 | [diff] [blame] | 108 | Potentially all ECDHE groups but x448: |
| 109 | secp256r1, x25519, secp384r1 and secp521r1. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 110 | |
| 111 | Finite field groups (DHE) are not supported. |
| 112 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 113 | - Supported signature algorithms (both for certificates and CertificateVerify): |
| 114 | depends on the library configuration. |
| 115 | Potentially: |
| 116 | rsa_pkcs1_sha256, rsa_pss_rsae_sha256, ecdsa_secp256r1_sha256, |
| 117 | ecdsa_secp384r1_sha384 and ecdsa_secp521r1_sha512. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 118 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 119 | Note that in absence of an application profile standard specifying otherwise |
| 120 | the three first ones in the list above are mandatory (see section 9.1 of the |
| 121 | specification). |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 122 | |
Jerry Yu | 72a0565 | 2022-01-25 14:36:30 +0800 | [diff] [blame] | 123 | - Supported versions: |
| 124 | |
Jerry Yu | adb1869 | 2022-01-27 12:55:32 +0800 | [diff] [blame] | 125 | - TLS 1.2 and TLS 1.3 but version negotiation is not supported. |
Jerry Yu | 72a0565 | 2022-01-25 14:36:30 +0800 | [diff] [blame] | 126 | |
Jerry Yu | bd19287 | 2022-02-08 10:20:53 +0800 | [diff] [blame] | 127 | - TLS 1.3 cannot be enabled in the build (MBEDTLS_SSL_PROTO_TLS1_3 |
| 128 | configuration option) without TLS 1.2 (MBEDTLS_SSL_PROTO_TLS1_2 configuration |
| 129 | option). |
Jerry Yu | 72a0565 | 2022-01-25 14:36:30 +0800 | [diff] [blame] | 130 | |
Jerry Yu | adb1869 | 2022-01-27 12:55:32 +0800 | [diff] [blame] | 131 | - TLS 1.2 can be enabled in the build independently of TLS 1.3. |
Jerry Yu | 72a0565 | 2022-01-25 14:36:30 +0800 | [diff] [blame] | 132 | |
Jerry Yu | adb1869 | 2022-01-27 12:55:32 +0800 | [diff] [blame] | 133 | - If both TLS 1.3 and TLS 1.2 are enabled at build time, only one of them can |
| 134 | be configured at runtime via `mbedtls_ssl_conf_{min,max}_version`. Otherwise, |
| 135 | `mbedtls_ssl_setup` will raise `MBEDTLS_ERR_SSL_BAD_CONFIG` error. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 136 | |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 137 | - Compatibility with existing SSL/TLS build options: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 138 | |
| 139 | The TLS 1.3 MVP is compatible with all TLS 1.2 configuration options in the |
| 140 | sense that when enabling the TLS 1.3 MVP in the library there is no need to |
Ronald Cron | 4279bac | 2022-02-11 15:08:48 +0100 | [diff] [blame] | 141 | modify the configuration for TLS 1.2. The MBEDTLS_USE_PSA_CRYPTO configuration |
| 142 | option is an exception though, the TLS 1.3 MVP is not compatible with it. |
| 143 | |
| 144 | Mbed TLS SSL/TLS related features are not supported or not applicable to the |
| 145 | TLS 1.3 MVP: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 146 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 147 | | Mbed TLS configuration option | Support | |
| 148 | | ---------------------------------------- | ------- | |
| 149 | | MBEDTLS_SSL_ALL_ALERT_MESSAGES | no | |
| 150 | | MBEDTLS_SSL_ASYNC_PRIVATE | no | |
| 151 | | MBEDTLS_SSL_CONTEXT_SERIALIZATION | no | |
| 152 | | MBEDTLS_SSL_DEBUG_ALL | no | |
| 153 | | MBEDTLS_SSL_ENCRYPT_THEN_MAC | n/a | |
| 154 | | MBEDTLS_SSL_EXTENDED_MASTER_SECRET | n/a | |
| 155 | | MBEDTLS_SSL_KEEP_PEER_CERTIFICATE | no | |
| 156 | | MBEDTLS_SSL_RENEGOTIATION | n/a | |
| 157 | | MBEDTLS_SSL_MAX_FRAGMENT_LENGTH | no | |
| 158 | | | | |
| 159 | | MBEDTLS_SSL_SESSION_TICKETS | no | |
| 160 | | MBEDTLS_SSL_EXPORT_KEYS | no (1) | |
| 161 | | MBEDTLS_SSL_SERVER_NAME_INDICATION | no | |
| 162 | | MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH | no | |
| 163 | | | | |
| 164 | | MBEDTLS_ECP_RESTARTABLE | no | |
| 165 | | MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED | no | |
| 166 | | | | |
| 167 | | MBEDTLS_KEY_EXCHANGE_PSK_ENABLED | n/a (2) | |
| 168 | | MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED | n/a | |
| 169 | | MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED | n/a | |
| 170 | | MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED | n/a | |
| 171 | | MBEDTLS_KEY_EXCHANGE_RSA_ENABLED | n/a | |
| 172 | | MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED | n/a | |
| 173 | | MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED | n/a | |
| 174 | | MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED | n/a | |
| 175 | | MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED | n/a | |
| 176 | | MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED | n/a | |
| 177 | | MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED | n/a | |
| 178 | | | | |
Ronald Cron | 87829e5 | 2022-02-14 16:31:33 +0100 | [diff] [blame] | 179 | | MBEDTLS_USE_PSA_CRYPTO | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 180 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 181 | (1) Some support has already been upstreamed but it is incomplete. |
Ronald Cron | 1fa5088 | 2021-09-27 12:06:52 +0200 | [diff] [blame] | 182 | (2) Key exchange configuration options for TLS 1.3 will likely to be |
| 183 | organized around the notion of key exchange mode along the line |
Xiaofei Bai | 746f948 | 2021-11-12 08:53:56 +0000 | [diff] [blame] | 184 | of the MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_NONE/PSK/PSK_EPHEMERAL/EPHEMERAL |
Ronald Cron | 1fa5088 | 2021-09-27 12:06:52 +0200 | [diff] [blame] | 185 | runtime configuration macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 186 | |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 187 | - Quality considerations |
| 188 | - Standard Mbed TLS review bar |
| 189 | - Interoperability testing with OpenSSL and GnuTLS. Test with all the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 190 | cipher suites and signature algorithms supported by OpenSSL/GnuTLS server. |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 191 | - Negative testing against OpenSSL/GnuTLS servers with which the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 192 | handshake fails due to incompatibility with the capabilities of the |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 193 | MVP: TLS 1.2 or 1.1 server, server sending an HelloRetryRequest message in |
| 194 | response to the MVP ClientHello, server sending a CertificateRequest |
| 195 | message ... |
| 196 | |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 197 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 198 | Prototype upstreaming status |
| 199 | ---------------------------- |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 200 | |
Ronald Cron | b1822ef | 2021-12-10 14:28:13 +0100 | [diff] [blame] | 201 | The following summarizes which parts of the TLS 1.3 prototype remain to be |
| 202 | upstreamed: |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 203 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 204 | - Ephemeral only handshake on client side: client authentication, |
| 205 | HelloRetryRequest support, version negotiation. |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 206 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 207 | - Ephemeral only handshake server side. |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 208 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 209 | - Pre-shared keys, session resumption and 0-RTT data (both client and server |
| 210 | side). |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 211 | |
| 212 | - New TLS Message Processing Stack (MPS) |
| 213 | |
| 214 | The TLS 1.3 prototype is developed alongside a rewrite of the TLS messaging layer, |
| 215 | encompassing low-level details such as record parsing, handshake reassembly, and |
| 216 | DTLS retransmission state machine. |
| 217 | |
| 218 | MPS has the following components: |
| 219 | - Layer 1 (Datagram handling) |
| 220 | - Layer 2 (Record handling) |
| 221 | - Layer 3 (Message handling) |
| 222 | - Layer 4 (Retransmission State Machine) |
| 223 | - Reader (Abstracted pointer arithmetic and reassembly logic for incoming data) |
| 224 | - Writer (Abstracted pointer arithmetic and fragmentation logic for outgoing data) |
| 225 | |
| 226 | Of those components, the following have been upstreamed |
| 227 | as part of `MBEDTLS_SSL_PROTO_TLS1_3`: |
| 228 | |
| 229 | - Reader ([`library/mps_reader.h`](../../library/mps_reader.h)) |
| 230 | |
| 231 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 232 | Coding rules checklist for TLS 1.3 |
| 233 | ---------------------------------- |
| 234 | |
| 235 | The following coding rules are aimed to be a checklist for TLS 1.3 upstreaming |
| 236 | work to reduce review rounds and the number of comments in each round. They |
| 237 | come along (do NOT replace) the project coding rules |
| 238 | (https://tls.mbed.org/kb/development/mbedtls-coding-standards). They have been |
| 239 | established and discussed following the review of #4882 that was the |
| 240 | PR upstreaming the first part of TLS 1.3 ClientHello writing code. |
| 241 | |
| 242 | TLS 1.3 specific coding rules: |
| 243 | |
| 244 | - TLS 1.3 specific C modules, headers, static functions names are prefixed |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 245 | with `ssl_tls13_`. The same applies to structures and types that are |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 246 | internal to C modules. |
| 247 | |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 248 | - TLS 1.3 specific exported functions, structures and types are |
| 249 | prefixed with `mbedtls_ssl_tls13_`. |
| 250 | |
| 251 | - Use TLS1_3 in TLS 1.3 specific macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 252 | |
| 253 | - The names of macros and variables related to a field or structure in the |
| 254 | TLS 1.3 specification should contain as far as possible the field name as |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 255 | it is in the specification. If the field name is "too long" and we prefer |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 256 | to introduce some kind of abbreviation of it, use the same abbreviation |
| 257 | everywhere in the code. |
| 258 | |
| 259 | Example 1: #define CLIENT_HELLO_RANDOM_LEN 32, macro for the length of the |
| 260 | `random` field of the ClientHello message. |
| 261 | |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 262 | Example 2 (consistent abbreviation): `mbedtls_ssl_tls13_write_sig_alg_ext()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 263 | and `MBEDTLS_TLS_EXT_SIG_ALG`, `sig_alg` standing for |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 264 | `signature_algorithms`. |
| 265 | |
| 266 | - Regarding vectors that are represented by a length followed by their value |
| 267 | in the data exchanged between servers and clients: |
| 268 | |
| 269 | - Use `<vector name>_len` for the name of a variable used to compute the |
| 270 | length in bytes of the vector, where <vector name> is the name of the |
| 271 | vector as defined in the TLS 1.3 specification. |
| 272 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 273 | - Use `p_<vector_name>_len` for the name of a variable intended to hold |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 274 | the address of the first byte of the vector length. |
| 275 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 276 | - Use `<vector_name>` for the name of a variable intended to hold the |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 277 | address of the first byte of the vector value. |
| 278 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 279 | - Use `<vector_name>_end` for the name of a variable intended to hold |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 280 | the address of the first byte past the vector value. |
| 281 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 282 | Those idioms should lower the risk of mis-using one of the address in place |
| 283 | of another one which could potentially lead to some nasty issues. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 284 | |
| 285 | Example: `cipher_suites` vector of ClientHello in |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 286 | `ssl_tls13_write_client_hello_cipher_suites()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 287 | ``` |
| 288 | size_t cipher_suites_len; |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 289 | unsigned char *p_cipher_suites_len; |
| 290 | unsigned char *cipher_suites; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 291 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 292 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 293 | - Where applicable, use: |
| 294 | - the macros to extract a byte from a multi-byte integer MBEDTLS_BYTE_{0-8}. |
| 295 | - the macros to write in memory in big-endian order a multi-byte integer |
| 296 | MBEDTLS_PUT_UINT{8|16|32|64}_BE. |
| 297 | - the macros to read from memory a multi-byte integer in big-endian order |
| 298 | MBEDTLS_GET_UINT{8|16|32|64}_BE. |
| 299 | - the macro to check for space when writing into an output buffer |
| 300 | `MBEDTLS_SSL_CHK_BUF_PTR`. |
| 301 | - the macro to check for data when reading from an input buffer |
| 302 | `MBEDTLS_SSL_CHK_BUF_READ_PTR`. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 303 | |
| 304 | These macros were introduced after the prototype was written thus are |
| 305 | likely not to be used in prototype where we now would use them in |
| 306 | development. |
| 307 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 308 | The three first types, MBEDTLS_BYTE_{0-8}, MBEDTLS_PUT_UINT{8|16|32|64}_BE |
| 309 | and MBEDTLS_GET_UINT{8|16|32|64}_BE improve the readability of the code and |
| 310 | reduce the risk of writing or reading bytes in the wrong order. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 311 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 312 | The two last types, `MBEDTLS_SSL_CHK_BUF_PTR` and |
| 313 | `MBEDTLS_SSL_CHK_BUF_READ_PTR`, improve the readability of the code and |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 314 | reduce the risk of error in the non-completely-trivial arithmetic to |
| 315 | check that we do not write or read past the end of a data buffer. The |
| 316 | usage of those macros combined with the following rule mitigate the risk |
| 317 | to read/write past the end of a data buffer. |
| 318 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 319 | Examples: |
| 320 | ``` |
| 321 | hs_hdr[1] = MBEDTLS_BYTE_2( total_hs_len ); |
| 322 | MBEDTLS_PUT_UINT16_BE( MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS, p, 0 ); |
| 323 | MBEDTLS_SSL_CHK_BUF_PTR( p, end, 7 ); |
| 324 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 325 | |
| 326 | - To mitigate what happened here |
Dave Rodgman | 017a199 | 2022-03-31 14:07:01 +0100 | [diff] [blame^] | 327 | (https://github.com/Mbed-TLS/mbedtls/pull/4882#discussion_r701704527) from |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 328 | happening again, use always a local variable named `p` for the reading |
| 329 | pointer in functions parsing TLS 1.3 data, and for the writing pointer in |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 330 | functions writing data into an output buffer and only that variable. The |
| 331 | name `p` has been chosen as it was already widely used in TLS code. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 332 | |
| 333 | - When an TLS 1.3 structure is written or read by a function or as part of |
| 334 | a function, provide as documentation the definition of the structure as |
| 335 | it is in the TLS 1.3 specification. |
| 336 | |
| 337 | General coding rules: |
| 338 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 339 | - We prefer grouping "related statement lines" by not adding blank lines |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 340 | between them. |
| 341 | |
| 342 | Example 1: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 343 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 344 | ret = ssl_tls13_write_client_hello_cipher_suites( ssl, buf, end, &output_len ); |
| 345 | if( ret != 0 ) |
| 346 | return( ret ); |
| 347 | buf += output_len; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 348 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 349 | |
| 350 | Example 2: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 351 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 352 | MBEDTLS_SSL_CHK_BUF_PTR( cipher_suites_iter, end, 2 ); |
| 353 | MBEDTLS_PUT_UINT16_BE( cipher_suite, cipher_suites_iter, 0 ); |
| 354 | cipher_suites_iter += 2; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 355 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 356 | |
| 357 | - Use macros for constants that are used in different functions, different |
| 358 | places in the code. When a constant is used only locally in a function |
| 359 | (like the length in bytes of the vector lengths in functions reading and |
| 360 | writing TLS handshake message) there is no need to define a macro for it. |
| 361 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 362 | Example: `#define CLIENT_HELLO_RANDOM_LEN 32` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 363 | |
| 364 | - When declaring a pointer the dereferencing operator should be prepended to |
| 365 | the pointer name not appended to the pointer type: |
| 366 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 367 | Example: `mbedtls_ssl_context *ssl;` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 368 | |
| 369 | - Maximum line length is 80 characters. |
| 370 | |
| 371 | Exceptions: |
| 372 | |
| 373 | - string literals can extend beyond 80 characters as we do not want to |
| 374 | split them to ease their search in the code base. |
| 375 | |
| 376 | - A line can be more than 80 characters by a few characters if just looking |
| 377 | at the 80 first characters is enough to fully understand the line. For |
| 378 | example it is generally fine if some closure characters like ";" or ")" |
| 379 | are beyond the 80 characters limit. |
| 380 | |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 381 | If a line becomes too long due to a refactoring (for example renaming a |
| 382 | function to a longer name, or indenting a block more), avoid rewrapping |
| 383 | lines in the same commit: it makes the review harder. Make one commit with |
| 384 | the longer lines and another commit with just the rewrapping. |
| 385 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 386 | - When in successive lines, functions and macros parameters should be aligned |
| 387 | vertically. |
| 388 | |
| 389 | Example: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 390 | ``` |
Ronald Cron | 8f6d39a | 2022-03-10 18:56:50 +0100 | [diff] [blame] | 391 | int mbedtls_ssl_start_handshake_msg( mbedtls_ssl_context *ssl, |
| 392 | unsigned hs_type, |
| 393 | unsigned char **buf, |
| 394 | size_t *buf_len ); |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 395 | ``` |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 396 | |
| 397 | - When a function's parameters span several lines, group related parameters |
| 398 | together if possible. |
| 399 | |
| 400 | For example, prefer: |
| 401 | |
| 402 | ``` |
Ronald Cron | 8f6d39a | 2022-03-10 18:56:50 +0100 | [diff] [blame] | 403 | mbedtls_ssl_start_handshake_msg( ssl, hs_type, |
| 404 | buf, buf_len ); |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 405 | ``` |
| 406 | over |
| 407 | ``` |
Ronald Cron | 8f6d39a | 2022-03-10 18:56:50 +0100 | [diff] [blame] | 408 | mbedtls_ssl_start_handshake_msg( ssl, hs_type, buf, |
| 409 | buf_len ); |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 410 | ``` |
| 411 | even if it fits. |