From f5d7eef11ff08603acaa5eea51a23385204de23c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Nov 2021 22:12:47 +0100 Subject: [PATCH 01/17] PSA operation structures: move less-used fields to the end Move fields around to have fewer accesses outside the 128-element Thumb direct access window. In psa_hkdf_key_derivation_t, move the large fields (output_block, prk, hmac) after the state bit-fields. Experimentally, it's slightly better to put hmac last. In aead_operation_t, tag_length was outside the window. The details depend on the sizes of contexts included in ctx. Make the large ctx be the last field. In mbedtls_psa_hmac_operation_t, the opad field is outside the window when SHA-512 is enabled. Moving opad before hash_ctx only saves 4 bytes and made the structure clumsy, so I left it alone. Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/psa_crypto.o: 16246 -> 16166 (diff: 80) library/psa_crypto_aead.o: 952 -> 928 (diff: 24) Signed-off-by: Gilles Peskine --- include/psa/crypto_struct.h | 6 +++--- library/psa_crypto_aead.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/psa/crypto_struct.h b/include/psa/crypto_struct.h index 94242f897..f08fdc8bd 100644 --- a/include/psa/crypto_struct.h +++ b/include/psa/crypto_struct.h @@ -175,9 +175,6 @@ typedef struct { uint8_t *info; size_t info_length; - psa_mac_operation_t hmac; - uint8_t prk[PSA_HASH_MAX_SIZE]; - uint8_t output_block[PSA_HASH_MAX_SIZE]; #if PSA_HASH_MAX_SIZE > 0xff #error "PSA_HASH_MAX_SIZE does not fit in uint8_t" #endif @@ -185,6 +182,9 @@ typedef struct uint8_t block_number; unsigned int state : 2; unsigned int info_set : 1; + uint8_t output_block[PSA_HASH_MAX_SIZE]; + uint8_t prk[PSA_HASH_MAX_SIZE]; + psa_mac_operation_t hmac; } psa_hkdf_key_derivation_t; #endif /* MBEDTLS_PSA_BUILTIN_ALG_HKDF */ diff --git a/library/psa_crypto_aead.c b/library/psa_crypto_aead.c index 356679c38..2769028d0 100644 --- a/library/psa_crypto_aead.c +++ b/library/psa_crypto_aead.c @@ -32,6 +32,8 @@ typedef struct { + psa_algorithm_t core_alg; + uint8_t tag_length; union { unsigned dummy; /* Make the union non-empty even with no supported algorithms. */ @@ -45,11 +47,9 @@ typedef struct mbedtls_chachapoly_context chachapoly; #endif /* MBEDTLS_PSA_BUILTIN_ALG_CHACHA20_POLY1305 */ } ctx; - psa_algorithm_t core_alg; - uint8_t tag_length; } aead_operation_t; -#define AEAD_OPERATION_INIT {{0}, 0, 0} +#define AEAD_OPERATION_INIT {0, 0, {0}} static void psa_aead_abort_internal( aead_operation_t *operation ) { From b8006a66f243b5d48fc968e3fb0606b6b0fe3f48 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 8 Nov 2021 22:20:03 +0100 Subject: [PATCH 02/17] PSA global data: move fields around to save code size Move fields around to have fewer accesses outside the 128-element Thumb direct access window. In psa_crypto.c's global_data, put the state fields first (-20). In psa_crypto_slot_management.c's global_data, keep the key slots first (otherwise it's +24). In mbedtls_psa_random_context_t, swapping entropy and drbg makes no difference (at least when the DRBG is mbedtls_ctr_drbg_context). Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/psa_crypto.o: 16166 -> 16146 (diff: 20) Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 406e6c4cf..e85bbb8d5 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -108,9 +108,9 @@ static int key_type_is_raw_bytes( psa_key_type_t type ) typedef struct { - mbedtls_psa_random_context_t rng; unsigned initialized : 1; unsigned rng_state : 2; + mbedtls_psa_random_context_t rng; } psa_global_data_t; static psa_global_data_t global_data; From 4a13ebff39a552dfdcf3b209126a5beef1fe0297 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 15:21:44 +0100 Subject: [PATCH 03/17] Tweak whitespace for readability Signed-off-by: Gilles Peskine --- include/mbedtls/ssl_internal.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 2097a6dd9..03807f3a7 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -434,9 +434,11 @@ struct mbedtls_ssl_handshake_params defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) mbedtls_ssl_sig_hash_set_t hash_algs; /*!< Set of suitable sig-hash pairs */ #endif + #if defined(MBEDTLS_DHM_C) mbedtls_dhm_context dhm_ctx; /*!< DHM key exchange */ #endif + /* Adding guard for MBEDTLS_ECDSA_C to ensure no compile errors due * to guards also being in ssl_srv.c and ssl_cli.c. There is a gap * in functionality that access to ecdh_ctx structure is needed for @@ -461,10 +463,12 @@ struct mbedtls_ssl_handshake_params size_t ecjpake_cache_len; /*!< Length of cached data */ #endif #endif /* MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED */ -#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ + +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) const mbedtls_ecp_curve_info **curves; /*!< Supported elliptic curves */ #endif + #if defined(MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED) #if defined(MBEDTLS_USE_PSA_CRYPTO) psa_key_id_t psk_opaque; /*!< Opaque PSK from the callback */ @@ -472,6 +476,7 @@ struct mbedtls_ssl_handshake_params unsigned char *psk; /*!< PSK from the callback */ size_t psk_len; /*!< Length of PSK from callback */ #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED */ + #if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_ssl_key_cert *key_cert; /*!< chosen key/cert pair (server) */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) @@ -481,6 +486,7 @@ struct mbedtls_ssl_handshake_params mbedtls_x509_crl *sni_ca_crl; /*!< trusted CAs CRLs from SNI */ #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ + #if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) int ecrs_enabled; /*!< Handshake supports EC restart? */ mbedtls_x509_crt_restart_ctx ecrs_ctx; /*!< restart context */ @@ -494,10 +500,12 @@ struct mbedtls_ssl_handshake_params mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ size_t ecrs_n; /*!< place for saving a length */ #endif -#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) mbedtls_pk_context peer_pubkey; /*!< The public key from the peer. */ #endif /* MBEDTLS_X509_CRT_PARSE_C && !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + #if defined(MBEDTLS_SSL_PROTO_DTLS) unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ @@ -565,8 +573,8 @@ struct mbedtls_ssl_handshake_params */ #if defined(MBEDTLS_SSL_PROTO_SSL3) || defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) - mbedtls_md5_context fin_md5; - mbedtls_sha1_context fin_sha1; + mbedtls_md5_context fin_md5; + mbedtls_sha1_context fin_sha1; #endif #if defined(MBEDTLS_SSL_PROTO_TLS1_2) #if defined(MBEDTLS_SHA256_C) @@ -606,6 +614,7 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL_SESSION_TICKETS) int new_session_ticket; /*!< use NewSessionTicket? */ #endif /* MBEDTLS_SSL_SESSION_TICKETS */ + #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) int extended_ms; /*!< use Extended Master Secret? */ #endif From 51070849faccc7d3078607dbbbed618d1964bff4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 16:34:14 +0100 Subject: [PATCH 04/17] mbedtls_ssl_handshake_params: use bytes for some small values Replace bitfields mbedtls_ssl_handshake_params by bytes. This saves some code size, and since the bitfields weren't group, this doesn't increase the RAM usage. Replace several ints that only store values in the range 0..255 by uint8_t. This can increase or decrease the code size depending on the architecture and on how the field is used. I chose changes that save code size on Arm Thumb builds and may potentially save more after field reordering. Leave the bitfields in struct mbedtls_ssl_hs_buffer alone: replacing them by uint8_t slightly increases the code size. Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/ssl_srv.o: 22735 -> 22691 (diff: 44) library/ssl_tls.o: 23566 -> 23570 (diff: -4) Signed-off-by: Gilles Peskine --- include/mbedtls/ssl_internal.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 03807f3a7..fc268566e 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -480,7 +480,7 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_ssl_key_cert *key_cert; /*!< chosen key/cert pair (server) */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - int sni_authmode; /*!< authmode from SNI callback */ + uint8_t sni_authmode; /*!< authmode from SNI callback */ mbedtls_ssl_key_cert *sni_key_cert; /*!< key/cert list from SNI */ mbedtls_x509_crt *sni_ca_chain; /*!< trusted CAs from SNI callback */ mbedtls_x509_crl *sni_ca_crl; /*!< trusted CAs CRLs from SNI */ @@ -488,8 +488,8 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) - int ecrs_enabled; /*!< Handshake supports EC restart? */ mbedtls_x509_crt_restart_ctx ecrs_ctx; /*!< restart context */ + uint8_t ecrs_enabled; /*!< Handshake supports EC restart? */ enum { /* this complements ssl->state with info on intra-state operations */ ssl_ecrs_none = 0, /*!< nothing going on (yet) */ ssl_ecrs_crt_verify, /*!< Certificate: crt_verify() */ @@ -606,21 +606,21 @@ struct mbedtls_ssl_handshake_params unsigned char premaster[MBEDTLS_PREMASTER_SIZE]; /*!< premaster secret */ - int resume; /*!< session resume indicator*/ - int max_major_ver; /*!< max. major version client*/ - int max_minor_ver; /*!< max. minor version client*/ - int cli_exts; /*!< client extension presence*/ + uint8_t max_major_ver; /*!< max. major version client*/ + uint8_t max_minor_ver; /*!< max. minor version client*/ + uint8_t resume; /*!< session resume indicator*/ + uint8_t cli_exts; /*!< client extension presence*/ #if defined(MBEDTLS_SSL_SESSION_TICKETS) - int new_session_ticket; /*!< use NewSessionTicket? */ + uint8_t new_session_ticket; /*!< use NewSessionTicket? */ #endif /* MBEDTLS_SSL_SESSION_TICKETS */ #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - int extended_ms; /*!< use Extended Master Secret? */ + uint8_t extended_ms; /*!< use Extended Master Secret? */ #endif #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) - unsigned int async_in_progress : 1; /*!< an asynchronous operation is in progress */ + uint8_t async_in_progress; /*!< an asynchronous operation is in progress */ #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) From baccfef741ea9e749a92b84f63575596a3340064 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 17:44:31 +0100 Subject: [PATCH 05/17] mbedtls_ssl_handshake_params: reorder fields to save code size Reorder fields mbedtls_ssl_handshake_params in order to save code on Arm Thumb builds. The general idea is to put often-used fields in the direct access window of 128 elements from the beginning of the structure. The reordering is a human selection based on a report of field offset and use counts, and informed by measuring the code size with various arrangements. Some notes: * I moved most byte-sized fields at the beginning where they're sure to be in the direct access window. * I moved buffering earlier because it can be around the threshold depending on the configuration, and it's accessed in a lot of places. * I moved several fields, including update_checksum and friends, early so that they're guaranteed to be in the early access window. * I tried moving randbytes or premaster to the early access window, but I couldn't find a placement which would save code size, presumably because they're bumping too many other fields, and they're mostly accessed through memcpy and friends which translates to instructions that don't have an offset for free anyway. Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/ssl_cli.o: 20200 -> 20104 (diff: 96) library/ssl_msg.o: 25978 -> 25942 (diff: 36) library/ssl_srv.o: 22691 -> 22467 (diff: 224) library/ssl_tls.o: 23570 -> 23390 (diff: 180) Results (same architecture, config-suite-b.h + MBEDTLS_ECDH_LEGACY_CONTEXT + MBEDTLS_ECP_RESTARTABLE): library/ssl_cli.o: 3012 -> 2928 (diff: 84) library/ssl_msg.o: 2932 -> 2924 (diff: 8) library/ssl_srv.o: 3288 -> 3232 (diff: 56) library/ssl_tls.o: 6032 -> 5904 (diff: 128) Signed-off-by: Gilles Peskine --- include/mbedtls/ssl_internal.h | 150 ++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 70 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index fc268566e..259d84677 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -430,11 +430,59 @@ struct mbedtls_ssl_handshake_params * Handshake specific crypto variables */ -#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ + uint8_t max_major_ver; /*!< max. major version client*/ + uint8_t max_minor_ver; /*!< max. minor version client*/ + uint8_t resume; /*!< session resume indicator*/ + uint8_t cli_exts; /*!< client extension presence*/ + +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + uint8_t sni_authmode; /*!< authmode from SNI callback */ +#endif + +#if defined(MBEDTLS_SSL_SESSION_TICKETS) + uint8_t new_session_ticket; /*!< use NewSessionTicket? */ +#endif /* MBEDTLS_SSL_SESSION_TICKETS */ + +#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) + uint8_t extended_ms; /*!< use Extended Master Secret? */ +#endif + +#if defined(MBEDTLS_SSL_ASYNC_PRIVATE) + uint8_t async_in_progress; /*!< an asynchronous operation is in progress */ +#endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + unsigned char retransmit_state; /*!< Retransmission state */ +#endif + +#if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) + uint8_t ecrs_enabled; /*!< Handshake supports EC restart? */ + enum { /* this complements ssl->state with info on intra-state operations */ + ssl_ecrs_none = 0, /*!< nothing going on (yet) */ + ssl_ecrs_crt_verify, /*!< Certificate: crt_verify() */ + ssl_ecrs_ske_start_processing, /*!< ServerKeyExchange: pk_verify() */ + ssl_ecrs_cke_ecdh_calc_secret, /*!< ClientKeyExchange: ECDH step 2 */ + ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ + } ecrs_state; /*!< current (or last) operation */ + mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ + size_t ecrs_n; /*!< place for saving a length */ +#endif + +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) mbedtls_ssl_sig_hash_set_t hash_algs; /*!< Set of suitable sig-hash pairs */ #endif + size_t pmslen; /*!< premaster length */ + + mbedtls_ssl_ciphersuite_t const *ciphersuite_info; + + void (*update_checksum)(mbedtls_ssl_context *, const unsigned char *, size_t); + void (*calc_verify)(const mbedtls_ssl_context *, unsigned char *, size_t *); + void (*calc_finished)(mbedtls_ssl_context *, unsigned char *, int); + mbedtls_ssl_tls_prf_cb *tls_prf; + #if defined(MBEDTLS_DHM_C) mbedtls_dhm_context dhm_ctx; /*!< DHM key exchange */ #endif @@ -480,7 +528,6 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_ssl_key_cert *key_cert; /*!< chosen key/cert pair (server) */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - uint8_t sni_authmode; /*!< authmode from SNI callback */ mbedtls_ssl_key_cert *sni_key_cert; /*!< key/cert list from SNI */ mbedtls_x509_crt *sni_ca_chain; /*!< trusted CAs from SNI callback */ mbedtls_x509_crl *sni_ca_crl; /*!< trusted CAs CRLs from SNI */ @@ -489,16 +536,6 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) mbedtls_x509_crt_restart_ctx ecrs_ctx; /*!< restart context */ - uint8_t ecrs_enabled; /*!< Handshake supports EC restart? */ - enum { /* this complements ssl->state with info on intra-state operations */ - ssl_ecrs_none = 0, /*!< nothing going on (yet) */ - ssl_ecrs_crt_verify, /*!< Certificate: crt_verify() */ - ssl_ecrs_ske_start_processing, /*!< ServerKeyExchange: pk_verify() */ - ssl_ecrs_cke_ecdh_calc_secret, /*!< ClientKeyExchange: ECDH step 2 */ - ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ - } ecrs_state; /*!< current (or last) operation */ - mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ - size_t ecrs_n; /*!< place for saving a length */ #endif #if defined(MBEDTLS_X509_CRT_PARSE_C) && \ @@ -507,38 +544,6 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_X509_CRT_PARSE_C && !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #if defined(MBEDTLS_SSL_PROTO_DTLS) - unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ - unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ - - unsigned char *verify_cookie; /*!< Cli: HelloVerifyRequest cookie - Srv: unused */ - unsigned char verify_cookie_len; /*!< Cli: cookie length - Srv: flag for sending a cookie */ - - uint32_t retransmit_timeout; /*!< Current value of timeout */ - unsigned char retransmit_state; /*!< Retransmission state */ - mbedtls_ssl_flight_item *flight; /*!< Current outgoing flight */ - mbedtls_ssl_flight_item *cur_msg; /*!< Current message in flight */ - unsigned char *cur_msg_p; /*!< Position in current message */ - unsigned int in_flight_start_seq; /*!< Minimum message sequence in the - flight being received */ - mbedtls_ssl_transform *alt_transform_out; /*!< Alternative transform for - resending messages */ - unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter - for resending messages */ - -#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - /* The state of CID configuration in this handshake. */ - - uint8_t cid_in_use; /*!< This indicates whether the use of the CID extension - * has been negotiated. Possible values are - * #MBEDTLS_SSL_CID_ENABLED and - * #MBEDTLS_SSL_CID_DISABLED. */ - unsigned char peer_cid[ MBEDTLS_SSL_CID_OUT_LEN_MAX ]; /*! The peer's CID */ - uint8_t peer_cid_len; /*!< The length of - * \c peer_cid. */ -#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ - struct { size_t total_bytes_buffered; /*!< Cumulative size of heap allocated @@ -565,6 +570,37 @@ struct mbedtls_ssl_handshake_params } buffering; + unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ + unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ + + unsigned char *verify_cookie; /*!< Cli: HelloVerifyRequest cookie + Srv: unused */ + unsigned char verify_cookie_len; /*!< Cli: cookie length + Srv: flag for sending a cookie */ + + uint32_t retransmit_timeout; /*!< Current value of timeout */ + mbedtls_ssl_flight_item *flight; /*!< Current outgoing flight */ + mbedtls_ssl_flight_item *cur_msg; /*!< Current message in flight */ + unsigned char *cur_msg_p; /*!< Position in current message */ + unsigned int in_flight_start_seq; /*!< Minimum message sequence in the + flight being received */ + mbedtls_ssl_transform *alt_transform_out; /*!< Alternative transform for + resending messages */ + unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter + for resending messages */ + +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + /* The state of CID configuration in this handshake. */ + + uint8_t cid_in_use; /*!< This indicates whether the use of the CID extension + * has been negotiated. Possible values are + * #MBEDTLS_SSL_CID_ENABLED and + * #MBEDTLS_SSL_CID_DISABLED. */ + unsigned char peer_cid[ MBEDTLS_SSL_CID_OUT_LEN_MAX ]; /*! The peer's CID */ + uint8_t peer_cid_len; /*!< The length of + * \c peer_cid. */ +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ + uint16_t mtu; /*!< Handshake mtu, used to fragment outgoing messages */ #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -593,36 +629,10 @@ struct mbedtls_ssl_handshake_params #endif #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ - void (*update_checksum)(mbedtls_ssl_context *, const unsigned char *, size_t); - void (*calc_verify)(const mbedtls_ssl_context *, unsigned char *, size_t *); - void (*calc_finished)(mbedtls_ssl_context *, unsigned char *, int); - mbedtls_ssl_tls_prf_cb *tls_prf; - - mbedtls_ssl_ciphersuite_t const *ciphersuite_info; - - size_t pmslen; /*!< premaster length */ - unsigned char randbytes[64]; /*!< random bytes */ unsigned char premaster[MBEDTLS_PREMASTER_SIZE]; /*!< premaster secret */ - uint8_t max_major_ver; /*!< max. major version client*/ - uint8_t max_minor_ver; /*!< max. minor version client*/ - uint8_t resume; /*!< session resume indicator*/ - uint8_t cli_exts; /*!< client extension presence*/ - -#if defined(MBEDTLS_SSL_SESSION_TICKETS) - uint8_t new_session_ticket; /*!< use NewSessionTicket? */ -#endif /* MBEDTLS_SSL_SESSION_TICKETS */ - -#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - uint8_t extended_ms; /*!< use Extended Master Secret? */ -#endif - -#if defined(MBEDTLS_SSL_ASYNC_PRIVATE) - uint8_t async_in_progress; /*!< an asynchronous operation is in progress */ -#endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ - #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) /** Asynchronous operation context. This field is meant for use by the * asynchronous operation callbacks (mbedtls_ssl_config::f_async_sign_start, From 9a0e0affef2277dc3417ed9f8b14891c9dda57c6 Mon Sep 17 00:00:00 2001 From: Lukasz Gniadzik Date: Tue, 17 Aug 2021 17:34:08 +0200 Subject: [PATCH 06/17] mbedtls_ssl_config, mbedtls_ssl_session: reorder fields Move small fields first so that more fields can be within the Arm Thumb 128-element direct access window. The ordering in this commit is not based on field access frequency. Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/ssl_cli.o: 20104 -> 19952 (diff: 152) library/ssl_msg.o: 25942 -> 25810 (diff: 132) library/ssl_srv.o: 22467 -> 22371 (diff: 96) library/ssl_tls.o: 23390 -> 23274 (diff: 116) Results (same architecture, config-suite-b.h + MBEDTLS_ECDH_LEGACY_CONTEXT + MBEDTLS_ECP_RESTARTABLE): library/ssl_cli.o: 2928 -> 2868 (diff: 60) library/ssl_msg.o: 2924 -> 2916 (diff: 8) library/ssl_srv.o: 3232 -> 3204 (diff: 28) library/ssl_tls.o: 5904 -> 5860 (diff: 44) Signed-off-by: Lukasz Gniadzik Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 184 +++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 90 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 2ed295a1f..ef6037986 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -974,6 +974,10 @@ mbedtls_dtls_srtp_info; */ struct mbedtls_ssl_session { +#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) + unsigned char mfl_code; /*!< MaxFragmentLength negotiated by peer */ +#endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ + #if defined(MBEDTLS_HAVE_TIME) mbedtls_time_t start; /*!< starting time */ #endif @@ -1002,10 +1006,6 @@ struct mbedtls_ssl_session uint32_t ticket_lifetime; /*!< ticket lifetime hint */ #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ -#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) - unsigned char mfl_code; /*!< MaxFragmentLength negotiated by peer */ -#endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ - #if defined(MBEDTLS_SSL_TRUNCATED_HMAC) int trunc_hmac; /*!< flag for truncated hmac activation */ #endif /* MBEDTLS_SSL_TRUNCATED_HMAC */ @@ -1020,7 +1020,96 @@ struct mbedtls_ssl_session */ struct mbedtls_ssl_config { - /* Group items by size (largest first) to minimize padding overhead */ + /* Group items by size and reorder them to maximize usage of immediate offset access. */ + + /* + * Numerical settings (char) + */ + + unsigned char max_major_ver; /*!< max. major version used */ + unsigned char max_minor_ver; /*!< max. minor version used */ + unsigned char min_major_ver; /*!< min. major version used */ + unsigned char min_minor_ver; /*!< min. minor version used */ + + /* + * Flags (bitfields) + */ + + unsigned int endpoint : 1; /*!< 0: client, 1: server */ + unsigned int transport : 1; /*!< stream (TLS) or datagram (DTLS) */ + unsigned int authmode : 2; /*!< MBEDTLS_SSL_VERIFY_XXX */ + /* needed even with renego disabled for LEGACY_BREAK_HANDSHAKE */ + unsigned int allow_legacy_renegotiation : 2 ; /*!< MBEDTLS_LEGACY_XXX */ +#if defined(MBEDTLS_ARC4_C) + unsigned int arc4_disabled : 1; /*!< blacklist RC4 ciphersuites? */ +#endif +#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) + unsigned int mfl_code : 3; /*!< desired fragment length */ +#endif +#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) + unsigned int encrypt_then_mac : 1 ; /*!< negotiate encrypt-then-mac? */ +#endif +#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) + unsigned int extended_ms : 1; /*!< negotiate extended master secret? */ +#endif +#if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) + unsigned int anti_replay : 1; /*!< detect and prevent replay? */ +#endif +#if defined(MBEDTLS_SSL_CBC_RECORD_SPLITTING) + unsigned int cbc_record_splitting : 1; /*!< do cbc record splitting */ +#endif +#if defined(MBEDTLS_SSL_RENEGOTIATION) + unsigned int disable_renegotiation : 1; /*!< disable renegotiation? */ +#endif +#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) + unsigned int trunc_hmac : 1; /*!< negotiate truncated hmac? */ +#endif +#if defined(MBEDTLS_SSL_SESSION_TICKETS) + unsigned int session_tickets : 1; /*!< use session tickets? */ +#endif +#if defined(MBEDTLS_SSL_FALLBACK_SCSV) && defined(MBEDTLS_SSL_CLI_C) + unsigned int fallback : 1; /*!< is this a fallback? */ +#endif +#if defined(MBEDTLS_SSL_SRV_C) + unsigned int cert_req_ca_list : 1; /*!< enable sending CA list in + Certificate Request messages? */ +#endif +#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) + unsigned int ignore_unexpected_cid : 1; /*!< Determines whether DTLS + * record with unexpected CID + * should lead to failure. */ +#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ +#if defined(MBEDTLS_SSL_DTLS_SRTP) + unsigned int dtls_srtp_mki_support : 1; /* support having mki_value + in the use_srtp extension */ +#endif + + /* + * Numerical settings (int then char) + */ + + uint32_t read_timeout; /*!< timeout for mbedtls_ssl_read (ms) */ + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + uint32_t hs_timeout_min; /*!< initial value of the handshake + retransmission timeout (ms) */ + uint32_t hs_timeout_max; /*!< maximum value of the handshake + retransmission timeout (ms) */ +#endif + +#if defined(MBEDTLS_SSL_RENEGOTIATION) + int renego_max_records; /*!< grace period for renegotiation */ + unsigned char renego_period[8]; /*!< value of the record counters + that triggers renegotiation */ +#endif + +#if defined(MBEDTLS_SSL_DTLS_BADMAC_LIMIT) + unsigned int badmac_limit; /*!< limit of records with a bad MAC */ +#endif + +#if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_SSL_CLI_C) + unsigned int dhm_min_bitlen; /*!< min. bit length of the DHM prime */ +#endif /* * Pointers @@ -1174,91 +1263,6 @@ struct mbedtls_ssl_config /*! number of supported profiles */ size_t dtls_srtp_profile_list_len; #endif /* MBEDTLS_SSL_DTLS_SRTP */ - - /* - * Numerical settings (int then char) - */ - - uint32_t read_timeout; /*!< timeout for mbedtls_ssl_read (ms) */ - -#if defined(MBEDTLS_SSL_PROTO_DTLS) - uint32_t hs_timeout_min; /*!< initial value of the handshake - retransmission timeout (ms) */ - uint32_t hs_timeout_max; /*!< maximum value of the handshake - retransmission timeout (ms) */ -#endif - -#if defined(MBEDTLS_SSL_RENEGOTIATION) - int renego_max_records; /*!< grace period for renegotiation */ - unsigned char renego_period[8]; /*!< value of the record counters - that triggers renegotiation */ -#endif - -#if defined(MBEDTLS_SSL_DTLS_BADMAC_LIMIT) - unsigned int badmac_limit; /*!< limit of records with a bad MAC */ -#endif - -#if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_SSL_CLI_C) - unsigned int dhm_min_bitlen; /*!< min. bit length of the DHM prime */ -#endif - - unsigned char max_major_ver; /*!< max. major version used */ - unsigned char max_minor_ver; /*!< max. minor version used */ - unsigned char min_major_ver; /*!< min. major version used */ - unsigned char min_minor_ver; /*!< min. minor version used */ - - /* - * Flags (bitfields) - */ - - unsigned int endpoint : 1; /*!< 0: client, 1: server */ - unsigned int transport : 1; /*!< stream (TLS) or datagram (DTLS) */ - unsigned int authmode : 2; /*!< MBEDTLS_SSL_VERIFY_XXX */ - /* needed even with renego disabled for LEGACY_BREAK_HANDSHAKE */ - unsigned int allow_legacy_renegotiation : 2 ; /*!< MBEDTLS_LEGACY_XXX */ -#if defined(MBEDTLS_ARC4_C) - unsigned int arc4_disabled : 1; /*!< blacklist RC4 ciphersuites? */ -#endif -#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) - unsigned int mfl_code : 3; /*!< desired fragment length */ -#endif -#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) - unsigned int encrypt_then_mac : 1 ; /*!< negotiate encrypt-then-mac? */ -#endif -#if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - unsigned int extended_ms : 1; /*!< negotiate extended master secret? */ -#endif -#if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) - unsigned int anti_replay : 1; /*!< detect and prevent replay? */ -#endif -#if defined(MBEDTLS_SSL_CBC_RECORD_SPLITTING) - unsigned int cbc_record_splitting : 1; /*!< do cbc record splitting */ -#endif -#if defined(MBEDTLS_SSL_RENEGOTIATION) - unsigned int disable_renegotiation : 1; /*!< disable renegotiation? */ -#endif -#if defined(MBEDTLS_SSL_TRUNCATED_HMAC) - unsigned int trunc_hmac : 1; /*!< negotiate truncated hmac? */ -#endif -#if defined(MBEDTLS_SSL_SESSION_TICKETS) - unsigned int session_tickets : 1; /*!< use session tickets? */ -#endif -#if defined(MBEDTLS_SSL_FALLBACK_SCSV) && defined(MBEDTLS_SSL_CLI_C) - unsigned int fallback : 1; /*!< is this a fallback? */ -#endif -#if defined(MBEDTLS_SSL_SRV_C) - unsigned int cert_req_ca_list : 1; /*!< enable sending CA list in - Certificate Request messages? */ -#endif -#if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - unsigned int ignore_unexpected_cid : 1; /*!< Determines whether DTLS - * record with unexpected CID - * should lead to failure. */ -#endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ -#if defined(MBEDTLS_SSL_DTLS_SRTP) - unsigned int dtls_srtp_mki_support : 1; /* support having mki_value - in the use_srtp extension */ -#endif }; struct mbedtls_ssl_context From 7f03d9ecc6d96edcda9898ea8c2dc8cd7b1815cc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 18:31:46 +0100 Subject: [PATCH 07/17] mbedtls_ssl_config: Replace bit-fields by separate bytes This slightly increases the RAM consumption per context, but saves code size on architectures with an instruction for direct byte access (which is most of them). Although this is technically an API break, in practice, a realistic application won't break: it would have had to bypass API functions and rely on the field size (e.g. relying on -1 == 1 in a 1-bit field). Results (arm-none-eabi-gcc 7.3.1, build_arm_none_eabi_gcc_m0plus build): library/ssl_cli.o: 19952 -> 19900 (diff: 52) library/ssl_msg.o: 25810 -> 25798 (diff: 12) library/ssl_srv.o: 22371 -> 22299 (diff: 72) library/ssl_tls.o: 23274 -> 23038 (diff: 236) Results (same architecture, config-suite-b.h + MBEDTLS_ECDH_LEGACY_CONTEXT + MBEDTLS_ECP_RESTARTABLE): library/ssl_cli.o: 2868 -> 2848 (diff: 20) library/ssl_msg.o: 2916 -> 2924 (diff: -8) library/ssl_srv.o: 3204 -> 3184 (diff: 20) library/ssl_tls.o: 5860 -> 5756 (diff: 104) Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index ef6037986..db519fa20 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1032,56 +1032,58 @@ struct mbedtls_ssl_config unsigned char min_minor_ver; /*!< min. minor version used */ /* - * Flags (bitfields) + * Flags (could be bit-fields to save RAM, but separate bytes make + * the code smaller on architectures with an instruction for direct + * byte access). */ - unsigned int endpoint : 1; /*!< 0: client, 1: server */ - unsigned int transport : 1; /*!< stream (TLS) or datagram (DTLS) */ - unsigned int authmode : 2; /*!< MBEDTLS_SSL_VERIFY_XXX */ + uint8_t endpoint /*bool*/; /*!< 0: client, 1: server */ + uint8_t transport /*bool*/; /*!< stream (TLS) or datagram (DTLS) */ + uint8_t authmode /*2 bits*/; /*!< MBEDTLS_SSL_VERIFY_XXX */ /* needed even with renego disabled for LEGACY_BREAK_HANDSHAKE */ - unsigned int allow_legacy_renegotiation : 2 ; /*!< MBEDTLS_LEGACY_XXX */ + uint8_t allow_legacy_renegotiation /*2 bits*/; /*!< MBEDTLS_LEGACY_XXX */ #if defined(MBEDTLS_ARC4_C) - unsigned int arc4_disabled : 1; /*!< blacklist RC4 ciphersuites? */ + uint8_t arc4_disabled /*bool*/; /*!< blacklist RC4 ciphersuites? */ #endif #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) - unsigned int mfl_code : 3; /*!< desired fragment length */ + uint8_t mfl_code /*3 bits*/; /*!< desired fragment length */ #endif #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) - unsigned int encrypt_then_mac : 1 ; /*!< negotiate encrypt-then-mac? */ + uint8_t encrypt_then_mac /*bool*/; /*!< negotiate encrypt-then-mac? */ #endif #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - unsigned int extended_ms : 1; /*!< negotiate extended master secret? */ + uint8_t extended_ms /*bool*/; /*!< negotiate extended master secret? */ #endif #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) - unsigned int anti_replay : 1; /*!< detect and prevent replay? */ + uint8_t anti_replay /*bool*/; /*!< detect and prevent replay? */ #endif #if defined(MBEDTLS_SSL_CBC_RECORD_SPLITTING) - unsigned int cbc_record_splitting : 1; /*!< do cbc record splitting */ + uint8_t cbc_record_splitting /*bool*/; /*!< do cbc record splitting */ #endif #if defined(MBEDTLS_SSL_RENEGOTIATION) - unsigned int disable_renegotiation : 1; /*!< disable renegotiation? */ + uint8_t disable_renegotiation /*bool*/; /*!< disable renegotiation? */ #endif #if defined(MBEDTLS_SSL_TRUNCATED_HMAC) - unsigned int trunc_hmac : 1; /*!< negotiate truncated hmac? */ + uint8_t trunc_hmac /*bool*/; /*!< negotiate truncated hmac? */ #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) - unsigned int session_tickets : 1; /*!< use session tickets? */ + uint8_t session_tickets /*bool*/; /*!< use session tickets? */ #endif #if defined(MBEDTLS_SSL_FALLBACK_SCSV) && defined(MBEDTLS_SSL_CLI_C) - unsigned int fallback : 1; /*!< is this a fallback? */ + uint8_t fallback /*bool*/; /*!< is this a fallback? */ #endif #if defined(MBEDTLS_SSL_SRV_C) - unsigned int cert_req_ca_list : 1; /*!< enable sending CA list in + uint8_t cert_req_ca_list /*bool*/; /*!< enable sending CA list in Certificate Request messages? */ #endif #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) - unsigned int ignore_unexpected_cid : 1; /*!< Determines whether DTLS + uint8_t ignore_unexpected_cid /*bool*/; /*!< Determines whether DTLS * record with unexpected CID * should lead to failure. */ #endif /* MBEDTLS_SSL_DTLS_CONNECTION_ID */ #if defined(MBEDTLS_SSL_DTLS_SRTP) - unsigned int dtls_srtp_mki_support : 1; /* support having mki_value - in the use_srtp extension */ + uint8_t dtls_srtp_mki_support /*bool*/; /*!< support having mki_value + in the use_srtp extension? */ #endif /* From c0656b43f1e1489f247777be336ae57121efe99f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 18:55:20 +0100 Subject: [PATCH 08/17] Note the reordered fields in SSL structures This is technically an API break according to the unwritten rules of API compatibility for Mbed TLS 2.x. However, it is very unlikely to affect any realistic application, with the possible exception of applications that define a global constant of type mbedtls_ssl_config. Signed-off-by: Gilles Peskine --- ChangeLog.d/semi-public-structure-fields.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/semi-public-structure-fields.txt diff --git a/ChangeLog.d/semi-public-structure-fields.txt b/ChangeLog.d/semi-public-structure-fields.txt new file mode 100644 index 000000000..802f8de2b --- /dev/null +++ b/ChangeLog.d/semi-public-structure-fields.txt @@ -0,0 +1,5 @@ +API changes + * Some fields of mbedtls_ssl_session and mbedtls_ssl_config are in a + different order. This only affects applications that define such + structures directly or serialize them. + From 7493c4017a95d82110a6c05fd331fcae3a56c200 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 20:48:59 +0100 Subject: [PATCH 09/17] Fix comment parsing Fix cases like ``` /*short comment*/ /*long comment */ int mbedtls_foo; ``` where the previous code thought that the second line started outside of a comment and ended inside of a comment. I believe that the new code strips comments correctly. It also strips string literals, just in case. Fixes #5191. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index cae722e58..32eaf2164 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -509,18 +509,19 @@ class CodeParser(): previous_line = "" for line_no, line in enumerate(header): - # Skip parsing this line if a block comment ends on it, - # but don't skip if it has just started -- there is a chance - # it ends on the same line. - if re.search(r"/\*", line): - in_block_comment = not in_block_comment - if re.search(r"\*/", line): - in_block_comment = not in_block_comment - continue - + # Terminate current comment? if in_block_comment: - previous_line = "" - continue + line = re.sub(r".*?\*/", r"", line, 1) + in_block_comment = False + # Remove full comments and string literals + line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', + lambda s: '""' if s.group(1) else ' ', + line) + # Start an unfinished comment? + m = re.match(r"/\*", line) + if m: + in_block_comment = True + line = line[:m.end(0)] if exclusion_lines.search(line): previous_line = "" From b3f4dd5c814de9a7171981c90129f9ac3d403808 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Nov 2021 20:56:47 +0100 Subject: [PATCH 10/17] Lift some code out of parse_identifiers Make parse_identifiers less complex. Pylint was complaining that it had too many local variables, and it had a point. * Lift the constants identifier_regex and exclusion_lines to class constants (renamed to uppercase because they're constants). * Lift the per-file loop into a new function parse_identifiers_in_file. No intended behavior change. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 188 +++++++++++++++++++---------------- 1 file changed, 100 insertions(+), 88 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index 32eaf2164..95dae3645 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -457,6 +457,105 @@ class CodeParser(): return enum_consts + IDENTIFIER_REGEX = re.compile( + # Match " something(a" or " *something(a". Functions. + # Assumptions: + # - function definition from return type to one of its arguments is + # all on one line + # - function definition line only contains alphanumeric, asterisk, + # underscore, and open bracket + r".* \**(\w+) *\( *\w|" + # Match "(*something)(". + r".*\( *\* *(\w+) *\) *\(|" + # Match names of named data structures. + r"(?:typedef +)?(?:struct|union|enum) +(\w+)(?: *{)?$|" + # Match names of typedef instances, after closing bracket. + r"}? *(\w+)[;[].*" + ) + # The regex below is indented for clarity. + EXCLUSION_LINES = re.compile( + r"^(" + r"extern +\"C\"|" # pylint: disable=bad-continuation + r"(typedef +)?(struct|union|enum)( *{)?$|" + r"} *;?$|" + r"$|" + r"//|" + r"#" + r")" + ) + + def parse_identifiers_in_file(self, header_file, identifiers): + """ + Parse all lines of a header where a function/enum/struct/union/typedef + identifier is declared, based on some regex and heuristics. Highly + dependent on formatting style. + + Append found matches to the list ``identifiers``. + """ + + with open(header_file, "r", encoding="utf-8") as header: + in_block_comment = False + # The previous line variable is used for concatenating lines + # when identifiers are formatted and spread across multiple + # lines. + previous_line = "" + + for line_no, line in enumerate(header): + # Terminate current comment? + if in_block_comment: + line = re.sub(r".*?\*/", r"", line, 1) + in_block_comment = False + # Remove full comments and string literals + line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', + lambda s: '""' if s.group(1) else ' ', + line) + # Start an unfinished comment? + m = re.match(r"/\*", line) + if m: + in_block_comment = True + line = line[:m.end(0)] + + if self.EXCLUSION_LINES.search(line): + previous_line = "" + continue + + # If the line contains only space-separated alphanumeric + # characters (or underscore, asterisk, or, open bracket), + # and nothing else, high chance it's a declaration that + # continues on the next line + if re.search(r"^([\w\*\(]+\s+)+$", line): + previous_line += line + continue + + # If previous line seemed to start an unfinished declaration + # (as above), concat and treat them as one. + if previous_line: + line = previous_line.strip() + " " + line.strip() + "\n" + previous_line = "" + + # Skip parsing if line has a space in front = heuristic to + # skip function argument lines (highly subject to formatting + # changes) + if line[0] == " ": + continue + + identifier = self.IDENTIFIER_REGEX.search(line) + + if not identifier: + continue + + # Find the group that matched, and append it + for group in identifier.groups(): + if not group: + continue + + identifiers.append(Match( + header_file, + line, + line_no, + identifier.span(), + group)) + def parse_identifiers(self, include, exclude=None): """ Parse all lines of a header where a function/enum/struct/union/typedef @@ -469,100 +568,13 @@ class CodeParser(): Returns a List of Match objects with identifiers. """ - identifier_regex = re.compile( - # Match " something(a" or " *something(a". Functions. - # Assumptions: - # - function definition from return type to one of its arguments is - # all on one line - # - function definition line only contains alphanumeric, asterisk, - # underscore, and open bracket - r".* \**(\w+) *\( *\w|" - # Match "(*something)(". - r".*\( *\* *(\w+) *\) *\(|" - # Match names of named data structures. - r"(?:typedef +)?(?:struct|union|enum) +(\w+)(?: *{)?$|" - # Match names of typedef instances, after closing bracket. - r"}? *(\w+)[;[].*" - ) - # The regex below is indented for clarity. - exclusion_lines = re.compile( - r"^(" - r"extern +\"C\"|" # pylint: disable=bad-continuation - r"(typedef +)?(struct|union|enum)( *{)?$|" - r"} *;?$|" - r"$|" - r"//|" - r"#" - r")" - ) files = self.get_files(include, exclude) self.log.debug("Looking for identifiers in {} files".format(len(files))) identifiers = [] for header_file in files: - with open(header_file, "r", encoding="utf-8") as header: - in_block_comment = False - # The previous line variable is used for concatenating lines - # when identifiers are formatted and spread across multiple - # lines. - previous_line = "" - - for line_no, line in enumerate(header): - # Terminate current comment? - if in_block_comment: - line = re.sub(r".*?\*/", r"", line, 1) - in_block_comment = False - # Remove full comments and string literals - line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', - lambda s: '""' if s.group(1) else ' ', - line) - # Start an unfinished comment? - m = re.match(r"/\*", line) - if m: - in_block_comment = True - line = line[:m.end(0)] - - if exclusion_lines.search(line): - previous_line = "" - continue - - # If the line contains only space-separated alphanumeric - # characters (or underscore, asterisk, or, open bracket), - # and nothing else, high chance it's a declaration that - # continues on the next line - if re.search(r"^([\w\*\(]+\s+)+$", line): - previous_line += line - continue - - # If previous line seemed to start an unfinished declaration - # (as above), concat and treat them as one. - if previous_line: - line = previous_line.strip() + " " + line.strip() + "\n" - previous_line = "" - - # Skip parsing if line has a space in front = heuristic to - # skip function argument lines (highly subject to formatting - # changes) - if line[0] == " ": - continue - - identifier = identifier_regex.search(line) - - if not identifier: - continue - - # Find the group that matched, and append it - for group in identifier.groups(): - if not group: - continue - - identifiers.append(Match( - header_file, - line, - line_no, - identifier.span(), - group)) + self.parse_identifiers_in_file(header_file, identifiers) return identifiers From e833998b58f1af8c134c036e84ddf30a65f11eac Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:00:13 +0100 Subject: [PATCH 11/17] Update comment Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index db519fa20..209dbf605 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1087,7 +1087,7 @@ struct mbedtls_ssl_config #endif /* - * Numerical settings (int then char) + * Numerical settings (int or larger) */ uint32_t read_timeout; /*!< timeout for mbedtls_ssl_read (ms) */ From c8fc67f3416cfd4f10d2d8f6685748556203ad3c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:23:18 +0100 Subject: [PATCH 12/17] Simplify some regex definitions Use '|'.join([comma-separated list]) rather than r'...|' r'...|'. This way there's less risk of forgetting a '|'. Pylint will yell if we forget a comma between list elements. Use match rather than search + mandatory start anchor for EXCLUSION_LINES. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index 95dae3645..5c1cb81f2 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -457,32 +457,30 @@ class CodeParser(): return enum_consts - IDENTIFIER_REGEX = re.compile( + IDENTIFIER_REGEX = re.compile('|'.join([ # Match " something(a" or " *something(a". Functions. # Assumptions: # - function definition from return type to one of its arguments is # all on one line # - function definition line only contains alphanumeric, asterisk, # underscore, and open bracket - r".* \**(\w+) *\( *\w|" + r".* \**(\w+) *\( *\w", # Match "(*something)(". - r".*\( *\* *(\w+) *\) *\(|" + r".*\( *\* *(\w+) *\) *\(", # Match names of named data structures. - r"(?:typedef +)?(?:struct|union|enum) +(\w+)(?: *{)?$|" + r"(?:typedef +)?(?:struct|union|enum) +(\w+)(?: *{)?$", # Match names of typedef instances, after closing bracket. - r"}? *(\w+)[;[].*" - ) + r"}? *(\w+)[;[].*", + ])) # The regex below is indented for clarity. - EXCLUSION_LINES = re.compile( - r"^(" - r"extern +\"C\"|" # pylint: disable=bad-continuation - r"(typedef +)?(struct|union|enum)( *{)?$|" - r"} *;?$|" - r"$|" - r"//|" - r"#" - r")" - ) + EXCLUSION_LINES = re.compile("|".join([ + r"extern +\"C\"", + r"(typedef +)?(struct|union|enum)( *{)?$", + r"} *;?$", + r"$", + r"//", + r"#", + ])) def parse_identifiers_in_file(self, header_file, identifiers): """ @@ -515,7 +513,7 @@ class CodeParser(): in_block_comment = True line = line[:m.end(0)] - if self.EXCLUSION_LINES.search(line): + if self.EXCLUSION_LINES.match(line): previous_line = "" continue From df30665a161e26c5c12222ea4f80e78eb2b0e778 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:32:31 +0100 Subject: [PATCH 13/17] Move comment and string literal processing to a new function No intended behavior change. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 45 +++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index 5c1cb81f2..acf60db46 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -457,6 +457,36 @@ class CodeParser(): return enum_consts + def strip_comments_and_literals(self, line, in_block_comment): + """Strip comments and string literals from line. + + Continuation lines are not supported. + + If in_block_comment is true, assume that the line starts inside a + block comment. + + Return updated values of (line, in_block_comment) where: + * Comments in line have been replaced by a space (or nothing at the + start or end of the line). + * String contents have been removed. + * in_block_comment indicates whether the line ends inside a block + comment that continues on the next line. + """ + # Terminate current comment? + if in_block_comment: + line = re.sub(r".*?\*/", r"", line, 1) + in_block_comment = False + # Remove full comments and string literals + line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', + lambda s: '""' if s.group(1) else ' ', + line) + # Start an unfinished comment? + m = re.match(r"/\*", line) + if m: + in_block_comment = True + line = line[:m.end(0)] + return line, in_block_comment + IDENTIFIER_REGEX = re.compile('|'.join([ # Match " something(a" or " *something(a". Functions. # Assumptions: @@ -499,19 +529,8 @@ class CodeParser(): previous_line = "" for line_no, line in enumerate(header): - # Terminate current comment? - if in_block_comment: - line = re.sub(r".*?\*/", r"", line, 1) - in_block_comment = False - # Remove full comments and string literals - line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', - lambda s: '""' if s.group(1) else ' ', - line) - # Start an unfinished comment? - m = re.match(r"/\*", line) - if m: - in_block_comment = True - line = line[:m.end(0)] + line, in_block_comment = \ + self.strip_comments_and_literals(line, in_block_comment) if self.EXCLUSION_LINES.match(line): previous_line = "" From 4f04d619b5eaacced2878d57cf335cd5275d88f8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:39:56 +0100 Subject: [PATCH 14/17] Fix terminology in comment In computing, brackets are []. () are called parentheses. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index acf60db46..e1341107a 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -537,7 +537,7 @@ class CodeParser(): continue # If the line contains only space-separated alphanumeric - # characters (or underscore, asterisk, or, open bracket), + # characters (or underscore, asterisk, or open parenthesis), # and nothing else, high chance it's a declaration that # continues on the next line if re.search(r"^([\w\*\(]+\s+)+$", line): From 44801627d2a4dd5e7e86f6c164a65f950c667d9d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:43:35 +0100 Subject: [PATCH 15/17] Improve comment and string stripping Make that part of the code more readable. Add support for // line comments. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index e1341107a..588f72b6d 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -457,6 +457,12 @@ class CodeParser(): return enum_consts + IGNORED_CHUNK_REGEX = re.compile('|'.join([ + r'/\*.*?\*/', # block comment entirely on one line + r'//.*', # line comment + r'(?P")(?:[^\\\"]|\\.)*"', # string literal + ])) + def strip_comments_and_literals(self, line, in_block_comment): """Strip comments and string literals from line. @@ -476,15 +482,21 @@ class CodeParser(): if in_block_comment: line = re.sub(r".*?\*/", r"", line, 1) in_block_comment = False - # Remove full comments and string literals - line = re.sub(r'/\*.*?\*/|(")(?:[^\\\"]|\\.)*"', - lambda s: '""' if s.group(1) else ' ', + + # Remove full comments and string literals. + # Do it all together to handle cases like "/*" correctly. + # Note that continuation lines are not supported. + line = re.sub(self.IGNORED_CHUNK_REGEX, + lambda s: '""' if s.group('string') else ' ', line) + # Start an unfinished comment? + # (If `/*` was part of a complete comment, it's already been removed.) m = re.match(r"/\*", line) if m: in_block_comment = True line = line[:m.end(0)] + return line, in_block_comment IDENTIFIER_REGEX = re.compile('|'.join([ From 23b4096ecfb909ba53d7a81cd635aa729d9760bd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Nov 2021 20:45:39 +0100 Subject: [PATCH 16/17] Fix several bugs with multiline comments Empty the current line if it's entirely inside a comment. Don't incorrectly end a block comment at the second line if it doesn't contain `*/`. Recognize `/*` to start a multiline comment even if it isn't at the start of the line. When stripping off comments, consistently strip off `/*` and `*/`. Signed-off-by: Gilles Peskine --- tests/scripts/check_names.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py index 588f72b6d..f4af4c4dd 100755 --- a/tests/scripts/check_names.py +++ b/tests/scripts/check_names.py @@ -478,10 +478,15 @@ class CodeParser(): * in_block_comment indicates whether the line ends inside a block comment that continues on the next line. """ - # Terminate current comment? + + # Terminate current multiline comment? if in_block_comment: - line = re.sub(r".*?\*/", r"", line, 1) - in_block_comment = False + m = re.search(r"\*/", line) + if m: + in_block_comment = False + line = line[m.end(0):] + else: + return '', True # Remove full comments and string literals. # Do it all together to handle cases like "/*" correctly. @@ -492,10 +497,10 @@ class CodeParser(): # Start an unfinished comment? # (If `/*` was part of a complete comment, it's already been removed.) - m = re.match(r"/\*", line) + m = re.search(r"/\*", line) if m: in_block_comment = True - line = line[:m.end(0)] + line = line[:m.start(0)] return line, in_block_comment From 9501bd7d6e3e796b79ba154f56680b13ef8bfe29 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 25 Nov 2021 20:30:47 +0100 Subject: [PATCH 17/17] Make PSA headers more self-contained Several files among include/psa/crypto_*.h are not meant to be included directly, and are not guaranteed to be valid if included directly. This makes it harder to perform some static analyses. So make these files more self-contained so that at least, if included on their own, there is no missing macro or type definition (excluding the deliberate use of forward declarations of structs and unions). Signed-off-by: Gilles Peskine --- include/psa/crypto_driver_common.h | 3 +++ include/psa/crypto_extra.h | 1 + include/psa/crypto_struct.h | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/include/psa/crypto_driver_common.h b/include/psa/crypto_driver_common.h index 1b6f32256..26363c6b2 100644 --- a/include/psa/crypto_driver_common.h +++ b/include/psa/crypto_driver_common.h @@ -42,6 +42,9 @@ * of these types. */ #include "crypto_types.h" #include "crypto_values.h" +/* Include size definitions which are used to size some arrays in operation + * structures. */ +#include /** For encrypt-decrypt functions, whether the operation is an encryption * or a decryption. */ diff --git a/include/psa/crypto_extra.h b/include/psa/crypto_extra.h index 1310bb576..3ee0482cb 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -30,6 +30,7 @@ #include "mbedtls/platform_util.h" +#include "crypto_types.h" #include "crypto_compat.h" #ifdef __cplusplus diff --git a/include/psa/crypto_struct.h b/include/psa/crypto_struct.h index f08fdc8bd..23a02a5d8 100644 --- a/include/psa/crypto_struct.h +++ b/include/psa/crypto_struct.h @@ -184,7 +184,7 @@ typedef struct unsigned int info_set : 1; uint8_t output_block[PSA_HASH_MAX_SIZE]; uint8_t prk[PSA_HASH_MAX_SIZE]; - psa_mac_operation_t hmac; + struct psa_mac_operation_s hmac; } psa_hkdf_key_derivation_t; #endif /* MBEDTLS_PSA_BUILTIN_ALG_HKDF */