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