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 |
| 15 | development branch into the prototype. |
Hanno Becker | 9338f9f | 2020-05-31 07:39:50 +0100 | [diff] [blame] | 16 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 17 | |
| 18 | MVP definition |
| 19 | -------------- |
| 20 | |
Ronald Cron | f164b6a | 2021-09-27 15:36:29 +0200 | [diff] [blame] | 21 | - Overview |
| 22 | |
| 23 | - The TLS 1.3 MVP implements only the client side of the protocol. |
| 24 | |
| 25 | - The TLS 1.3 MVP supports ECDHE key establishment. |
| 26 | |
| 27 | - The TLS 1.3 MVP does not support DHE key establishment. |
| 28 | |
| 29 | - The TLS 1.3 MVP does not support pre-shared keys, including any form of |
| 30 | session resumption. This implies that it does not support sending early |
| 31 | data (0-RTT data). |
| 32 | |
| 33 | - The TLS 1.3 MVP supports the authentication of the server by the client |
| 34 | but does not support authentication of the client by the server. In terms |
| 35 | of TLS 1.3 authentication messages, this means that the TLS 1.3 MVP |
| 36 | supports the processing of the Certificate and CertificateVerify messages |
| 37 | but not of the CertificateRequest message. |
| 38 | |
| 39 | - The TLS 1.3 MVP does not support the handling of server HelloRetryRequest |
| 40 | message. In practice, this means that the handshake will fail if the MVP |
| 41 | does not provide in its ClientHello the shared secret associated to the |
| 42 | group selected by the server for key establishement. For more information, |
| 43 | see the comment associated to the `key_share` extension below. |
| 44 | |
| 45 | - If the TLS 1.3 MVP receives a HelloRetryRequest or a CertificateRequest |
| 46 | message, it aborts the handshake with an handshake_failure closure alert |
| 47 | and the `mbedtls_ssl_handshake()` returns in error with the |
| 48 | `MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE` error code. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 49 | |
| 50 | - Supported cipher suites: depends on the library configuration. Potentially |
| 51 | all of them: |
| 52 | TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, |
| 53 | TLS_AES_128_CCM_SHA256 and TLS_AES_128_CCM_8_SHA256. |
| 54 | |
| 55 | - Supported ClientHello extensions: |
| 56 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 57 | | Extension | MVP | Prototype (1) | |
| 58 | | ---------------------------- | ------- | ------------- | |
Ronald Cron | 85e5108 | 2021-09-27 12:13:16 +0200 | [diff] [blame] | 59 | | server_name | YES | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 60 | | max_fragment_length | no | YES | |
| 61 | | status_request | no | no | |
| 62 | | supported_groups | YES | YES | |
| 63 | | signature_algorithms | YES | YES | |
| 64 | | use_srtp | no | no | |
| 65 | | heartbeat | no | no | |
| 66 | | apln | no | YES | |
| 67 | | signed_certificate_timestamp | no | no | |
| 68 | | client_certificate_type | no | no | |
| 69 | | server_certificate_type | no | no | |
| 70 | | padding | no | no | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 71 | | key_share | YES (2) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 72 | | pre_shared_key | no | YES | |
| 73 | | psk_key_exchange_modes | no | YES | |
| 74 | | early_data | no | YES | |
| 75 | | cookie | no | YES | |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 76 | | supported_versions | YES (3) | YES | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 77 | | certificate_authorities | no | no | |
| 78 | | post_handshake_auth | no | no | |
| 79 | | signature_algorithms_cert | no | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 80 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 81 | (1) This is just for comparison. |
| 82 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame] | 83 | (2) The MVP sends only one shared secret corresponding to the configured |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 84 | preferred group. This could end up with connection failure if the |
| 85 | 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] | 86 | HelloRetryRequest. The preferred group is the group of the first curve in |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 87 | the list of allowed curves as defined by the configuration. The allowed |
| 88 | curves are by default ordered as follows: `x25519`, `secp256r1`, |
| 89 | `secp384r1` and finally `secp521r1`. Note that, in the absence of an |
| 90 | application profile standard specifying otherwise, section 9.1 of the |
| 91 | specification rather promotes curve `secp256r1` to be supported over |
Paul Elliott | cce0f5a | 2021-12-03 16:13:30 +0000 | [diff] [blame] | 92 | curve `x25519`. The MVP would, however, rather keep the preference order |
Paul Elliott | c0d335b | 2021-12-02 16:38:05 +0000 | [diff] [blame] | 93 | currently promoted by Mbed TLS as this applies to TLS 1.2 as well, and |
| 94 | changing the order only for TLS1.3 would be potentially difficult. |
| 95 | In the unlikely event a server does not support curve `x25519` but does |
| 96 | support curve `secp256r1`, curve `secp256r1` can be set as the preferred |
| 97 | curve through the `mbedtls_ssl_conf_curves()` API. |
Ronald Cron | 3160d70 | 2021-09-27 13:27:21 +0200 | [diff] [blame] | 98 | |
Paul Elliott | 66491c7 | 2021-11-26 20:24:43 +0000 | [diff] [blame] | 99 | (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] | 100 | Out-of-protocol fallback is supported though if the Mbed TLS library |
| 101 | has been built to support both TLS 1.3 and TLS 1.2: just set the |
| 102 | maximum of the minor version of the SSL configuration to |
| 103 | MBEDTLS_SSL_MINOR_VERSION_3 (`mbedtls_ssl_conf_min_version()` API) and |
| 104 | re-initiate a server handshake. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 105 | |
| 106 | - Supported groups: depends on the library configuration. |
Ronald Cron | 8ee9ed6 | 2021-09-28 14:46:43 +0200 | [diff] [blame] | 107 | Potentially all ECDHE groups but x448: |
| 108 | secp256r1, x25519, secp384r1 and secp521r1. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 109 | |
| 110 | Finite field groups (DHE) are not supported. |
| 111 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 112 | - Supported signature algorithms (both for certificates and CertificateVerify): |
| 113 | depends on the library configuration. |
| 114 | Potentially: |
| 115 | rsa_pkcs1_sha256, rsa_pss_rsae_sha256, ecdsa_secp256r1_sha256, |
| 116 | ecdsa_secp384r1_sha384 and ecdsa_secp521r1_sha512. |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 117 | |
Ronald Cron | fb87721 | 2021-09-28 15:49:39 +0200 | [diff] [blame] | 118 | Note that in absence of an application profile standard specifying otherwise |
| 119 | the three first ones in the list above are mandatory (see section 9.1 of the |
| 120 | specification). |
Ronald Cron | c3b510f | 2021-09-27 13:36:33 +0200 | [diff] [blame] | 121 | |
| 122 | - Supported versions: only TLS 1.3, version negotiation is not supported. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 123 | |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 124 | - Compatibility with existing SSL/TLS build options: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 125 | |
| 126 | The TLS 1.3 MVP is compatible with all TLS 1.2 configuration options in the |
| 127 | sense that when enabling the TLS 1.3 MVP in the library there is no need to |
| 128 | modify the configuration for TLS 1.2. Mbed TLS SSL/TLS related features are |
| 129 | not supported or not applicable to the TLS 1.3 MVP: |
| 130 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 131 | | Mbed TLS configuration option | Support | |
| 132 | | ---------------------------------------- | ------- | |
| 133 | | MBEDTLS_SSL_ALL_ALERT_MESSAGES | no | |
| 134 | | MBEDTLS_SSL_ASYNC_PRIVATE | no | |
| 135 | | MBEDTLS_SSL_CONTEXT_SERIALIZATION | no | |
| 136 | | MBEDTLS_SSL_DEBUG_ALL | no | |
| 137 | | MBEDTLS_SSL_ENCRYPT_THEN_MAC | n/a | |
| 138 | | MBEDTLS_SSL_EXTENDED_MASTER_SECRET | n/a | |
| 139 | | MBEDTLS_SSL_KEEP_PEER_CERTIFICATE | no | |
| 140 | | MBEDTLS_SSL_RENEGOTIATION | n/a | |
| 141 | | MBEDTLS_SSL_MAX_FRAGMENT_LENGTH | no | |
| 142 | | | | |
| 143 | | MBEDTLS_SSL_SESSION_TICKETS | no | |
| 144 | | MBEDTLS_SSL_EXPORT_KEYS | no (1) | |
| 145 | | MBEDTLS_SSL_SERVER_NAME_INDICATION | no | |
| 146 | | MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH | no | |
| 147 | | | | |
| 148 | | MBEDTLS_ECP_RESTARTABLE | no | |
| 149 | | MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED | no | |
| 150 | | | | |
| 151 | | MBEDTLS_KEY_EXCHANGE_PSK_ENABLED | n/a (2) | |
| 152 | | MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED | n/a | |
| 153 | | MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED | n/a | |
| 154 | | MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED | n/a | |
| 155 | | MBEDTLS_KEY_EXCHANGE_RSA_ENABLED | n/a | |
| 156 | | MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED | n/a | |
| 157 | | MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED | n/a | |
| 158 | | MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED | n/a | |
| 159 | | MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED | n/a | |
| 160 | | MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED | n/a | |
| 161 | | MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED | n/a | |
| 162 | | | | |
| 163 | | MBEDTLS_USE_PSA_CRYPTO | no | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 164 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 165 | (1) Some support has already been upstreamed but it is incomplete. |
Ronald Cron | 1fa5088 | 2021-09-27 12:06:52 +0200 | [diff] [blame] | 166 | (2) Key exchange configuration options for TLS 1.3 will likely to be |
| 167 | organized around the notion of key exchange mode along the line |
Xiaofei Bai | 746f948 | 2021-11-12 08:53:56 +0000 | [diff] [blame] | 168 | 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] | 169 | runtime configuration macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 170 | |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 171 | - Quality considerations |
| 172 | - Standard Mbed TLS review bar |
| 173 | - Interoperability testing with OpenSSL and GnuTLS. Test with all the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 174 | cipher suites and signature algorithms supported by OpenSSL/GnuTLS server. |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 175 | - Negative testing against OpenSSL/GnuTLS servers with which the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 176 | handshake fails due to incompatibility with the capabilities of the |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 177 | MVP: TLS 1.2 or 1.1 server, server sending an HelloRetryRequest message in |
| 178 | response to the MVP ClientHello, server sending a CertificateRequest |
| 179 | message ... |
| 180 | |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame^] | 181 | |
| 182 | Status |
| 183 | ------ |
| 184 | |
| 185 | The following lists which parts of the TLS 1.3 prototype have already been upstreamed |
| 186 | together with their level of testing: |
| 187 | |
| 188 | * TLS 1.3 record protection mechanisms |
| 189 | |
| 190 | The record protection routines `mbedtls_ssl_{encrypt|decrypt}_buf()` have been extended |
| 191 | to support the modified TLS 1.3 record protection mechanism, including modified computation |
| 192 | of AAD, IV, and the introduction of a flexible padding. |
| 193 | |
| 194 | Those record protection routines have unit tests in `test_suite_ssl` alongside the |
| 195 | tests for the other record protection routines. |
| 196 | |
| 197 | TODO: Add some test vectors from RFC 8448. |
| 198 | |
| 199 | - The HKDF key derivation function on which the TLS 1.3 key schedule is based, |
| 200 | is already present as an independent module controlled by `MBEDTLS_HKDF_C` |
| 201 | independently of the development of the TLS 1.3 prototype. |
| 202 | |
| 203 | - The TLS 1.3-specific HKDF-based key derivation functions (see RFC 8446): |
| 204 | * HKDF-Expand-Label |
| 205 | * Derive-Secret |
| 206 | - Secret evolution |
| 207 | * The traffic {Key,IV} generation from secret |
| 208 | Those functions are implemented in `library/ssl_tls13_keys.c` and |
| 209 | tested in `test_suite_ssl` using test vectors from RFC 8448 and |
| 210 | https://tls13.ulfheim.net/. |
| 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 |
| 327 | (https://github.com/ARMmbed/mbedtls/pull/4882#discussion_r701704527) from |
| 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 | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 391 | int mbedtls_ssl_tls13_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 | ``` |
| 403 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, |
| 404 | buf, buf_len ); |
| 405 | ``` |
| 406 | over |
| 407 | ``` |
| 408 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, buf, |
| 409 | buf_len ); |
| 410 | ``` |
| 411 | even if it fits. |