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 | |
| 123 | - Supported versions: only TLS 1.3, version negotiation is not supported. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 124 | |
Ronald Cron | 3e7c403 | 2021-09-27 14:22:38 +0200 | [diff] [blame] | 125 | - Compatibility with existing SSL/TLS build options: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 126 | |
| 127 | The TLS 1.3 MVP is compatible with all TLS 1.2 configuration options in the |
| 128 | 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^] | 129 | modify the configuration for TLS 1.2. The MBEDTLS_USE_PSA_CRYPTO configuration |
| 130 | option is an exception though, the TLS 1.3 MVP is not compatible with it. |
| 131 | |
| 132 | Mbed TLS SSL/TLS related features are not supported or not applicable to the |
| 133 | TLS 1.3 MVP: |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 134 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 135 | | Mbed TLS configuration option | Support | |
| 136 | | ---------------------------------------- | ------- | |
| 137 | | MBEDTLS_SSL_ALL_ALERT_MESSAGES | no | |
| 138 | | MBEDTLS_SSL_ASYNC_PRIVATE | no | |
| 139 | | MBEDTLS_SSL_CONTEXT_SERIALIZATION | no | |
| 140 | | MBEDTLS_SSL_DEBUG_ALL | no | |
| 141 | | MBEDTLS_SSL_ENCRYPT_THEN_MAC | n/a | |
| 142 | | MBEDTLS_SSL_EXTENDED_MASTER_SECRET | n/a | |
| 143 | | MBEDTLS_SSL_KEEP_PEER_CERTIFICATE | no | |
| 144 | | MBEDTLS_SSL_RENEGOTIATION | n/a | |
| 145 | | MBEDTLS_SSL_MAX_FRAGMENT_LENGTH | no | |
| 146 | | | | |
| 147 | | MBEDTLS_SSL_SESSION_TICKETS | no | |
| 148 | | MBEDTLS_SSL_EXPORT_KEYS | no (1) | |
| 149 | | MBEDTLS_SSL_SERVER_NAME_INDICATION | no | |
| 150 | | MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH | no | |
| 151 | | | | |
| 152 | | MBEDTLS_ECP_RESTARTABLE | no | |
| 153 | | MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED | no | |
| 154 | | | | |
| 155 | | MBEDTLS_KEY_EXCHANGE_PSK_ENABLED | n/a (2) | |
| 156 | | MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED | n/a | |
| 157 | | MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED | n/a | |
| 158 | | MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED | n/a | |
| 159 | | MBEDTLS_KEY_EXCHANGE_RSA_ENABLED | n/a | |
| 160 | | MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED | n/a | |
| 161 | | MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED | n/a | |
| 162 | | MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED | n/a | |
| 163 | | MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED | n/a | |
| 164 | | MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED | n/a | |
| 165 | | MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED | n/a | |
| 166 | | | | |
Ronald Cron | 4279bac | 2022-02-11 15:08:48 +0100 | [diff] [blame^] | 167 | | MBEDTLS_USE_PSA_CRYPTO | no (3) | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 168 | |
Ronald Cron | 023987f | 2021-09-27 11:59:25 +0200 | [diff] [blame] | 169 | (1) Some support has already been upstreamed but it is incomplete. |
Ronald Cron | 1fa5088 | 2021-09-27 12:06:52 +0200 | [diff] [blame] | 170 | (2) Key exchange configuration options for TLS 1.3 will likely to be |
| 171 | organized around the notion of key exchange mode along the line |
Xiaofei Bai | 746f948 | 2021-11-12 08:53:56 +0000 | [diff] [blame] | 172 | 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] | 173 | runtime configuration macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 174 | |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 175 | - Quality considerations |
| 176 | - Standard Mbed TLS review bar |
| 177 | - Interoperability testing with OpenSSL and GnuTLS. Test with all the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 178 | cipher suites and signature algorithms supported by OpenSSL/GnuTLS server. |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 179 | - Negative testing against OpenSSL/GnuTLS servers with which the |
Ronald Cron | 7fc96c1 | 2021-09-28 15:54:57 +0200 | [diff] [blame] | 180 | handshake fails due to incompatibility with the capabilities of the |
Ronald Cron | 660c723 | 2021-09-27 13:40:53 +0200 | [diff] [blame] | 181 | MVP: TLS 1.2 or 1.1 server, server sending an HelloRetryRequest message in |
| 182 | response to the MVP ClientHello, server sending a CertificateRequest |
| 183 | message ... |
| 184 | |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 185 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 186 | Prototype upstreaming status |
| 187 | ---------------------------- |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 188 | |
Ronald Cron | b1822ef | 2021-12-10 14:28:13 +0100 | [diff] [blame] | 189 | The following summarizes which parts of the TLS 1.3 prototype remain to be |
| 190 | upstreamed: |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 191 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 192 | - Ephemeral only handshake on client side: client authentication, |
| 193 | HelloRetryRequest support, version negotiation. |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 194 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 195 | - Ephemeral only handshake server side. |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 196 | |
Ronald Cron | 7aa6fc1 | 2021-12-09 14:53:59 +0100 | [diff] [blame] | 197 | - Pre-shared keys, session resumption and 0-RTT data (both client and server |
| 198 | side). |
Ronald Cron | 653d5bc | 2021-12-09 14:35:56 +0100 | [diff] [blame] | 199 | |
| 200 | - New TLS Message Processing Stack (MPS) |
| 201 | |
| 202 | The TLS 1.3 prototype is developed alongside a rewrite of the TLS messaging layer, |
| 203 | encompassing low-level details such as record parsing, handshake reassembly, and |
| 204 | DTLS retransmission state machine. |
| 205 | |
| 206 | MPS has the following components: |
| 207 | - Layer 1 (Datagram handling) |
| 208 | - Layer 2 (Record handling) |
| 209 | - Layer 3 (Message handling) |
| 210 | - Layer 4 (Retransmission State Machine) |
| 211 | - Reader (Abstracted pointer arithmetic and reassembly logic for incoming data) |
| 212 | - Writer (Abstracted pointer arithmetic and fragmentation logic for outgoing data) |
| 213 | |
| 214 | Of those components, the following have been upstreamed |
| 215 | as part of `MBEDTLS_SSL_PROTO_TLS1_3`: |
| 216 | |
| 217 | - Reader ([`library/mps_reader.h`](../../library/mps_reader.h)) |
| 218 | |
| 219 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 220 | Coding rules checklist for TLS 1.3 |
| 221 | ---------------------------------- |
| 222 | |
| 223 | The following coding rules are aimed to be a checklist for TLS 1.3 upstreaming |
| 224 | work to reduce review rounds and the number of comments in each round. They |
| 225 | come along (do NOT replace) the project coding rules |
| 226 | (https://tls.mbed.org/kb/development/mbedtls-coding-standards). They have been |
| 227 | established and discussed following the review of #4882 that was the |
| 228 | PR upstreaming the first part of TLS 1.3 ClientHello writing code. |
| 229 | |
| 230 | TLS 1.3 specific coding rules: |
| 231 | |
| 232 | - TLS 1.3 specific C modules, headers, static functions names are prefixed |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 233 | with `ssl_tls13_`. The same applies to structures and types that are |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 234 | internal to C modules. |
| 235 | |
Ronald Cron | b194466 | 2021-09-27 13:56:46 +0200 | [diff] [blame] | 236 | - TLS 1.3 specific exported functions, structures and types are |
| 237 | prefixed with `mbedtls_ssl_tls13_`. |
| 238 | |
| 239 | - Use TLS1_3 in TLS 1.3 specific macros. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 240 | |
| 241 | - The names of macros and variables related to a field or structure in the |
| 242 | 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] | 243 | 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] | 244 | to introduce some kind of abbreviation of it, use the same abbreviation |
| 245 | everywhere in the code. |
| 246 | |
| 247 | Example 1: #define CLIENT_HELLO_RANDOM_LEN 32, macro for the length of the |
| 248 | `random` field of the ClientHello message. |
| 249 | |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 250 | Example 2 (consistent abbreviation): `mbedtls_ssl_tls13_write_sig_alg_ext()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 251 | and `MBEDTLS_TLS_EXT_SIG_ALG`, `sig_alg` standing for |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 252 | `signature_algorithms`. |
| 253 | |
| 254 | - Regarding vectors that are represented by a length followed by their value |
| 255 | in the data exchanged between servers and clients: |
| 256 | |
| 257 | - Use `<vector name>_len` for the name of a variable used to compute the |
| 258 | length in bytes of the vector, where <vector name> is the name of the |
| 259 | vector as defined in the TLS 1.3 specification. |
| 260 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 261 | - 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] | 262 | the address of the first byte of the vector length. |
| 263 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 264 | - 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] | 265 | address of the first byte of the vector value. |
| 266 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 267 | - 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] | 268 | the address of the first byte past the vector value. |
| 269 | |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 270 | Those idioms should lower the risk of mis-using one of the address in place |
| 271 | of another one which could potentially lead to some nasty issues. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 272 | |
| 273 | Example: `cipher_suites` vector of ClientHello in |
Dave Rodgman | c8aaac8 | 2021-10-18 12:56:53 +0100 | [diff] [blame] | 274 | `ssl_tls13_write_client_hello_cipher_suites()` |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 275 | ``` |
| 276 | size_t cipher_suites_len; |
Ronald Cron | 99733f0 | 2021-09-27 13:58:21 +0200 | [diff] [blame] | 277 | unsigned char *p_cipher_suites_len; |
| 278 | unsigned char *cipher_suites; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 279 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 280 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 281 | - Where applicable, use: |
| 282 | - the macros to extract a byte from a multi-byte integer MBEDTLS_BYTE_{0-8}. |
| 283 | - the macros to write in memory in big-endian order a multi-byte integer |
| 284 | MBEDTLS_PUT_UINT{8|16|32|64}_BE. |
| 285 | - the macros to read from memory a multi-byte integer in big-endian order |
| 286 | MBEDTLS_GET_UINT{8|16|32|64}_BE. |
| 287 | - the macro to check for space when writing into an output buffer |
| 288 | `MBEDTLS_SSL_CHK_BUF_PTR`. |
| 289 | - the macro to check for data when reading from an input buffer |
| 290 | `MBEDTLS_SSL_CHK_BUF_READ_PTR`. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 291 | |
| 292 | These macros were introduced after the prototype was written thus are |
| 293 | likely not to be used in prototype where we now would use them in |
| 294 | development. |
| 295 | |
Ronald Cron | fecda8d | 2021-09-27 13:59:38 +0200 | [diff] [blame] | 296 | The three first types, MBEDTLS_BYTE_{0-8}, MBEDTLS_PUT_UINT{8|16|32|64}_BE |
| 297 | and MBEDTLS_GET_UINT{8|16|32|64}_BE improve the readability of the code and |
| 298 | reduce the risk of writing or reading bytes in the wrong order. |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 299 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 300 | The two last types, `MBEDTLS_SSL_CHK_BUF_PTR` and |
| 301 | `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] | 302 | reduce the risk of error in the non-completely-trivial arithmetic to |
| 303 | check that we do not write or read past the end of a data buffer. The |
| 304 | usage of those macros combined with the following rule mitigate the risk |
| 305 | to read/write past the end of a data buffer. |
| 306 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 307 | Examples: |
| 308 | ``` |
| 309 | hs_hdr[1] = MBEDTLS_BYTE_2( total_hs_len ); |
| 310 | MBEDTLS_PUT_UINT16_BE( MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS, p, 0 ); |
| 311 | MBEDTLS_SSL_CHK_BUF_PTR( p, end, 7 ); |
| 312 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 313 | |
| 314 | - To mitigate what happened here |
| 315 | (https://github.com/ARMmbed/mbedtls/pull/4882#discussion_r701704527) from |
| 316 | happening again, use always a local variable named `p` for the reading |
| 317 | 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] | 318 | functions writing data into an output buffer and only that variable. The |
| 319 | 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] | 320 | |
| 321 | - When an TLS 1.3 structure is written or read by a function or as part of |
| 322 | a function, provide as documentation the definition of the structure as |
| 323 | it is in the TLS 1.3 specification. |
| 324 | |
| 325 | General coding rules: |
| 326 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 327 | - We prefer grouping "related statement lines" by not adding blank lines |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 328 | between them. |
| 329 | |
| 330 | Example 1: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 331 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 332 | ret = ssl_tls13_write_client_hello_cipher_suites( ssl, buf, end, &output_len ); |
| 333 | if( ret != 0 ) |
| 334 | return( ret ); |
| 335 | buf += output_len; |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 336 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 337 | |
| 338 | Example 2: |
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 | MBEDTLS_SSL_CHK_BUF_PTR( cipher_suites_iter, end, 2 ); |
| 341 | MBEDTLS_PUT_UINT16_BE( cipher_suite, cipher_suites_iter, 0 ); |
| 342 | cipher_suites_iter += 2; |
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 | |
| 345 | - Use macros for constants that are used in different functions, different |
| 346 | places in the code. When a constant is used only locally in a function |
| 347 | (like the length in bytes of the vector lengths in functions reading and |
| 348 | writing TLS handshake message) there is no need to define a macro for it. |
| 349 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 350 | Example: `#define CLIENT_HELLO_RANDOM_LEN 32` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 351 | |
| 352 | - When declaring a pointer the dereferencing operator should be prepended to |
| 353 | the pointer name not appended to the pointer type: |
| 354 | |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 355 | Example: `mbedtls_ssl_context *ssl;` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 356 | |
| 357 | - Maximum line length is 80 characters. |
| 358 | |
| 359 | Exceptions: |
| 360 | |
| 361 | - string literals can extend beyond 80 characters as we do not want to |
| 362 | split them to ease their search in the code base. |
| 363 | |
| 364 | - A line can be more than 80 characters by a few characters if just looking |
| 365 | at the 80 first characters is enough to fully understand the line. For |
| 366 | example it is generally fine if some closure characters like ";" or ")" |
| 367 | are beyond the 80 characters limit. |
| 368 | |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 369 | If a line becomes too long due to a refactoring (for example renaming a |
| 370 | function to a longer name, or indenting a block more), avoid rewrapping |
| 371 | lines in the same commit: it makes the review harder. Make one commit with |
| 372 | the longer lines and another commit with just the rewrapping. |
| 373 | |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 374 | - When in successive lines, functions and macros parameters should be aligned |
| 375 | vertically. |
| 376 | |
| 377 | Example: |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 378 | ``` |
Ronald Cron | 3785c90 | 2021-09-20 09:05:36 +0200 | [diff] [blame] | 379 | int mbedtls_ssl_tls13_start_handshake_msg( mbedtls_ssl_context *ssl, |
| 380 | unsigned hs_type, |
| 381 | unsigned char **buf, |
| 382 | size_t *buf_len ); |
Ronald Cron | 72064b3 | 2021-09-27 13:54:28 +0200 | [diff] [blame] | 383 | ``` |
Ronald Cron | 847c358 | 2021-09-27 14:24:43 +0200 | [diff] [blame] | 384 | |
| 385 | - When a function's parameters span several lines, group related parameters |
| 386 | together if possible. |
| 387 | |
| 388 | For example, prefer: |
| 389 | |
| 390 | ``` |
| 391 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, |
| 392 | buf, buf_len ); |
| 393 | ``` |
| 394 | over |
| 395 | ``` |
| 396 | mbedtls_ssl_tls13_start_handshake_msg( ssl, hs_type, buf, |
| 397 | buf_len ); |
| 398 | ``` |
| 399 | even if it fits. |