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