From 0483e3d65272a20ca13baf4e1f6e93b154d87892 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Mon, 4 Oct 2021 11:13:22 +0200 Subject: [PATCH 01/48] Add key_opaque option to ssl_server2.c + test Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_server2.c | 32 +++++++++++++++++++++++++++++++- tests/ssl-opt.sh | 15 +++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index e8e4ed8aeaaf..abc9b5f5d4e5 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -80,6 +80,7 @@ int main( void ) #define DFL_CA_PATH "" #define DFL_CRT_FILE "" #define DFL_KEY_FILE "" +#define DFL_KEY_OPAQUE 0 #define DFL_KEY_PWD "" #define DFL_CRT_FILE2 "" #define DFL_KEY_FILE2 "" @@ -200,6 +201,13 @@ int main( void ) #else #define USAGE_IO "" #endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_X509_CRT_PARSE_C) +#define USAGE_KEY_OPAQUE \ + " key_opaque=%%d Handle your private key as if it were opaque\n" \ + " default: 0 (disabled)\n" +#else +#define USAGE_KEY_OPAQUE "" +#endif #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) #define USAGE_SSL_ASYNC \ @@ -483,6 +491,7 @@ int main( void ) " cert_req_ca_list=%%d default: 1 (send ca list)\n" \ " options: 1 (send ca list), 0 (don't send)\n" \ USAGE_IO \ + USAGE_KEY_OPAQUE \ "\n" \ USAGE_PSK \ USAGE_CA_CALLBACK \ @@ -567,6 +576,7 @@ struct options const char *ca_path; /* the path with the CA certificate(s) reside */ const char *crt_file; /* the file with the server certificate */ const char *key_file; /* the file with the server key */ + int key_opaque; /* handle private key as if it were opaque */ const char *key_pwd; /* the password for the server key */ const char *crt_file2; /* the file with the 2nd server certificate */ const char *key_file2; /* the file with the 2nd server key */ @@ -1315,6 +1325,9 @@ int main( int argc, char *argv[] ) mbedtls_pk_context pkey; mbedtls_x509_crt srvcert2; mbedtls_pk_context pkey2; +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_key_id_t key_slot = 0; /* invalid key slot */ +#endif int key_cert_init = 0, key_cert_init2 = 0; #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) ssl_async_key_context_t ssl_async_keys; @@ -1491,6 +1504,7 @@ int main( int argc, char *argv[] ) opt.ca_path = DFL_CA_PATH; opt.crt_file = DFL_CRT_FILE; opt.key_file = DFL_KEY_FILE; + opt.key_opaque = DFL_KEY_OPAQUE; opt.key_pwd = DFL_KEY_PWD; opt.crt_file2 = DFL_CRT_FILE2; opt.key_file2 = DFL_KEY_FILE2; @@ -1622,6 +1636,10 @@ int main( int argc, char *argv[] ) opt.key_file = q; else if( strcmp( p, "key_pwd" ) == 0 ) opt.key_pwd = q; +#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_X509_CRT_PARSE_C) + else if( strcmp( p, "key_opaque" ) == 0 ) + opt.key_opaque = atoi( q ); +#endif else if( strcmp( p, "crt_file2" ) == 0 ) opt.crt_file2 = q; else if( strcmp( p, "key_file2" ) == 0 ) @@ -2473,11 +2491,23 @@ int main( int argc, char *argv[] ) (unsigned int) -ret ); goto exit; } +#if defined(MBEDTLS_USE_PSA_CRYPTO) + if( opt.key_opaque != 0 ) + { + if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey2, &key_slot, + PSA_ALG_SHA_256 ) ) != 0 ) + { + mbedtls_printf( " failed\n ! " + "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); + goto exit; + } + } +#endif /* MBEDTLS_USE_PSA_CRYPTO */ key_cert_init2 = 2; #endif /* MBEDTLS_ECDSA_C */ } - mbedtls_printf( " ok\n" ); + mbedtls_printf( " ok (key type: %s)\n", mbedtls_pk_get_name( &pkey2 ) ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 66c648573bfc..d6fef1301332 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1444,6 +1444,21 @@ run_test "Opaque key for client authentication" \ -S "error" \ -C "error" +# Test using an opaque private key for server authentication +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +requires_config_enabled MBEDTLS_X509_CRT_PARSE_C +requires_config_enabled MBEDTLS_ECDSA_C +requires_config_enabled MBEDTLS_SHA256_C +run_test "Opaque key for server authentication" \ + "$P_SRV auth_mode=required key_opaque=1" \ + "$P_CLI crt_file=data_files/server5.crt \ + key_file=data_files/server5.key" \ + 0 \ + -c "Verifying peer X.509 certificate... ok" \ + -s "key type: Opaque" \ + -S "error" \ + -C "error" + # Test ciphersuites which we expect to be fully supported by PSA Crypto # and check that we don't fall back to Mbed TLS' internal crypto primitives. run_test_psa TLS-ECDHE-ECDSA-WITH-AES-128-CCM From f28261fc14da5f52b460c903e027aff187f9cf5f Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Thu, 30 Sep 2021 16:39:07 +0200 Subject: [PATCH 02/48] Remove output buffer limitation for PSA with GCM. The requirement of minimum 15 bytes for output buffer in psa_aead_finish() and psa_aead_verify() does not apply to the built-in implementation of the GCM. Alternative implementations are expected to verify the length of the provided output buffers and to return the MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL in case the buffer length is too small. Signed-off-by: Mateusz Starzyk --- ChangeLog.d/psa_gcm_buffer_limitation.txt | 11 +++++++++++ include/mbedtls/gcm.h | 2 ++ library/psa_crypto.c | 2 ++ library/psa_crypto_aead.c | 3 --- tests/suites/test_suite_psa_crypto.data | 2 +- 5 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 ChangeLog.d/psa_gcm_buffer_limitation.txt diff --git a/ChangeLog.d/psa_gcm_buffer_limitation.txt b/ChangeLog.d/psa_gcm_buffer_limitation.txt new file mode 100644 index 000000000000..7259e50684dd --- /dev/null +++ b/ChangeLog.d/psa_gcm_buffer_limitation.txt @@ -0,0 +1,11 @@ +Bugfix + * Remove PSA'a AEAD finish/verify output buffer limitation for GCM. + The requirement of minimum 15 bytes for output buffer in + psa_aead_finish() and psa_aead_verify() does not apply to the built-in + implementation of GCM. + +API changes + * New error code for GCM: MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL. + Alternative GCM implementations are expected to verify + the length of the provided output buffers and to return the + MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL in case the buffer length is too small. diff --git a/include/mbedtls/gcm.h b/include/mbedtls/gcm.h index 9d9155fc5b0a..a4de9191d864 100644 --- a/include/mbedtls/gcm.h +++ b/include/mbedtls/gcm.h @@ -45,6 +45,8 @@ #define MBEDTLS_ERR_GCM_AUTH_FAILED -0x0012 /** Bad input parameters to function. */ #define MBEDTLS_ERR_GCM_BAD_INPUT -0x0014 +/** An output buffer is too small. */ +#define MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL -0x0018 #ifdef __cplusplus extern "C" { diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ece64b100d61..5978b6ac5b65 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -201,6 +201,8 @@ psa_status_t mbedtls_to_psa_error( int ret ) case MBEDTLS_ERR_GCM_AUTH_FAILED: return( PSA_ERROR_INVALID_SIGNATURE ); + case MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL: + return( PSA_ERROR_BUFFER_TOO_SMALL ); case MBEDTLS_ERR_GCM_BAD_INPUT: return( PSA_ERROR_INVALID_ARGUMENT ); diff --git a/library/psa_crypto_aead.c b/library/psa_crypto_aead.c index a72865c04c16..673cdf3448b4 100644 --- a/library/psa_crypto_aead.c +++ b/library/psa_crypto_aead.c @@ -567,9 +567,6 @@ psa_status_t mbedtls_psa_aead_finish( #if defined(MBEDTLS_PSA_BUILTIN_ALG_GCM) if( operation->alg == PSA_ALG_GCM ) { - if( ciphertext_size < 15 ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - status = mbedtls_to_psa_error( mbedtls_gcm_finish( &operation->ctx.gcm, ciphertext, ciphertext_size, ciphertext_length, diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 063629e59934..3a3e67821ad6 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -3348,7 +3348,7 @@ aead_multipart_update_buffer_test:PSA_KEY_TYPE_CHACHA20:"808182838485868788898a8 PSA AEAD finish buffer test: AES - GCM, BUF = 8, TAG = 16 depends_on:PSA_WANT_ALG_GCM:PSA_WANT_KEY_TYPE_AES -aead_multipart_finish_buffer_test:PSA_KEY_TYPE_AES:"fbc0b4c56a714c83217b2d1bcadd2ed2e9efb0dcac6cc19f":PSA_ALG_GCM:8:16:"5f4b43e811da9c470d6a9b01":"":"d2ae38c4375954835d75b8e4c2f9bbb4":PSA_ERROR_BUFFER_TOO_SMALL +aead_multipart_finish_buffer_test:PSA_KEY_TYPE_AES:"fbc0b4c56a714c83217b2d1bcadd2ed2e9efb0dcac6cc19f":PSA_ALG_GCM:8:16:"5f4b43e811da9c470d6a9b01":"":"d2ae38c4375954835d75b8e4c2f9bbb4":PSA_SUCCESS PSA AEAD finish buffer test: AES - GCM, BUF = 15, TAG = 20 depends_on:PSA_WANT_ALG_GCM:PSA_WANT_KEY_TYPE_AES From c48f43b44de534af4a89aae26cefebdba0a14aca Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Mon, 4 Oct 2021 13:46:38 +0200 Subject: [PATCH 03/48] Fix PSA AEAD GCM's update output buffer length verification. Move GCM's update output buffer length verification from PSA AEAD to the built-in implementation of the GCM. Signed-off-by: Mateusz Starzyk --- ChangeLog.d/psa_gcm_buffer_limitation.txt | 5 +++++ library/gcm.c | 2 +- library/psa_crypto_aead.c | 3 --- tests/suites/test_suite_gcm.aes128_de.data | 4 ++++ tests/suites/test_suite_gcm.aes128_en.data | 3 +++ tests/suites/test_suite_gcm.function | 24 ++++++++++++++++++++++ 6 files changed, 37 insertions(+), 4 deletions(-) diff --git a/ChangeLog.d/psa_gcm_buffer_limitation.txt b/ChangeLog.d/psa_gcm_buffer_limitation.txt index 7259e50684dd..0c07e2415405 100644 --- a/ChangeLog.d/psa_gcm_buffer_limitation.txt +++ b/ChangeLog.d/psa_gcm_buffer_limitation.txt @@ -3,6 +3,11 @@ Bugfix The requirement of minimum 15 bytes for output buffer in psa_aead_finish() and psa_aead_verify() does not apply to the built-in implementation of GCM. + * Move GCM's update output buffer length verification from PSA AEAD to + the built-in implementation of the GCM. + The requirement for output buffer size to be equal or greater then + input buffer size is valid only for the built-in implementation of GCM. + Alternative GCM implementations can process whole blocks only. API changes * New error code for GCM: MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL. diff --git a/library/gcm.c b/library/gcm.c index 910646b281cb..6d625642ebde 100644 --- a/library/gcm.c +++ b/library/gcm.c @@ -431,7 +431,7 @@ int mbedtls_gcm_update( mbedtls_gcm_context *ctx, unsigned char ectr[16]; if( output_size < input_length ) - return( MBEDTLS_ERR_GCM_BAD_INPUT ); + return( MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL ); GCM_VALIDATE_RET( output_length != NULL ); *output_length = input_length; diff --git a/library/psa_crypto_aead.c b/library/psa_crypto_aead.c index 673cdf3448b4..c7f7352fbdcb 100644 --- a/library/psa_crypto_aead.c +++ b/library/psa_crypto_aead.c @@ -510,9 +510,6 @@ psa_status_t mbedtls_psa_aead_update( #if defined(MBEDTLS_PSA_BUILTIN_ALG_GCM) if( operation->alg == PSA_ALG_GCM ) { - if( output_size < input_length ) - return( PSA_ERROR_BUFFER_TOO_SMALL ); - status = mbedtls_to_psa_error( mbedtls_gcm_update( &operation->ctx.gcm, input, input_length, diff --git a/tests/suites/test_suite_gcm.aes128_de.data b/tests/suites/test_suite_gcm.aes128_de.data index 3df31e56bfdf..ede6f243c1cd 100644 --- a/tests/suites/test_suite_gcm.aes128_de.data +++ b/tests/suites/test_suite_gcm.aes128_de.data @@ -726,6 +726,10 @@ AES-GCM Bad IV (AES-128,128,0,0,32) #0 depends_on:MBEDTLS_AES_C gcm_bad_parameters:MBEDTLS_CIPHER_ID_AES:MBEDTLS_GCM_DECRYPT:"d0194b6ee68f0ed8adc4b22ed15dbf14":"":"":"":32:MBEDTLS_ERR_GCM_BAD_INPUT +AES-GCM, output buffer too small, NIST Validation (AES-128,128,1024,0,128) #0 +depends_on:MBEDTLS_AES_C +gcm_update_output_buffer_too_small:MBEDTLS_CIPHER_ID_AES:MBEDTLS_GCM_DECRYPT:"0dd358bc3f992f26e81e3a2f3aa2d517":"87cc4fd75788c9d5cc83bae5d764dd249d178ab23224049795d4288b5ed9ea3f317068a39a7574b300c8544226e87b08e008fbe241d094545c211d56ac44437d41491a438272738968c8d371aa7787b5f606c8549a9d868d8a71380e9657d3c0337979feb01de5991fc1470dfc59eb02511efbbff3fcb479a862ba3844a25aaa":"d8c750bb443ee1a169dfe97cfe4d855b" + AES-GCM Selftest depends_on:MBEDTLS_AES_C gcm_selftest: diff --git a/tests/suites/test_suite_gcm.aes128_en.data b/tests/suites/test_suite_gcm.aes128_en.data index d60c458bcd23..273642cbd7e7 100644 --- a/tests/suites/test_suite_gcm.aes128_en.data +++ b/tests/suites/test_suite_gcm.aes128_en.data @@ -726,6 +726,9 @@ AES-GCM Bad IV (AES-128,128,0,0,32) #0 depends_on:MBEDTLS_AES_C gcm_bad_parameters:MBEDTLS_CIPHER_ID_AES:MBEDTLS_GCM_ENCRYPT:"d0194b6ee68f0ed8adc4b22ed15dbf14":"":"":"":32:MBEDTLS_ERR_GCM_BAD_INPUT +AES-GCM, output buffer too small, NIST Validation (AES-128,128,1024,0,128) #0 +gcm_update_output_buffer_too_small:MBEDTLS_CIPHER_ID_AES:MBEDTLS_GCM_ENCRYPT:"ce0f8cfe9d64c4f4c045d11b97c2d918":"dfff250d380f363880963b42d6913c1ba11e8edf7c4ab8b76d79ccbaac628f548ee542f48728a9a2620a0d69339c8291e8d398440d740e310908cdee7c273cc91275ce7271ba12f69237998b07b789b3993aaac8dc4ec1914432a30f5172f79ea0539bd1f70b36d437e5170bc63039a5280816c05e1e41760b58e35696cebd55":"ad4c3627a494fc628316dc03faf81db8" + AES-GCM Selftest depends_on:MBEDTLS_AES_C gcm_selftest: diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index c530e6b4292d..816ebc4ec520 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -431,6 +431,30 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void gcm_update_output_buffer_too_small( int cipher_id, int mode, + data_t * key_str, const data_t *input, + const data_t *iv ) +{ + mbedtls_gcm_context ctx; + uint8_t *output = NULL; + size_t olen; + size_t output_len = input->len - 1; + + mbedtls_gcm_init( &ctx ); + TEST_EQUAL( mbedtls_gcm_setkey( &ctx, cipher_id, key_str->x, key_str->len * 8 ), 0 ); + TEST_EQUAL( 0, mbedtls_gcm_starts( &ctx, mode, iv->x, iv->len ) ); + + ASSERT_ALLOC( output, output_len ); + olen = 0xdeadbeef; + TEST_EQUAL( MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL, mbedtls_gcm_update( &ctx, input->x, input->len, output, output_len, &olen ) ); + +exit: + mbedtls_free( output ); + mbedtls_gcm_free( &ctx ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void gcm_selftest( ) { From 575f23c3d5bad874a03b3def02fa68a5458cdc9b Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Wed, 6 Oct 2021 11:31:49 +0200 Subject: [PATCH 04/48] add client/server opaque test Signed-off-by: Przemyslaw Stekiel --- tests/ssl-opt.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d6fef1301332..7d0b31381f56 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1459,6 +1459,23 @@ run_test "Opaque key for server authentication" \ -S "error" \ -C "error" +# Test using an opaque private key for client/server authentication +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +requires_config_enabled MBEDTLS_X509_CRT_PARSE_C +requires_config_enabled MBEDTLS_ECDSA_C +requires_config_enabled MBEDTLS_SHA256_C +run_test "Opaque key for client/server authentication" \ + "$P_SRV auth_mode=required key_opaque=1" \ + "$P_CLI key_opaque=1 crt_file=data_files/server5.crt \ + key_file=data_files/server5.key" \ + 0 \ + -c "key type: Opaque" \ + -c "Verifying peer X.509 certificate... ok" \ + -s "key type: Opaque" \ + -s "Verifying peer X.509 certificate... ok" \ + -S "error" \ + -C "error" + # Test ciphersuites which we expect to be fully supported by PSA Crypto # and check that we don't fall back to Mbed TLS' internal crypto primitives. run_test_psa TLS-ECDHE-ECDSA-WITH-AES-128-CCM From db0ed7c57999ddc2f50c841a6df94ef242599876 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Thu, 7 Oct 2021 15:11:32 +0200 Subject: [PATCH 05/48] ssl_server2.c: fix build err (key_slot - unused variable) Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_server2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index abc9b5f5d4e5..68e92b7121dd 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -1325,7 +1325,7 @@ int main( int argc, char *argv[] ) mbedtls_pk_context pkey; mbedtls_x509_crt srvcert2; mbedtls_pk_context pkey2; -#if defined(MBEDTLS_USE_PSA_CRYPTO) +#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_USE_PSA_CRYPTO) psa_key_id_t key_slot = 0; /* invalid key slot */ #endif int key_cert_init = 0, key_cert_init2 = 0; From 77804132ba8b43360fc01012cd70bf68e34972ba Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Mon, 11 Oct 2021 16:38:17 +0200 Subject: [PATCH 06/48] Use PSA_HASH_LENGTH instead hardcoded integer values --- tests/suites/test_suite_psa_crypto.data | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 063629e59934..6090cdf0066b 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -4422,19 +4422,19 @@ derive_output:PSA_ALG_TLS12_PSK_TO_MS(PSA_ALG_SHA_384):PSA_KEY_DERIVATION_INPUT_ PSA key derivation: HKDF SHA-256, request maximum capacity depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256 -derive_output:PSA_ALG_HKDF(PSA_ALG_SHA_256):PSA_KEY_DERIVATION_INPUT_SALT:"000102030405060708090a0b0c":PSA_KEY_DERIVATION_INPUT_SECRET:"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":PSA_KEY_DERIVATION_INPUT_INFO:"f0f1f2f3f4f5f6f7f8f9":255 * 32:"3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf34007208d5b887185865":"" +derive_output:PSA_ALG_HKDF(PSA_ALG_SHA_256):PSA_KEY_DERIVATION_INPUT_SALT:"000102030405060708090a0b0c":PSA_KEY_DERIVATION_INPUT_SECRET:"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":PSA_KEY_DERIVATION_INPUT_INFO:"f0f1f2f3f4f5f6f7f8f9":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256):"3cb25f25faacd57a90434f64d0362f2a2d2d0a90cf1a5a4c5db02d56ecc4c5bf34007208d5b887185865":"" PSA key derivation: HKDF SHA-1, request maximum capacity depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_1 -derive_output:PSA_ALG_HKDF(PSA_ALG_SHA_1):PSA_KEY_DERIVATION_INPUT_SALT:"":PSA_KEY_DERIVATION_INPUT_SECRET:"0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c":PSA_KEY_DERIVATION_INPUT_INFO:"":255 * 20:"2c91117204d745f3500d636a62f64f0ab3bae548aa53d423b0d1f27ebba6f5e5673a081d70cce7acfc48":"" +derive_output:PSA_ALG_HKDF(PSA_ALG_SHA_1):PSA_KEY_DERIVATION_INPUT_SALT:"":PSA_KEY_DERIVATION_INPUT_SECRET:"0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c0c":PSA_KEY_DERIVATION_INPUT_INFO:"":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_1):"2c91117204d745f3500d636a62f64f0ab3bae548aa53d423b0d1f27ebba6f5e5673a081d70cce7acfc48":"" PSA key derivation: HKDF SHA-256, request too much capacity depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256 -derive_set_capacity:PSA_ALG_HKDF(PSA_ALG_SHA_256):255 * 32 + 1:PSA_ERROR_INVALID_ARGUMENT +derive_set_capacity:PSA_ALG_HKDF(PSA_ALG_SHA_256):255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256) + 1:PSA_ERROR_INVALID_ARGUMENT PSA key derivation: HKDF SHA-1, request too much capacity depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_1 -derive_set_capacity:PSA_ALG_HKDF(PSA_ALG_SHA_1):255 * 20 + 1:PSA_ERROR_INVALID_ARGUMENT +derive_set_capacity:PSA_ALG_HKDF(PSA_ALG_SHA_1):255 * PSA_HASH_LENGTH(PSA_ALG_SHA_1) + 1:PSA_ERROR_INVALID_ARGUMENT PSA key derivation: over capacity 42: output 42+1 depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256 @@ -4454,19 +4454,19 @@ derive_output:PSA_ALG_HKDF(PSA_ALG_SHA_256):PSA_KEY_DERIVATION_INPUT_SALT:"00010 PSA key derivation: HKDF SHA-256, read maximum capacity minus 1 depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256 -derive_full:PSA_ALG_HKDF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * 32 - 1 +derive_full:PSA_ALG_HKDF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256) - 1 PSA key derivation: HKDF SHA-256, read maximum capacity depends_on:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256 -derive_full:PSA_ALG_HKDF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * 32 +derive_full:PSA_ALG_HKDF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256) PSA key derivation: TLS 1.2 PRF SHA-256, read maximum capacity minus 1 depends_on:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_TLS12_PRF -derive_full:PSA_ALG_TLS12_PRF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * 32 - 1 +derive_full:PSA_ALG_TLS12_PRF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256) - 1 PSA key derivation: TLS 1.2 PRF SHA-256, read maximum capacity depends_on:PSA_WANT_ALG_SHA_256:PSA_WANT_ALG_TLS12_PRF -derive_full:PSA_ALG_TLS12_PRF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * 32 +derive_full:PSA_ALG_TLS12_PRF(PSA_ALG_SHA_256):"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":"000102030405060708090a0b0c":"f0f1f2f3f4f5f6f7f8f9":255 * PSA_HASH_LENGTH(PSA_ALG_SHA_256) PSA key derivation: HKDF SHA-256, exercise AES128-CTR depends_on:PSA_WANT_ALG_CTR:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256:PSA_WANT_KEY_TYPE_AES From 236bf98cfdcabc31ba7e76d40a0dd28ce43b384f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 19 Oct 2021 16:25:10 +0200 Subject: [PATCH 07/48] Move some code of run_test into auxiliary functions No behavior change. Signed-off-by: Gilles Peskine --- tests/ssl-opt.sh | 237 +++++++++++++++++++++++++++-------------------- 1 file changed, 134 insertions(+), 103 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e90a35226bea..1d73d3af05ad 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -795,68 +795,12 @@ skip_handshake_stage_check() { SKIP_HANDSHAKE_CHECK="YES" } -# Usage: run_test name [-p proxy_cmd] srv_cmd cli_cmd cli_exit [option [...]] -# Options: -s pattern pattern that must be present in server output -# -c pattern pattern that must be present in client output -# -u pattern lines after pattern must be unique in client output -# -f call shell function on client output -# -S pattern pattern that must be absent in server output -# -C pattern pattern that must be absent in client output -# -U pattern lines after pattern must be unique in server output -# -F call shell function on server output -# -g call shell function on server and client output -run_test() { - NAME="$1" - shift 1 - - if is_excluded "$NAME"; then - SKIP_NEXT="NO" - # There was no request to run the test, so don't record its outcome. - return - fi - - print_name "$NAME" - - # Do we only run numbered tests? - if [ -n "$RUN_TEST_NUMBER" ]; then - case ",$RUN_TEST_NUMBER," in - *",$TESTS,"*) :;; - *) SKIP_NEXT="YES";; - esac - fi - - # does this test use a proxy? - if [ "X$1" = "X-p" ]; then - PXY_CMD="$2" - shift 2 - else - PXY_CMD="" - fi - - # get commands and client output - SRV_CMD="$1" - CLI_CMD="$2" - CLI_EXPECT="$3" - shift 3 - - # Check if test uses files - case "$SRV_CMD $CLI_CMD" in - *data_files/*) - requires_config_enabled MBEDTLS_FS_IO;; - esac - - # If the client or serve requires a ciphersuite, check that it's enabled. - maybe_requires_ciphersuite_enabled "$SRV_CMD" "$@" - maybe_requires_ciphersuite_enabled "$CLI_CMD" "$@" - - # should we skip? - if [ "X$SKIP_NEXT" = "XYES" ]; then - SKIP_NEXT="NO" - record_outcome "SKIP" - SKIPS=$(( $SKIPS + 1 )) - return - fi - +# Analyze the commands that will be used in a test. +# +# Analyze and possibly instrument $PXY_CMD, $CLI_CMD, $SRV_CMD to pass +# extra arguments or go through wrappers. +# Set $DTLS (0=TLS, 1=DTLS). +analyze_test_commands() { # update DTLS variable detect_dtls "$SRV_CMD" @@ -910,48 +854,21 @@ run_test() { CLI_CMD="valgrind --leak-check=full $CLI_CMD" fi fi +} - TIMES_LEFT=2 - while [ $TIMES_LEFT -gt 0 ]; do - TIMES_LEFT=$(( $TIMES_LEFT - 1 )) - - # run the commands - if [ -n "$PXY_CMD" ]; then - printf "# %s\n%s\n" "$NAME" "$PXY_CMD" > $PXY_OUT - $PXY_CMD >> $PXY_OUT 2>&1 & - PXY_PID=$! - wait_proxy_start "$PXY_PORT" "$PXY_PID" - fi - - check_osrv_dtls - printf '# %s\n%s\n' "$NAME" "$SRV_CMD" > $SRV_OUT - provide_input | $SRV_CMD >> $SRV_OUT 2>&1 & - SRV_PID=$! - wait_server_start "$SRV_PORT" "$SRV_PID" - - printf '# %s\n%s\n' "$NAME" "$CLI_CMD" > $CLI_OUT - eval "$CLI_CMD" >> $CLI_OUT 2>&1 & - wait_client_done - - sleep 0.05 - - # terminate the server (and the proxy) - kill $SRV_PID - wait $SRV_PID - SRV_RET=$? - - if [ -n "$PXY_CMD" ]; then - kill $PXY_PID >/dev/null 2>&1 - wait $PXY_PID - fi - - # retry only on timeouts - if grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null; then - printf "RETRY " - else - TIMES_LEFT=0 - fi - done +# Check for failure conditions after a test case. +# +# Inputs from run_test: +# * positional parameters: test options (see run_test documentation) +# * $CLI_EXIT: client return code +# * $CLI_EXPECT: expected client return code +# * $SRV_RET: server return code +# * $CLI_OUT, $SRV_OUT, $PXY_OUT: files containing client/server/proxy logs +# +# Outputs: +# * $pass: set to 1 if no failures are detected, 0 otherwise +check_test_failure() { + pass=0 # check if the client and server went at least to the handshake stage # (useful to avoid tests with only negative assertions and non-zero @@ -1085,6 +1002,120 @@ run_test() { fi # if we're here, everything is ok + pass=1 +} + +# Usage: run_test name [-p proxy_cmd] srv_cmd cli_cmd cli_exit [option [...]] +# Options: -s pattern pattern that must be present in server output +# -c pattern pattern that must be present in client output +# -u pattern lines after pattern must be unique in client output +# -f call shell function on client output +# -S pattern pattern that must be absent in server output +# -C pattern pattern that must be absent in client output +# -U pattern lines after pattern must be unique in server output +# -F call shell function on server output +# -g call shell function on server and client output +run_test() { + NAME="$1" + shift 1 + + if is_excluded "$NAME"; then + SKIP_NEXT="NO" + # There was no request to run the test, so don't record its outcome. + return + fi + + print_name "$NAME" + + # Do we only run numbered tests? + if [ -n "$RUN_TEST_NUMBER" ]; then + case ",$RUN_TEST_NUMBER," in + *",$TESTS,"*) :;; + *) SKIP_NEXT="YES";; + esac + fi + + # does this test use a proxy? + if [ "X$1" = "X-p" ]; then + PXY_CMD="$2" + shift 2 + else + PXY_CMD="" + fi + + # get commands and client output + SRV_CMD="$1" + CLI_CMD="$2" + CLI_EXPECT="$3" + shift 3 + + # Check if test uses files + case "$SRV_CMD $CLI_CMD" in + *data_files/*) + requires_config_enabled MBEDTLS_FS_IO;; + esac + + # If the client or serve requires a ciphersuite, check that it's enabled. + maybe_requires_ciphersuite_enabled "$SRV_CMD" "$@" + maybe_requires_ciphersuite_enabled "$CLI_CMD" "$@" + + # should we skip? + if [ "X$SKIP_NEXT" = "XYES" ]; then + SKIP_NEXT="NO" + record_outcome "SKIP" + SKIPS=$(( $SKIPS + 1 )) + return + fi + + analyze_test_commands "$@" + + TIMES_LEFT=2 + while [ $TIMES_LEFT -gt 0 ]; do + TIMES_LEFT=$(( $TIMES_LEFT - 1 )) + + # run the commands + if [ -n "$PXY_CMD" ]; then + printf "# %s\n%s\n" "$NAME" "$PXY_CMD" > $PXY_OUT + $PXY_CMD >> $PXY_OUT 2>&1 & + PXY_PID=$! + wait_proxy_start "$PXY_PORT" "$PXY_PID" + fi + + check_osrv_dtls + printf '# %s\n%s\n' "$NAME" "$SRV_CMD" > $SRV_OUT + provide_input | $SRV_CMD >> $SRV_OUT 2>&1 & + SRV_PID=$! + wait_server_start "$SRV_PORT" "$SRV_PID" + + printf '# %s\n%s\n' "$NAME" "$CLI_CMD" > $CLI_OUT + eval "$CLI_CMD" >> $CLI_OUT 2>&1 & + wait_client_done + + sleep 0.05 + + # terminate the server (and the proxy) + kill $SRV_PID + wait $SRV_PID + SRV_RET=$? + + if [ -n "$PXY_CMD" ]; then + kill $PXY_PID >/dev/null 2>&1 + wait $PXY_PID + fi + + # retry only on timeouts + if grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null; then + printf "RETRY " + else + TIMES_LEFT=0 + fi + done + + check_test_failure "$@" + if [ "$pass" -eq 0 ]; then + return + fi + record_outcome "PASS" if [ "$PRESERVE_LOGS" -gt 0 ]; then mv $SRV_OUT o-srv-${TESTS}.log From 196d73bc1b9dd4a97c3a271af07ac9cf6a3fb4fb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 19 Oct 2021 16:35:35 +0200 Subject: [PATCH 08/48] Move the core loop of run_test into an auxiliary function No behavior change. Signed-off-by: Gilles Peskine --- tests/ssl-opt.sh | 73 +++++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 1d73d3af05ad..8f403113ccf1 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1005,6 +1005,49 @@ check_test_failure() { pass=1 } +# Run the current test case: start the server and if applicable the proxy, run +# the client, wait for all processes to finish or time out. +# +# Inputs: +# * $NAME: test case name +# * $CLI_CMD, $SRV_CMD, $PXY_CMD: commands to run +# * $CLI_OUT, $SRV_OUT, $PXY_OUT: files to contain client/server/proxy logs +# +# Outputs: +# * $CLI_EXIT: client return code +# * $SRV_RET: server return code +do_run_test_once() { + # run the commands + if [ -n "$PXY_CMD" ]; then + printf "# %s\n%s\n" "$NAME" "$PXY_CMD" > $PXY_OUT + $PXY_CMD >> $PXY_OUT 2>&1 & + PXY_PID=$! + wait_proxy_start "$PXY_PORT" "$PXY_PID" + fi + + check_osrv_dtls + printf '# %s\n%s\n' "$NAME" "$SRV_CMD" > $SRV_OUT + provide_input | $SRV_CMD >> $SRV_OUT 2>&1 & + SRV_PID=$! + wait_server_start "$SRV_PORT" "$SRV_PID" + + printf '# %s\n%s\n' "$NAME" "$CLI_CMD" > $CLI_OUT + eval "$CLI_CMD" >> $CLI_OUT 2>&1 & + wait_client_done + + sleep 0.05 + + # terminate the server (and the proxy) + kill $SRV_PID + wait $SRV_PID + SRV_RET=$? + + if [ -n "$PXY_CMD" ]; then + kill $PXY_PID >/dev/null 2>&1 + wait $PXY_PID + fi +} + # Usage: run_test name [-p proxy_cmd] srv_cmd cli_cmd cli_exit [option [...]] # Options: -s pattern pattern that must be present in server output # -c pattern pattern that must be present in client output @@ -1073,35 +1116,7 @@ run_test() { while [ $TIMES_LEFT -gt 0 ]; do TIMES_LEFT=$(( $TIMES_LEFT - 1 )) - # run the commands - if [ -n "$PXY_CMD" ]; then - printf "# %s\n%s\n" "$NAME" "$PXY_CMD" > $PXY_OUT - $PXY_CMD >> $PXY_OUT 2>&1 & - PXY_PID=$! - wait_proxy_start "$PXY_PORT" "$PXY_PID" - fi - - check_osrv_dtls - printf '# %s\n%s\n' "$NAME" "$SRV_CMD" > $SRV_OUT - provide_input | $SRV_CMD >> $SRV_OUT 2>&1 & - SRV_PID=$! - wait_server_start "$SRV_PORT" "$SRV_PID" - - printf '# %s\n%s\n' "$NAME" "$CLI_CMD" > $CLI_OUT - eval "$CLI_CMD" >> $CLI_OUT 2>&1 & - wait_client_done - - sleep 0.05 - - # terminate the server (and the proxy) - kill $SRV_PID - wait $SRV_PID - SRV_RET=$? - - if [ -n "$PXY_CMD" ]; then - kill $PXY_PID >/dev/null 2>&1 - wait $PXY_PID - fi + do_run_test_once # retry only on timeouts if grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null; then From 0e3534c67b396539a3bf498a92b9f0241d0a37f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 19 Oct 2021 17:23:25 +0200 Subject: [PATCH 09/48] Move retry logic into check_test_failure This will allow having other retry conditions, in particular based on run_test options. Signed-off-by: Gilles Peskine --- tests/ssl-opt.sh | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8f403113ccf1..b41a91f6ffea 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -864,11 +864,19 @@ analyze_test_commands() { # * $CLI_EXPECT: expected client return code # * $SRV_RET: server return code # * $CLI_OUT, $SRV_OUT, $PXY_OUT: files containing client/server/proxy logs +# * $TIMES_LEFT: if nonzero, a RETRY outcome is allowed # # Outputs: -# * $pass: set to 1 if no failures are detected, 0 otherwise +# * $outcome: one of PASS/RETRY/FAIL check_test_failure() { - pass=0 + outcome=FAIL + + if [ $TIMES_LEFT -gt 0 ] && + grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null + then + outcome=RETRY + return + fi # check if the client and server went at least to the handshake stage # (useful to avoid tests with only negative assertions and non-zero @@ -1002,7 +1010,7 @@ check_test_failure() { fi # if we're here, everything is ok - pass=1 + outcome=PASS } # Run the current test case: start the server and if applicable the proxy, run @@ -1118,19 +1126,15 @@ run_test() { do_run_test_once - # retry only on timeouts - if grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null; then - printf "RETRY " - else - TIMES_LEFT=0 - fi + check_test_failure "$@" + case $outcome in + PASS) break;; + RETRY) printf "RETRY ";; + FAIL) return;; + esac done - check_test_failure "$@" - if [ "$pass" -eq 0 ]; then - return - fi - + # If we get this far, the test case passed. record_outcome "PASS" if [ "$PRESERVE_LOGS" -gt 0 ]; then mv $SRV_OUT o-srv-${TESTS}.log From f11d30ecda8c6ea046972edfdd001eacf102e213 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 19 Oct 2021 18:00:10 +0200 Subject: [PATCH 10/48] Retry if a test case fails because of an unexpected resend Palliative for https://github.com/ARMmbed/mbedtls/issues/3377. If a test case fails due to an unexpected resend, allow retrying, like in the case of a client timeout. Signed-off-by: Gilles Peskine --- tests/ssl-opt.sh | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index b41a91f6ffea..23169e466d2a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -867,14 +867,14 @@ analyze_test_commands() { # * $TIMES_LEFT: if nonzero, a RETRY outcome is allowed # # Outputs: -# * $outcome: one of PASS/RETRY/FAIL +# * $outcome: one of PASS/RETRY*/FAIL check_test_failure() { outcome=FAIL if [ $TIMES_LEFT -gt 0 ] && grep '===CLIENT_TIMEOUT===' $CLI_OUT >/dev/null then - outcome=RETRY + outcome="RETRY(client-timeout)" return fi @@ -939,14 +939,22 @@ check_test_failure() { "-S") if grep -v '^==' $SRV_OUT | grep -v 'Serious error when reading debug info' | grep "$2" >/dev/null; then - fail "pattern '$2' MUST NOT be present in the Server output" + if [ "$2" = "resend" ] && [ $TIMES_LEFT -gt 0 ]; then + outcome="RETRY(resend)" + else + fail "pattern '$2' MUST NOT be present in the Server output" + fi return fi ;; "-C") if grep -v '^==' $CLI_OUT | grep -v 'Serious error when reading debug info' | grep "$2" >/dev/null; then - fail "pattern '$2' MUST NOT be present in the Client output" + if [ "$2" = "resend" ] && [ $TIMES_LEFT -gt 0 ]; then + outcome="RETRY(resend)" + else + fail "pattern '$2' MUST NOT be present in the Client output" + fi return fi ;; @@ -1129,7 +1137,7 @@ run_test() { check_test_failure "$@" case $outcome in PASS) break;; - RETRY) printf "RETRY ";; + RETRY*) printf "$outcome ";; FAIL) return;; esac done From 58ed8a7594ce279a13f85762d7449d369fb1f530 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Tue, 19 Oct 2021 17:56:39 +0100 Subject: [PATCH 11/48] Remove use of -p with lsof On machines with more modern kernels (>5.4 from testing so far) the useage of -b seems to conflict with the usage of -p. Whilst the usage of -b seems like a good idea to avoid blocks as we are tight looping on it, the usage of -p seems to require the usage of stat() (specifically in /proc) which -b forbids. All you get is a load of warnings (suppressable by -w) but never a positive result, which means that all servers are reported as "Failed to start". We are not keen on losing -b, so instead parse the output of lsof (using -F to format it) to check the if PIDs that it outputs match that we are looking for. Signed-off-by: Paul Elliott --- tests/ssl-opt.sh | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e90a35226bea..d5b9150ca039 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -633,7 +633,21 @@ if type lsof >/dev/null 2>/dev/null; then proto=TCP fi # Make a tight loop, server normally takes less than 1s to start. - while ! lsof -a -n -b -i "$proto:$1" -p "$2" >/dev/null 2>/dev/null; do + while true; do + SERVER_PIDS=$(lsof -a -n -b -i "$proto:$1" -F p | cut -c2-) + SERVER_FOUND=false + # When proxies are used, more than one PID can be listening on + # the same port. Each PID will be on its own line. + while read -r PID; do + if [[ $PID == $2 ]]; then + SERVER_FOUND=true + break + fi + done <<< "$SERVER_PIDS" + + if ($SERVER_FOUND == true); then + break + fi if [ $(( $(date +%s) - $START_TIME )) -gt $DOG_DELAY ]; then echo "$3 START TIMEOUT" echo "$3 START TIMEOUT" >> $4 From 788ad339b8cbf1dbb6233f6be92e2261c3e67194 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Oct 2021 14:17:02 +0200 Subject: [PATCH 12/48] Move is-it-resend logic into a function Improve the code structure in case we want to add other similar conditions later. Document better what we're doing, and document why we're doing it. Signed-off-by: Gilles Peskine --- tests/ssl-opt.sh | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 23169e466d2a..fa34ff66f30a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -552,6 +552,32 @@ record_outcome() { fi } +# True if the presence of the given pattern in a log definitely indicates +# that the test has failed. False if the presence is inconclusive. +# +# Inputs: +# * $1: pattern found in the logs +# * $TIMES_LEFT: >0 if retrying is an option +# +# Outputs: +# * $outcome: set to a retry reason if the pattern is inconclusive, +# unchanged otherwise. +# * Return value: 1 if the pattern is inconclusive, +# 0 if the failure is definitive. +log_pattern_presence_is_conclusive() { + # If we've run out of attempts, then don't retry no matter what. + if [ $TIMES_LEFT -eq 0 ]; then + return 0 + fi + case $1 in + "resend") + # An undesired resend may have been caused by the OS dropping or + # delaying a packet at an inopportune time. + outcome="RETRY(resend)" + return 1;; + esac +} + # fail fail() { record_outcome "FAIL" "$1" @@ -939,9 +965,7 @@ check_test_failure() { "-S") if grep -v '^==' $SRV_OUT | grep -v 'Serious error when reading debug info' | grep "$2" >/dev/null; then - if [ "$2" = "resend" ] && [ $TIMES_LEFT -gt 0 ]; then - outcome="RETRY(resend)" - else + if log_pattern_presence_is_conclusive "$2"; then fail "pattern '$2' MUST NOT be present in the Server output" fi return @@ -950,9 +974,7 @@ check_test_failure() { "-C") if grep -v '^==' $CLI_OUT | grep -v 'Serious error when reading debug info' | grep "$2" >/dev/null; then - if [ "$2" = "resend" ] && [ $TIMES_LEFT -gt 0 ]; then - outcome="RETRY(resend)" - else + if log_pattern_presence_is_conclusive "$2"; then fail "pattern '$2' MUST NOT be present in the Client output" fi return From 8132c2ff46d532ce9350860b25f6955d842438f6 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Thu, 21 Oct 2021 12:26:58 +0200 Subject: [PATCH 13/48] Address review comments Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_server2.c | 42 ++++++++++++++++++++++++++++---------- tests/ssl-opt.sh | 4 ++-- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 68e92b7121dd..c23d73045fb9 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -203,7 +203,7 @@ int main( void ) #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_X509_CRT_PARSE_C) #define USAGE_KEY_OPAQUE \ - " key_opaque=%%d Handle your private key as if it were opaque\n" \ + " key_opaque=%%d Handle your private keys as if they were opaque\n" \ " default: 0 (disabled)\n" #else #define USAGE_KEY_OPAQUE "" @@ -1325,8 +1325,9 @@ int main( int argc, char *argv[] ) mbedtls_pk_context pkey; mbedtls_x509_crt srvcert2; mbedtls_pk_context pkey2; -#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_USE_PSA_CRYPTO) +#if defined(MBEDTLS_USE_PSA_CRYPTO) psa_key_id_t key_slot = 0; /* invalid key slot */ + psa_key_id_t key_slot2 = 0; /* invalid key slot */ #endif int key_cert_init = 0, key_cert_init2 = 0; #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) @@ -2491,23 +2492,38 @@ int main( int argc, char *argv[] ) (unsigned int) -ret ); goto exit; } + key_cert_init2 = 2; +#endif /* MBEDTLS_ECDSA_C */ + } + #if defined(MBEDTLS_USE_PSA_CRYPTO) if( opt.key_opaque != 0 ) { - if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey2, &key_slot, - PSA_ALG_SHA_256 ) ) != 0 ) + if ( mbedtls_pk_get_type( &pkey ) == MBEDTLS_PK_ECKEY ) { - mbedtls_printf( " failed\n ! " - "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); - goto exit; + if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey, &key_slot, + PSA_ALG_SHA_256 ) ) != 0 ) + { + mbedtls_printf( " failed\n ! " + "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); + goto exit; + } + } + + if ( mbedtls_pk_get_type( &pkey2 ) == MBEDTLS_PK_ECKEY ) + { + if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey2, &key_slot2, + PSA_ALG_SHA_256 ) ) != 0 ) + { + mbedtls_printf( " failed\n ! " + "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); + goto exit; + } } } #endif /* MBEDTLS_USE_PSA_CRYPTO */ - key_cert_init2 = 2; -#endif /* MBEDTLS_ECDSA_C */ - } - mbedtls_printf( " ok (key type: %s)\n", mbedtls_pk_get_name( &pkey2 ) ); + mbedtls_printf( " ok (key types: %s - %s)\n", mbedtls_pk_get_name( &pkey ), mbedtls_pk_get_name( &pkey2 ) ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO) @@ -3953,6 +3969,10 @@ int main( int argc, char *argv[] ) mbedtls_pk_free( &pkey ); mbedtls_x509_crt_free( &srvcert2 ); mbedtls_pk_free( &pkey2 ); +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_destroy_key( key_slot ); + psa_destroy_key( key_slot2 ); +#endif #endif #if defined(MBEDTLS_SSL_ASYNC_PRIVATE) for( i = 0; (size_t) i < ssl_async_keys.slots_used; i++ ) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7d0b31381f56..628fad956091 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1455,7 +1455,7 @@ run_test "Opaque key for server authentication" \ key_file=data_files/server5.key" \ 0 \ -c "Verifying peer X.509 certificate... ok" \ - -s "key type: Opaque" \ + -s "key types: RSA - Opaque" \ -S "error" \ -C "error" @@ -1471,7 +1471,7 @@ run_test "Opaque key for client/server authentication" \ 0 \ -c "key type: Opaque" \ -c "Verifying peer X.509 certificate... ok" \ - -s "key type: Opaque" \ + -s "key types: RSA - Opaque" \ -s "Verifying peer X.509 certificate... ok" \ -S "error" \ -C "error" From 33d01ffe60fe7ce2ebb8e649b657e89a4a8318dd Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Thu, 21 Oct 2021 14:55:59 +0200 Subject: [PATCH 14/48] Remove redundant value assignemnt to olen. Signed-off-by: Mateusz Starzyk --- tests/suites/test_suite_gcm.function | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index 816ebc4ec520..5696679ea954 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -438,7 +438,7 @@ void gcm_update_output_buffer_too_small( int cipher_id, int mode, { mbedtls_gcm_context ctx; uint8_t *output = NULL; - size_t olen; + size_t olen = 0; size_t output_len = input->len - 1; mbedtls_gcm_init( &ctx ); @@ -446,7 +446,6 @@ void gcm_update_output_buffer_too_small( int cipher_id, int mode, TEST_EQUAL( 0, mbedtls_gcm_starts( &ctx, mode, iv->x, iv->len ) ); ASSERT_ALLOC( output, output_len ); - olen = 0xdeadbeef; TEST_EQUAL( MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL, mbedtls_gcm_update( &ctx, input->x, input->len, output, output_len, &olen ) ); exit: From e05e126933b81403620ca56f40f08cfad1a3ecc3 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 20 Oct 2021 15:59:33 +0100 Subject: [PATCH 15/48] Remove bash specific code Use case pattern matching instead of multiline split, given there is only the well formatted PIDs to match on this should be safe. Signed-off-by: Paul Elliott --- tests/ssl-opt.sh | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d5b9150ca039..25aa5bafe763 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -626,6 +626,8 @@ has_mem_err() { # Wait for process $2 named $3 to be listening on port $1. Print error to $4. if type lsof >/dev/null 2>/dev/null; then wait_app_start() { + newline=' +' START_TIME=$(date +%s) if [ "$DTLS" -eq 1 ]; then proto=UDP @@ -634,20 +636,14 @@ if type lsof >/dev/null 2>/dev/null; then fi # Make a tight loop, server normally takes less than 1s to start. while true; do - SERVER_PIDS=$(lsof -a -n -b -i "$proto:$1" -F p | cut -c2-) - SERVER_FOUND=false - # When proxies are used, more than one PID can be listening on - # the same port. Each PID will be on its own line. - while read -r PID; do - if [[ $PID == $2 ]]; then - SERVER_FOUND=true - break - fi - done <<< "$SERVER_PIDS" - - if ($SERVER_FOUND == true); then - break - fi + SERVER_PIDS=$(lsof -a -n -b -i "$proto:$1" -F p) + # When we use a proxy, it will be listening on the same port we + # are checking for as well as the server and lsof will list both. + # If multiple PIDs are returned, each one will be on a separate + # line, each prepended with 'p'. + case ${newline}${SERVER_PIDS}${newline} in + *${newline}p${2}${newline}*) break;; + esac if [ $(( $(date +%s) - $START_TIME )) -gt $DOG_DELAY ]; then echo "$3 START TIMEOUT" echo "$3 START TIMEOUT" >> $4 From 30bd7fa607f63f1a7fa013898817ecd8385b60e6 Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Fri, 22 Oct 2021 10:33:25 +0200 Subject: [PATCH 16/48] Change error code for MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL. Signed-off-by: Mateusz Starzyk --- include/mbedtls/error.h | 2 +- include/mbedtls/gcm.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/error.h b/include/mbedtls/error.h index 27420ce4fc8f..8b2b9ea58043 100644 --- a/include/mbedtls/error.h +++ b/include/mbedtls/error.h @@ -56,7 +56,7 @@ * Module Nr Codes assigned * ERROR 2 0x006E 0x0001 * MPI 7 0x0002-0x0010 - * GCM 3 0x0012-0x0014 0x0013-0x0013 + * GCM 3 0x0012-0x0016 0x0013-0x0013 * THREADING 3 0x001A-0x001E * AES 5 0x0020-0x0022 0x0021-0x0025 * CAMELLIA 3 0x0024-0x0026 0x0027-0x0027 diff --git a/include/mbedtls/gcm.h b/include/mbedtls/gcm.h index a4de9191d864..7dc9dfb8ec2e 100644 --- a/include/mbedtls/gcm.h +++ b/include/mbedtls/gcm.h @@ -46,7 +46,7 @@ /** Bad input parameters to function. */ #define MBEDTLS_ERR_GCM_BAD_INPUT -0x0014 /** An output buffer is too small. */ -#define MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL -0x0018 +#define MBEDTLS_ERR_GCM_BUFFER_TOO_SMALL -0x0016 #ifdef __cplusplus extern "C" { From 2c1442ebc03e7fb3ea3012876f964d4c90a5b2b2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 26 Jul 2021 20:20:54 +0200 Subject: [PATCH 17/48] New sample program to benchmark certificate loading Signed-off-by: Gilles Peskine --- programs/.gitignore | 1 + programs/Makefile | 5 + programs/x509/CMakeLists.txt | 1 + programs/x509/load_roots.c | 203 +++++++++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+) create mode 100644 programs/x509/load_roots.c diff --git a/programs/.gitignore b/programs/.gitignore index d8eb6baa03dd..deb104a40167 100644 --- a/programs/.gitignore +++ b/programs/.gitignore @@ -69,6 +69,7 @@ x509/cert_app x509/cert_req x509/cert_write x509/crl_app +x509/load_roots x509/req_app # Generated data files diff --git a/programs/Makefile b/programs/Makefile index 02eb5a143013..7f9d11e80daa 100644 --- a/programs/Makefile +++ b/programs/Makefile @@ -110,6 +110,7 @@ APPS = \ x509/cert_req \ x509/cert_write \ x509/crl_app \ + x509/load_roots \ x509/req_app \ # End of APPS @@ -387,6 +388,10 @@ x509/cert_req$(EXEXT): x509/cert_req.c $(DEP) echo " CC x509/cert_req.c" $(CC) $(LOCAL_CFLAGS) $(CFLAGS) x509/cert_req.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@ +x509/load_roots$(EXEXT): x509/load_roots.c $(DEP) + echo " CC x509/load_roots.c" + $(CC) $(LOCAL_CFLAGS) $(CFLAGS) x509/load_roots.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@ + x509/req_app$(EXEXT): x509/req_app.c $(DEP) echo " CC x509/req_app.c" $(CC) $(LOCAL_CFLAGS) $(CFLAGS) x509/req_app.c $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@ diff --git a/programs/x509/CMakeLists.txt b/programs/x509/CMakeLists.txt index a04fa8bcf8f4..5876b8d21ded 100644 --- a/programs/x509/CMakeLists.txt +++ b/programs/x509/CMakeLists.txt @@ -7,6 +7,7 @@ set(executables cert_req cert_write crl_app + load_roots req_app ) diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c new file mode 100644 index 000000000000..cb168126a228 --- /dev/null +++ b/programs/x509/load_roots.c @@ -0,0 +1,203 @@ +/* + * Root CA reading application + * + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + * + * This file is provided under the Apache License 2.0, or the + * GNU General Public License v2.0 or later. + * + * ********** + * Apache License 2.0: + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * ********** + * + * ********** + * GNU General Public License v2.0 or later: + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + * ********** + */ + +#include "mbedtls/build_info.h" + +#if defined(MBEDTLS_PLATFORM_C) +#include "mbedtls/platform.h" +#else +#include +#include +#define mbedtls_time time +#define mbedtls_time_t time_t +#define mbedtls_fprintf fprintf +#define mbedtls_printf printf +#define mbedtls_exit exit +#define MBEDTLS_EXIT_SUCCESS EXIT_SUCCESS +#define MBEDTLS_EXIT_FAILURE EXIT_FAILURE +#endif /* MBEDTLS_PLATFORM_C */ + +#if !defined(MBEDTLS_X509_CRT_PARSE_C) || !defined(MBEDTLS_FS_IO) || \ + !defined(MBEDTLS_TIMING_C) +int main( void ) +{ + mbedtls_printf("MBEDTLS_X509_CRT_PARSE_C and/or MBEDTLS_FS_IO and/or " + "MBEDTLS_TIMING_C not defined.\n"); + mbedtls_exit( 0 ); +} +#else + +#include "mbedtls/error.h" +#include "mbedtls/timing.h" +#include "mbedtls/x509_crt.h" + +#include +#include +#include + +#define DFL_ITERATIONS 1 +#define DFL_PRIME_CACHE 1 + +#define USAGE \ + "\n usage: load_roots param=<>... [--] {FILE|DIR}...\n" \ + "\n acceptable parameters:\n" \ + " iterations=%%d Iteration count (not including cache priming); default: 1\n" \ + " prime=%%d Prime the disk read cache? Default: 1 (yes)\n" \ + "\n" + + +/* + * global options + */ +struct options +{ + const char **filenames; /* NULL-terminated list of file names */ + unsigned iterations; /* Number of iterations to time */ + int prime_cache; /* Prime the disk read cache? */ +} opt; + + +int read_certificates( const char *const *filenames ) +{ + mbedtls_x509_crt cas; + int ret = 0; + const char *const *cur; + char error_message[200]; + + mbedtls_x509_crt_init( &cas ); + + for( cur = filenames; *cur != NULL; cur++ ) + { + ret = mbedtls_x509_crt_parse_file( &cas, *cur ); + if( ret != 0 ) + { + mbedtls_strerror( ret, error_message, sizeof( error_message ) ); + printf( "\n%s: -0x%04x (%s)\n", *cur, -ret, error_message ); + goto exit; + } + } + +exit: + mbedtls_x509_crt_free( &cas ); + return( ret == 0 ); +} + +int main( int argc, char *argv[] ) +{ + int exit_code = MBEDTLS_EXIT_FAILURE; + unsigned i, j; + struct mbedtls_timing_hr_time timer; + unsigned long ms; + + if( argc == 0 ) + { + mbedtls_printf( USAGE ); + goto exit; + } + + opt.filenames = NULL; + opt.iterations = DFL_ITERATIONS; + opt.prime_cache = DFL_PRIME_CACHE; + + for( i = 1; i < (unsigned) argc; i++ ) + { + char *p = argv[i]; + char *q = NULL; + + if( strcmp( p, "--" ) == 0 ) + break; + if( ( q = strchr( p, '=' ) ) == NULL ) + break; + *q++ = '\0'; + + for( j = 0; p + j < q; j++ ) + { + if( argv[i][j] >= 'A' && argv[i][j] <= 'Z' ) + argv[i][j] |= 0x20; + } + + if( strcmp( p, "iterations" ) == 0 ) + { + opt.iterations = atoi( q ); + } + else if( strcmp( p, "prime" ) == 0 ) + { + opt.iterations = atoi( q ) != 0; + } + else + mbedtls_printf( "Unknown option: %s\n", p ); + } + + opt.filenames = (const char**) argv + i; + if( *opt.filenames == 0 ) + { + mbedtls_printf( "Missing list of certificate files to parse\n" ); + goto exit; + } + + mbedtls_printf( "Parsing %u certificates", argc - i ); + if( opt.prime_cache ) + { + if( ! read_certificates( opt.filenames ) ) + goto exit; + mbedtls_printf( " " ); + } + + (void) mbedtls_timing_get_timer( &timer, 1 ); + for( i = 1; i <= opt.iterations; i++ ) + { + if( ! read_certificates( opt.filenames ) ) + goto exit; + mbedtls_printf( "." ); + } + ms = mbedtls_timing_get_timer( &timer, 0 ); + mbedtls_printf( "\n%u iterations -> %lu ms\n", opt.iterations, ms ); + exit_code = MBEDTLS_EXIT_SUCCESS; + +exit: + mbedtls_exit( exit_code ); +} +#endif /* necessary configuration */ From b553eaabeac6cc4a08676accfaad5458bf47c7f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 28 Jul 2021 11:33:04 +0200 Subject: [PATCH 18/48] Base64 decoding: don't use the table for '=' Base64 decoding uses equality comparison tests for characters that don't leak information about the content of the data other than its length, such as whitespace. Do this with '=' as well, since it only reveals information about the length. This way the table lookup can focus on character validity and decoding value. Signed-off-by: Gilles Peskine --- library/base64.c | 64 +++++++++++++++++++----------------------------- 1 file changed, 25 insertions(+), 39 deletions(-) diff --git a/library/base64.c b/library/base64.c index 9cf5dd41d42c..8b818c86a845 100644 --- a/library/base64.c +++ b/library/base64.c @@ -54,7 +54,7 @@ static const unsigned char base64_dec_map[128] = 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, 62, 127, 127, 127, 63, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 127, 127, - 127, 64, 127, 127, 127, 0, 1, 2, 3, 4, + 127, 127, 127, 127, 127, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 127, 127, 127, 127, 127, 127, 26, 27, 28, @@ -90,31 +90,6 @@ static void mbedtls_base64_cond_assign_uchar( unsigned char * dest, const unsign *dest = ( ( *src ) & mask ) | ( ( *dest ) & ~mask ); } -/* - * Constant flow conditional assignment to uint_32 - */ -static void mbedtls_base64_cond_assign_uint32( uint32_t * dest, const uint32_t src, - uint32_t condition ) -{ - /* MSVC has a warning about unary minus on unsigned integer types, - * but this is well-defined and precisely what we want to do here. */ -#if defined(_MSC_VER) -#pragma warning( push ) -#pragma warning( disable : 4146 ) -#endif - - /* Generate bitmask from condition, mask will either be 0xFFFFFFFF or 0 */ - uint32_t mask = ( condition | -condition ); - mask >>= 31; - mask = -mask; - -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif - - *dest = ( src & mask ) | ( ( *dest ) & ~mask ); -} - /* * Constant flow check for equality */ @@ -273,17 +248,22 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( x != 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - if( src[i] == '=' && ++j > 2 ) - return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - - dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ); - - if( src[i] > 127 || dec_map_lookup == 127 ) - return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - - if( dec_map_lookup < 64 && j != 0 ) + if( src[i] > 127 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); + if( src[i] == '=' ) + { + if( ++j > 2 ) + return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); + } + else + { + if( j != 0 ) + return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); + dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ); + if( dec_map_lookup == 127 ) + return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); + } n++; } @@ -311,10 +291,16 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; - dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ); - - mbedtls_base64_cond_assign_uint32( &j, j - 1, mbedtls_base64_eq( dec_map_lookup, 64 ) ); - x = ( x << 6 ) | ( dec_map_lookup & 0x3F ); + if( *src == '=' ) + { + --j; + x = x << 6; + } + else + { + dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ); + x = ( x << 6 ) | ( dec_map_lookup & 0x3F ); + } if( ++n == 4 ) { From ab043350525833314b5a2e6d0d2efcd6510a8142 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 28 Jul 2021 13:54:02 +0200 Subject: [PATCH 19/48] Base64 decoding: use ranges instead of tables Instead of doing constant-flow table lookup, which requires 128 memory loads for each lookup into a 128-entry table, do a range-based calculation, which requires more CPU instructions per range but there are only 5 ranges. Experimentally, this is ~12x faster on my PC (based on programs/x509/load_roots). The code is slightly smaller, too. Signed-off-by: Gilles Peskine --- library/base64.c | 64 +++++++++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 22 deletions(-) diff --git a/library/base64.c b/library/base64.c index 8b818c86a845..7d9ddf9cd86f 100644 --- a/library/base64.c +++ b/library/base64.c @@ -46,23 +46,6 @@ static const unsigned char base64_enc_map[64] = '8', '9', '+', '/' }; -static const unsigned char base64_dec_map[128] = -{ - 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, - 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, - 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, - 127, 127, 127, 127, 127, 127, 127, 127, 127, 127, - 127, 127, 127, 62, 127, 127, 127, 63, 52, 53, - 54, 55, 56, 57, 58, 59, 60, 61, 127, 127, - 127, 127, 127, 127, 127, 0, 1, 2, 3, 4, - 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, - 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, - 25, 127, 127, 127, 127, 127, 127, 26, 27, 28, - 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, - 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, - 49, 50, 51, 127, 127, 127, 127, 127 -}; - #define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ /* @@ -133,6 +116,18 @@ static unsigned char mbedtls_base64_table_lookup( const unsigned char * const ta return result; } +/* Return 0xff if low <= c <= high, 0 otherwise. + * + * Constant flow with respect to c. + */ +static unsigned char mask_of_range( unsigned char low, unsigned char high, + unsigned char c ) +{ + unsigned low_mask = ( c - low ) >> 8; + unsigned high_mask = ( c - high - 1 ) >> 8; + return( ~low_mask & high_mask & 0xff ); +} + /* * Encode a buffer into base64 format */ @@ -211,6 +206,34 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, return( 0 ); } +/* Given a Base64 digit, return its value. + * If c is not a Base64 digit ('A'..'Z', 'a'..'z', '0'..'9', '+' or '/'), + * return -1. + * + * The implementation assumes that letters are consecutive (e.g. ASCII + * but not EBCDIC). + * + * The implementation is constant-flow (no branch or memory access depending + * on the value of c) unless the compiler inlines and optimizes a specific + * access. + */ +static signed char dec_value( unsigned char c ) +{ + unsigned char val = 0; + /* For each range of digits, if c is in that range, mask val with + * the corresponding value. Since c can only be in a single range, + * only at most one masking will change val. Set val to one plus + * the desired value so that it stays 0 if c is in none of the ranges. */ + val |= mask_of_range( 'A', 'Z', c ) & ( c - 'A' + 0 + 1 ); + val |= mask_of_range( 'a', 'z', c ) & ( c - 'a' + 26 + 1 ); + val |= mask_of_range( '0', '9', c ) & ( c - '0' + 52 + 1 ); + val |= mask_of_range( '+', '+', c ) & ( c - '+' + 62 + 1 ); + val |= mask_of_range( '/', '/', c ) & ( c - '/' + 63 + 1 ); + /* At this point, val is 0 if c is an invalid digit and v+1 if c is + * a digit with the value v. */ + return( val - 1 ); +} + /* * Decode a base64-formatted buffer */ @@ -220,7 +243,6 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, size_t i, n; uint32_t j, x; unsigned char *p; - unsigned char dec_map_lookup; /* First pass: check for validity and get output length */ for( i = n = j = 0; i < slen; i++ ) @@ -260,8 +282,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, { if( j != 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), src[i] ); - if( dec_map_lookup == 127 ) + if( dec_value( src[i] ) < 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); } n++; @@ -298,8 +319,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, } else { - dec_map_lookup = mbedtls_base64_table_lookup( base64_dec_map, sizeof( base64_dec_map ), *src ); - x = ( x << 6 ) | ( dec_map_lookup & 0x3F ); + x = ( x << 6 ) | ( dec_value( *src ) & 0x3F ); } if( ++n == 4 ) From 1121cd29b65f89186102f82bf3ae0dda24029a63 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 28 Jul 2021 14:20:06 +0200 Subject: [PATCH 20/48] Base64 decode: simplify local variables Document what each local variable does when it isn't obvious from the name. Don't reuse a variable for different purposes. This commit has very little impact on the generated code (same code size on a sample Thumb build), although it does fix a theoretical bug that 2^32 spaces inside a line would be ignored instead of treated as an error. Signed-off-by: Gilles Peskine --- library/base64.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/library/base64.c b/library/base64.c index 7d9ddf9cd86f..960f778ca13c 100644 --- a/library/base64.c +++ b/library/base64.c @@ -240,19 +240,22 @@ static signed char dec_value( unsigned char c ) int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, const unsigned char *src, size_t slen ) { - size_t i, n; - uint32_t j, x; + size_t i; /* index in source */ + size_t n; /* number of digits or trailing = in source */ + uint32_t x; /* value accumulator */ + unsigned equals = 0; + int spaces_present = 0; unsigned char *p; /* First pass: check for validity and get output length */ - for( i = n = j = 0; i < slen; i++ ) + for( i = n = 0; i < slen; i++ ) { /* Skip spaces before checking for EOL */ - x = 0; + spaces_present = 0; while( i < slen && src[i] == ' ' ) { ++i; - ++x; + spaces_present = 1; } /* Spaces at end of buffer are OK */ @@ -267,7 +270,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, continue; /* Space inside a line is an error */ - if( x != 0 ) + if( spaces_present ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); if( src[i] > 127 ) @@ -275,12 +278,12 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( src[i] == '=' ) { - if( ++j > 2 ) + if( ++equals > 2 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); } else { - if( j != 0 ) + if( equals != 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); if( dec_value( src[i] ) < 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); @@ -299,7 +302,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, * n = ( ( n * 6 ) + 7 ) >> 3; */ n = ( 6 * ( n >> 3 ) ) + ( ( 6 * ( n & 0x7 ) + 7 ) >> 3 ); - n -= j; + n -= equals; if( dst == NULL || dlen < n ) { @@ -307,27 +310,24 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, return( MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL ); } - for( j = 3, n = x = 0, p = dst; i > 0; i--, src++ ) - { + equals = 0; + for( n = x = 0, p = dst; i > 0; i--, src++ ) + { if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; + x = x << 6; if( *src == '=' ) - { - --j; - x = x << 6; - } + ++equals; else - { - x = ( x << 6 ) | ( dec_value( *src ) & 0x3F ); - } + x |= dec_value( *src ); if( ++n == 4 ) { n = 0; - if( j > 0 ) *p++ = MBEDTLS_BYTE_2( x ); - if( j > 1 ) *p++ = MBEDTLS_BYTE_1( x ); - if( j > 2 ) *p++ = MBEDTLS_BYTE_0( x ); + *p++ = MBEDTLS_BYTE_2( x ); + if( equals <= 1 ) *p++ = MBEDTLS_BYTE_1( x ); + if( equals <= 0 ) *p++ = MBEDTLS_BYTE_0( x ); } } From 2c4a3686bb822c3c67e1f6157e5087ea6a3bf4db Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 28 Jul 2021 14:31:39 +0200 Subject: [PATCH 21/48] Base64 encoding: use ranges instead of tables Instead of doing constant-flow table lookup, which requires 64 memory loads for each lookup into a 64-entry table, do a range-based calculation, which requires more CPU instructions per range but there are only 5 ranges. I expect a significant performance gain (although smaller than for decoding since the encoding table is half the size), but I haven't measured. Code size is slightly smaller. Signed-off-by: Gilles Peskine --- library/base64.c | 122 ++++++++++------------------------------------- 1 file changed, 25 insertions(+), 97 deletions(-) diff --git a/library/base64.c b/library/base64.c index 960f778ca13c..832484f98fa9 100644 --- a/library/base64.c +++ b/library/base64.c @@ -35,87 +35,8 @@ #endif /* MBEDTLS_PLATFORM_C */ #endif /* MBEDTLS_SELF_TEST */ -static const unsigned char base64_enc_map[64] = -{ - 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', - 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', - 'U', 'V', 'W', 'X', 'Y', 'Z', 'a', 'b', 'c', 'd', - 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', - 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', - 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', - '8', '9', '+', '/' -}; - #define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ -/* - * Constant flow conditional assignment to unsigned char - */ -static void mbedtls_base64_cond_assign_uchar( unsigned char * dest, const unsigned char * const src, - unsigned char condition ) -{ - /* MSVC has a warning about unary minus on unsigned integer types, - * but this is well-defined and precisely what we want to do here. */ -#if defined(_MSC_VER) -#pragma warning( push ) -#pragma warning( disable : 4146 ) -#endif - - /* Generate bitmask from condition, mask will either be 0xFF or 0 */ - unsigned char mask = ( condition | -condition ); - mask >>= 7; - mask = -mask; - -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif - - *dest = ( ( *src ) & mask ) | ( ( *dest ) & ~mask ); -} - -/* - * Constant flow check for equality - */ -static unsigned char mbedtls_base64_eq( size_t in_a, size_t in_b ) -{ - size_t difference = in_a ^ in_b; - - /* MSVC has a warning about unary minus on unsigned integer types, - * but this is well-defined and precisely what we want to do here. */ -#if defined(_MSC_VER) -#pragma warning( push ) -#pragma warning( disable : 4146 ) -#endif - - difference |= -difference; - -#if defined(_MSC_VER) -#pragma warning( pop ) -#endif - - /* cope with the varying size of size_t per platform */ - difference >>= ( sizeof( difference ) * 8 - 1 ); - - return (unsigned char) ( 1 ^ difference ); -} - -/* - * Constant flow lookup into table. - */ -static unsigned char mbedtls_base64_table_lookup( const unsigned char * const table, - const size_t table_size, const size_t table_index ) -{ - size_t i; - unsigned char result = 0; - - for( i = 0; i < table_size; ++i ) - { - mbedtls_base64_cond_assign_uchar( &result, &table[i], mbedtls_base64_eq( i, table_index ) ); - } - - return result; -} - /* Return 0xff if low <= c <= high, 0 otherwise. * * Constant flow with respect to c. @@ -128,6 +49,24 @@ static unsigned char mask_of_range( unsigned char low, unsigned char high, return( ~low_mask & high_mask & 0xff ); } +/* Given a value in the range 0..63, return the corresponding Base64 digit. + * The implementation assumes that letters are consecutive (e.g. ASCII + * but not EBCDIC). + */ +static unsigned char enc_char( unsigned char val ) +{ + unsigned char digit = 0; + /* For each range of values, if val is in that range, mask digit with + * the corresponding value. Since val can only be in a single range, + * only at most one masking will change digit. */ + digit |= mask_of_range( 0, 25, val ) & ( 'A' + val ); + digit |= mask_of_range( 26, 51, val ) & ( 'a' + val - 26 ); + digit |= mask_of_range( 52, 61, val ) & ( '0' + val - 52 ); + digit |= mask_of_range( 62, 62, val ) & '+'; + digit |= mask_of_range( 63, 63, val ) & '/'; + return( digit ); +} + /* * Encode a buffer into base64 format */ @@ -168,17 +107,10 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C2 = *src++; C3 = *src++; - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( C1 >> 2 ) & 0x3F ) ); - - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ) ); - - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( ( ( C2 & 15 ) << 2 ) + ( C3 >> 6 ) ) & 0x3F ) ); - - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( C3 & 0x3F ) ); + *p++ = enc_char( ( C1 >> 2 ) & 0x3F ); + *p++ = enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ); + *p++ = enc_char( ( ( ( C2 & 15 ) << 2 ) + ( C3 >> 6 ) ) & 0x3F ); + *p++ = enc_char( C3 & 0x3F ); } if( i < slen ) @@ -186,15 +118,11 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C1 = *src++; C2 = ( ( i + 1 ) < slen ) ? *src++ : 0; - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( C1 >> 2 ) & 0x3F ) ); - - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ) ); + *p++ = enc_char( ( C1 >> 2 ) & 0x3F ); + *p++ = enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ); if( ( i + 1 ) < slen ) - *p++ = mbedtls_base64_table_lookup( base64_enc_map, sizeof( base64_enc_map ), - ( ( ( C2 & 15 ) << 2 ) & 0x3F ) ); + *p++ = enc_char( ( ( C2 & 15 ) << 2 ) & 0x3F ); else *p++ = '='; *p++ = '='; From 66884e6dae845e1e57b1c9c6b017cfd8f7856abc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 28 Jul 2021 14:37:14 +0200 Subject: [PATCH 22/48] Base64 range-based constant-flow code: changelog entry Signed-off-by: Gilles Peskine --- ChangeLog.d/base64-ranges.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/base64-ranges.txt diff --git a/ChangeLog.d/base64-ranges.txt b/ChangeLog.d/base64-ranges.txt new file mode 100644 index 000000000000..8ffba2deecd4 --- /dev/null +++ b/ChangeLog.d/base64-ranges.txt @@ -0,0 +1,4 @@ +Changes + * Improve the performance of base64 constant-flow code. The result is still + slower than the original non-constant-flow implementation, but much faster + than the previous constant-flow implemenation. Fixes #4814. From 67468e81a602b2d74fbaf6e0de7b72f7760dad9b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 30 Jul 2021 12:56:21 +0200 Subject: [PATCH 23/48] Base64 decode: simplify local variables (n) n was used for two different purposes. Give it a different name the second time. This does not seem to change the generated code when compiling with optimization for size or performance. Signed-off-by: Gilles Peskine --- library/base64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/base64.c b/library/base64.c index 832484f98fa9..d33429645b58 100644 --- a/library/base64.c +++ b/library/base64.c @@ -171,6 +171,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, size_t i; /* index in source */ size_t n; /* number of digits or trailing = in source */ uint32_t x; /* value accumulator */ + unsigned accumulated_digits = 0; unsigned equals = 0; int spaces_present = 0; unsigned char *p; @@ -239,7 +240,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, } equals = 0; - for( n = x = 0, p = dst; i > 0; i--, src++ ) + for( x = 0, p = dst; i > 0; i--, src++ ) { if( *src == '\r' || *src == '\n' || *src == ' ' ) continue; @@ -250,9 +251,9 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, else x |= dec_value( *src ); - if( ++n == 4 ) + if( ++accumulated_digits == 4 ) { - n = 0; + accumulated_digits = 0; *p++ = MBEDTLS_BYTE_2( x ); if( equals <= 1 ) *p++ = MBEDTLS_BYTE_1( x ); if( equals <= 0 ) *p++ = MBEDTLS_BYTE_0( x ); From 8635e2301fc2c9232922fce294884de99ea78923 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 30 Jul 2021 12:57:22 +0200 Subject: [PATCH 24/48] mask_of_range: simplify high comparison To test c <= high, instead of testing the sign of (high + 1) - c, negate the sign of high - c (as we're doing for c - low). This is a little easier to read and shaves 2 instructions off the arm thumb build with arm-none-eabi-gcc 7.3.1. Signed-off-by: Gilles Peskine --- library/base64.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/base64.c b/library/base64.c index d33429645b58..6ddd501a4f3b 100644 --- a/library/base64.c +++ b/library/base64.c @@ -44,9 +44,11 @@ static unsigned char mask_of_range( unsigned char low, unsigned char high, unsigned char c ) { - unsigned low_mask = ( c - low ) >> 8; - unsigned high_mask = ( c - high - 1 ) >> 8; - return( ~low_mask & high_mask & 0xff ); + /* low_mask is: 0 if low <= c, 0x...ff if low > c */ + unsigned low_mask = ( (unsigned) c - low ) >> 8; + /* high_mask is: 0 if c <= high, 0x...ff if high > c */ + unsigned high_mask = ( (unsigned) high - c ) >> 8; + return( ~( low_mask | high_mask ) & 0xff ); } /* Given a value in the range 0..63, return the corresponding Base64 digit. From 618a70ede70eefbe84805ef890ad643af74a58fa Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 30 Jul 2021 13:00:10 +0200 Subject: [PATCH 25/48] load_roots: arguments must be files I had originally thought to support directories with mbedtls_x509_crt_parse_path but it would have complicated the code more than I cared for. Remove a remnant of the original project in the documentation. Signed-off-by: Gilles Peskine --- programs/x509/load_roots.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c index cb168126a228..3ee548525029 100644 --- a/programs/x509/load_roots.c +++ b/programs/x509/load_roots.c @@ -82,7 +82,7 @@ int main( void ) #define DFL_PRIME_CACHE 1 #define USAGE \ - "\n usage: load_roots param=<>... [--] {FILE|DIR}...\n" \ + "\n usage: load_roots param=<>... [--] FILE...\n" \ "\n acceptable parameters:\n" \ " iterations=%%d Iteration count (not including cache priming); default: 1\n" \ " prime=%%d Prime the disk read cache? Default: 1 (yes)\n" \ From 01997e119f5c3d76c949f59420df0c339ce88c35 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 30 Jul 2021 13:01:36 +0200 Subject: [PATCH 26/48] load_roots: fix no-argument detection Signed-off-by: Gilles Peskine --- programs/x509/load_roots.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c index 3ee548525029..2dff95175600 100644 --- a/programs/x509/load_roots.c +++ b/programs/x509/load_roots.c @@ -132,7 +132,7 @@ int main( int argc, char *argv[] ) struct mbedtls_timing_hr_time timer; unsigned long ms; - if( argc == 0 ) + if( argc <= 1 ) { mbedtls_printf( USAGE ); goto exit; From 9a2114ca574658fd200d0e966ebf7064e531dc8f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 30 Jul 2021 13:01:52 +0200 Subject: [PATCH 27/48] load_roots: properly error out on an invalid option Signed-off-by: Gilles Peskine --- programs/x509/load_roots.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c index 2dff95175600..7e52957d8aaf 100644 --- a/programs/x509/load_roots.c +++ b/programs/x509/load_roots.c @@ -168,7 +168,11 @@ int main( int argc, char *argv[] ) opt.iterations = atoi( q ) != 0; } else + { mbedtls_printf( "Unknown option: %s\n", p ); + mbedtls_printf( USAGE ); + goto exit; + } } opt.filenames = (const char**) argv + i; From d7d3279fdf42c38f392653bfeed1b8f1bed822a8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Aug 2021 12:19:30 +0200 Subject: [PATCH 28/48] Expose internal base64 functions for testing Signed-off-by: Gilles Peskine --- include/mbedtls/base64.h | 10 ++++++++ library/base64.c | 53 +++++++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/include/mbedtls/base64.h b/include/mbedtls/base64.h index 8378589f31ff..f6f755913d6a 100644 --- a/include/mbedtls/base64.h +++ b/include/mbedtls/base64.h @@ -87,6 +87,16 @@ int mbedtls_base64_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ +#if defined(MBEDTLS_TEST_HOOKS) +/* These functions are only exposed in testing configurations for testing + * purposes and may change or disappear at any time. */ +unsigned char mbedtls_base64_mask_of_range( unsigned char low, + unsigned char high, + unsigned char c ); +unsigned char mbedtls_base64_enc_char( unsigned char val ); +signed char mbedtls_base64_dec_value( unsigned char c ); +#endif + #ifdef __cplusplus } #endif diff --git a/library/base64.c b/library/base64.c index 6ddd501a4f3b..96c94d1c6104 100644 --- a/library/base64.c +++ b/library/base64.c @@ -41,8 +41,10 @@ * * Constant flow with respect to c. */ -static unsigned char mask_of_range( unsigned char low, unsigned char high, - unsigned char c ) +MBEDTLS_STATIC_TESTABLE +unsigned char mbedtls_base64_mask_of_range( unsigned char low, + unsigned char high, + unsigned char c ) { /* low_mask is: 0 if low <= c, 0x...ff if low > c */ unsigned low_mask = ( (unsigned) c - low ) >> 8; @@ -55,17 +57,18 @@ static unsigned char mask_of_range( unsigned char low, unsigned char high, * The implementation assumes that letters are consecutive (e.g. ASCII * but not EBCDIC). */ -static unsigned char enc_char( unsigned char val ) +MBEDTLS_STATIC_TESTABLE +unsigned char mbedtls_base64_enc_char( unsigned char val ) { unsigned char digit = 0; /* For each range of values, if val is in that range, mask digit with * the corresponding value. Since val can only be in a single range, * only at most one masking will change digit. */ - digit |= mask_of_range( 0, 25, val ) & ( 'A' + val ); - digit |= mask_of_range( 26, 51, val ) & ( 'a' + val - 26 ); - digit |= mask_of_range( 52, 61, val ) & ( '0' + val - 52 ); - digit |= mask_of_range( 62, 62, val ) & '+'; - digit |= mask_of_range( 63, 63, val ) & '/'; + digit |= mbedtls_base64_mask_of_range( 0, 25, val ) & ( 'A' + val ); + digit |= mbedtls_base64_mask_of_range( 26, 51, val ) & ( 'a' + val - 26 ); + digit |= mbedtls_base64_mask_of_range( 52, 61, val ) & ( '0' + val - 52 ); + digit |= mbedtls_base64_mask_of_range( 62, 62, val ) & '+'; + digit |= mbedtls_base64_mask_of_range( 63, 63, val ) & '/'; return( digit ); } @@ -109,10 +112,12 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C2 = *src++; C3 = *src++; - *p++ = enc_char( ( C1 >> 2 ) & 0x3F ); - *p++ = enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ); - *p++ = enc_char( ( ( ( C2 & 15 ) << 2 ) + ( C3 >> 6 ) ) & 0x3F ); - *p++ = enc_char( C3 & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( C1 >> 2 ) & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) + & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( ( ( C2 & 15 ) << 2 ) + ( C3 >> 6 ) ) + & 0x3F ); + *p++ = mbedtls_base64_enc_char( C3 & 0x3F ); } if( i < slen ) @@ -120,11 +125,12 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, C1 = *src++; C2 = ( ( i + 1 ) < slen ) ? *src++ : 0; - *p++ = enc_char( ( C1 >> 2 ) & 0x3F ); - *p++ = enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( C1 >> 2 ) & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( ( ( C1 & 3 ) << 4 ) + ( C2 >> 4 ) ) + & 0x3F ); if( ( i + 1 ) < slen ) - *p++ = enc_char( ( ( C2 & 15 ) << 2 ) & 0x3F ); + *p++ = mbedtls_base64_enc_char( ( ( C2 & 15 ) << 2 ) & 0x3F ); else *p++ = '='; *p++ = '='; @@ -147,18 +153,19 @@ int mbedtls_base64_encode( unsigned char *dst, size_t dlen, size_t *olen, * on the value of c) unless the compiler inlines and optimizes a specific * access. */ -static signed char dec_value( unsigned char c ) +MBEDTLS_STATIC_TESTABLE +signed char mbedtls_base64_dec_value( unsigned char c ) { unsigned char val = 0; /* For each range of digits, if c is in that range, mask val with * the corresponding value. Since c can only be in a single range, * only at most one masking will change val. Set val to one plus * the desired value so that it stays 0 if c is in none of the ranges. */ - val |= mask_of_range( 'A', 'Z', c ) & ( c - 'A' + 0 + 1 ); - val |= mask_of_range( 'a', 'z', c ) & ( c - 'a' + 26 + 1 ); - val |= mask_of_range( '0', '9', c ) & ( c - '0' + 52 + 1 ); - val |= mask_of_range( '+', '+', c ) & ( c - '+' + 62 + 1 ); - val |= mask_of_range( '/', '/', c ) & ( c - '/' + 63 + 1 ); + val |= mbedtls_base64_mask_of_range( 'A', 'Z', c ) & ( c - 'A' + 0 + 1 ); + val |= mbedtls_base64_mask_of_range( 'a', 'z', c ) & ( c - 'a' + 26 + 1 ); + val |= mbedtls_base64_mask_of_range( '0', '9', c ) & ( c - '0' + 52 + 1 ); + val |= mbedtls_base64_mask_of_range( '+', '+', c ) & ( c - '+' + 62 + 1 ); + val |= mbedtls_base64_mask_of_range( '/', '/', c ) & ( c - '/' + 63 + 1 ); /* At this point, val is 0 if c is an invalid digit and v+1 if c is * a digit with the value v. */ return( val - 1 ); @@ -216,7 +223,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, { if( equals != 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); - if( dec_value( src[i] ) < 0 ) + if( mbedtls_base64_dec_value( src[i] ) < 0 ) return( MBEDTLS_ERR_BASE64_INVALID_CHARACTER ); } n++; @@ -251,7 +258,7 @@ int mbedtls_base64_decode( unsigned char *dst, size_t dlen, size_t *olen, if( *src == '=' ) ++equals; else - x |= dec_value( *src ); + x |= mbedtls_base64_dec_value( *src ); if( ++accumulated_digits == 4 ) { From a64417afe6d59647b3cf182f0a1185416140313a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Aug 2021 12:38:55 +0200 Subject: [PATCH 29/48] Add unit tests for base64 internal functions Add unit tests for mask_of_range(), enc_char() and dec_value(). When constant-flow testing is enabled, verify that these functions are constant-flow. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.data | 30 +++++++++++++ tests/suites/test_suite_base64.function | 58 +++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 3a892f4792a0..1f94c54348f0 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -1,3 +1,33 @@ +mask_of_range empty (1..0) +mask_of_range:1:0 + +mask_of_range empty (255..0) +mask_of_range:255:0 + +mask_of_range empty (42..7) +mask_of_range:42:7 + +mask_of_range 0..0 +mask_of_range:0:0 + +mask_of_range 42..42 +mask_of_range:42:42 + +mask_of_range 255..255 +mask_of_range:255:255 + +mask_of_range 0..255 +mask_of_range:0:255 + +mask_of_range 'A'..'Z' +mask_of_range:65:90 + +enc_char (all digits) +enc_chars:"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" + +dec_value (all characters) +dec_chars:"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" + Test case mbedtls_base64_encode #1 buffer just right mbedtls_base64_encode:"":"":0:0 diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index be9b6e8c3e66..89d7995116ec 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -8,6 +8,64 @@ * END_DEPENDENCIES */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void mask_of_range( int low_arg, int high_arg ) +{ + unsigned char low = low_arg, high = high_arg; + unsigned c; + for( c = 0; c <= 0xff; c++ ) + { + mbedtls_test_set_step( c ); + TEST_CF_SECRET( &c, sizeof( c ) ); + unsigned char m = mbedtls_base64_mask_of_range( low, high, c ); + TEST_CF_PUBLIC( &c, sizeof( c ) ); + if( low <= c && c <= high ) + TEST_EQUAL( m, 0xff ); + else + TEST_EQUAL( m, 0 ); + } +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void enc_chars( char *chars ) +{ + for( unsigned value = 0; value < 64; value++ ) + { + mbedtls_test_set_step( value ); + TEST_CF_SECRET( &value, sizeof( value ) ); + unsigned char digit = mbedtls_base64_enc_char( value ); + TEST_CF_PUBLIC( &value, sizeof( value ) ); + TEST_CF_PUBLIC( &digit, sizeof( digit ) ); + TEST_EQUAL( digit, chars[value] ); + } +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void dec_chars( char *chars ) +{ + char *p; + const size_t chars_len = strlen( chars ); + signed char expected; + + for( unsigned c = 0; c <= 0xff; c++ ) + { + mbedtls_test_set_step( c ); + p = memchr( chars, c, chars_len ); + if( p == NULL ) + expected = -1; + else + expected = p - chars; + TEST_CF_SECRET( &c, sizeof( c ) ); + signed char actual = mbedtls_base64_dec_value( c ); + TEST_CF_PUBLIC( &c, sizeof( c ) ); + TEST_CF_PUBLIC( &actual, sizeof( actual ) ); + TEST_EQUAL( actual, expected ); + } +} +/* END_CASE */ + /* BEGIN_CASE */ void mbedtls_base64_encode( char * src_string, char * dst_string, int dst_buf_size, int result ) From 987984482d46a71d9a9efca9ab4117b0343561d5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Aug 2021 13:15:04 +0200 Subject: [PATCH 30/48] Fix printf format signedness error Signed-off-by: Gilles Peskine --- programs/x509/load_roots.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c index 7e52957d8aaf..8570a0e62a51 100644 --- a/programs/x509/load_roots.c +++ b/programs/x509/load_roots.c @@ -115,7 +115,8 @@ int read_certificates( const char *const *filenames ) if( ret != 0 ) { mbedtls_strerror( ret, error_message, sizeof( error_message ) ); - printf( "\n%s: -0x%04x (%s)\n", *cur, -ret, error_message ); + printf( "\n%s: -0x%04x (%s)\n", + *cur, (unsigned) -ret, error_message ); goto exit; } } From 27298780468b8414ec2d2d9f558f87f400d711f2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Aug 2021 17:41:49 +0200 Subject: [PATCH 31/48] Mark output as public before testing it Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.function | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 89d7995116ec..c0548956ef36 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -19,6 +19,7 @@ void mask_of_range( int low_arg, int high_arg ) TEST_CF_SECRET( &c, sizeof( c ) ); unsigned char m = mbedtls_base64_mask_of_range( low, high, c ); TEST_CF_PUBLIC( &c, sizeof( c ) ); + TEST_CF_PUBLIC( &m, sizeof( m ) ); if( low <= c && c <= high ) TEST_EQUAL( m, 0xff ); else From 680747b8681742ba239cfd1072a52fbaa7e26a78 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 6 Aug 2021 14:37:01 +0200 Subject: [PATCH 32/48] Fix the build of sample programs without mbedtls_strerror Signed-off-by: Gilles Peskine --- ChangeLog.d/no-strerror.txt | 3 +++ programs/pkey/key_app_writer.c | 12 +++++++----- programs/x509/load_roots.c | 7 ++++++- 3 files changed, 16 insertions(+), 6 deletions(-) create mode 100644 ChangeLog.d/no-strerror.txt diff --git a/ChangeLog.d/no-strerror.txt b/ChangeLog.d/no-strerror.txt new file mode 100644 index 000000000000..69743a871570 --- /dev/null +++ b/ChangeLog.d/no-strerror.txt @@ -0,0 +1,3 @@ +Bugfix + * Fix the build of sample programs when neither MBEDTLS_ERROR_C nor + MBEDTLS_ERROR_STRERROR_DUMMY is enabled. diff --git a/programs/pkey/key_app_writer.c b/programs/pkey/key_app_writer.c index 8a09af5125d9..ed6addfef8a4 100644 --- a/programs/pkey/key_app_writer.c +++ b/programs/pkey/key_app_writer.c @@ -202,7 +202,9 @@ int main( int argc, char *argv[] ) { int ret = 1; int exit_code = MBEDTLS_EXIT_FAILURE; - char buf[1024]; +#if defined(MBEDTLS_ERROR_C) + char buf[200]; +#endif int i; char *p, *q; @@ -220,7 +222,9 @@ int main( int argc, char *argv[] ) mbedtls_ctr_drbg_init( &ctr_drbg ); mbedtls_pk_init( &key ); +#if defined(MBEDTLS_ERROR_C) memset( buf, 0, sizeof( buf ) ); +#endif mbedtls_mpi_init( &N ); mbedtls_mpi_init( &P ); mbedtls_mpi_init( &Q ); mbedtls_mpi_init( &D ); mbedtls_mpi_init( &E ); mbedtls_mpi_init( &DP ); @@ -316,8 +320,7 @@ int main( int argc, char *argv[] ) mbedtls_ctr_drbg_random, &ctr_drbg ); if( ret != 0 ) { - mbedtls_strerror( ret, (char *) buf, sizeof(buf) ); - mbedtls_printf( " failed\n ! mbedtls_pk_parse_keyfile returned -0x%04x - %s\n\n", (unsigned int) -ret, buf ); + mbedtls_printf( " failed\n ! mbedtls_pk_parse_keyfile returned -0x%04x", (unsigned int) -ret ); goto exit; } @@ -377,8 +380,7 @@ int main( int argc, char *argv[] ) if( ret != 0 ) { - mbedtls_strerror( ret, (char *) buf, sizeof(buf) ); - mbedtls_printf( " failed\n ! mbedtls_pk_parse_public_key returned -0x%04x - %s\n\n", (unsigned int) -ret, buf ); + mbedtls_printf( " failed\n ! mbedtls_pk_parse_public_key returned -0x%04x", (unsigned int) -ret ); goto exit; } diff --git a/programs/x509/load_roots.c b/programs/x509/load_roots.c index 8570a0e62a51..e07bed72117f 100644 --- a/programs/x509/load_roots.c +++ b/programs/x509/load_roots.c @@ -105,7 +105,6 @@ int read_certificates( const char *const *filenames ) mbedtls_x509_crt cas; int ret = 0; const char *const *cur; - char error_message[200]; mbedtls_x509_crt_init( &cas ); @@ -114,9 +113,15 @@ int read_certificates( const char *const *filenames ) ret = mbedtls_x509_crt_parse_file( &cas, *cur ); if( ret != 0 ) { +#if defined(MBEDTLS_ERROR_C) || defined(MBEDTLS_ERROR_STRERROR_DUMMY) + char error_message[200]; mbedtls_strerror( ret, error_message, sizeof( error_message ) ); printf( "\n%s: -0x%04x (%s)\n", *cur, (unsigned) -ret, error_message ); +#else + printf( "\n%s: -0x%04x\n", + *cur, (unsigned) -ret ); +#endif goto exit; } } From c1776a01d29a64e9629cdc6417ead16ed8a73ae8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 6 Aug 2021 14:47:10 +0200 Subject: [PATCH 33/48] Move declarations of testing-only base64 functions to their own header Signed-off-by: Gilles Peskine --- include/mbedtls/base64.h | 10 ----- library/base64.c | 1 + library/base64_invasive.h | 55 +++++++++++++++++++++++++ tests/suites/test_suite_base64.function | 1 + 4 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 library/base64_invasive.h diff --git a/include/mbedtls/base64.h b/include/mbedtls/base64.h index f6f755913d6a..8378589f31ff 100644 --- a/include/mbedtls/base64.h +++ b/include/mbedtls/base64.h @@ -87,16 +87,6 @@ int mbedtls_base64_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ -#if defined(MBEDTLS_TEST_HOOKS) -/* These functions are only exposed in testing configurations for testing - * purposes and may change or disappear at any time. */ -unsigned char mbedtls_base64_mask_of_range( unsigned char low, - unsigned char high, - unsigned char c ); -unsigned char mbedtls_base64_enc_char( unsigned char val ); -signed char mbedtls_base64_dec_value( unsigned char c ); -#endif - #ifdef __cplusplus } #endif diff --git a/library/base64.c b/library/base64.c index 96c94d1c6104..085c71f3c1de 100644 --- a/library/base64.c +++ b/library/base64.c @@ -22,6 +22,7 @@ #if defined(MBEDTLS_BASE64_C) #include "mbedtls/base64.h" +#include "base64_invasive.h" #include diff --git a/library/base64_invasive.h b/library/base64_invasive.h new file mode 100644 index 000000000000..9e264719d4f8 --- /dev/null +++ b/library/base64_invasive.h @@ -0,0 +1,55 @@ +/** + * \file base_invasive.h + * + * \brief Base64 module: interfaces for invasive testing only. + * + * The interfaces in this file are intended for testing purposes only. + * They SHOULD NOT be made available in library integrations except when + * building the library for testing. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef MBEDTLS_BASE64_INVASIVE_H +#define MBEDTLS_BASE64_INVASIVE_H + +#include "common.h" + +#if defined(MBEDTLS_TEST_HOOKS) +/* Return 0xff if low <= c <= high, 0 otherwise. + * + * Constant flow with respect to c. + */ +unsigned char mbedtls_base64_mask_of_range( unsigned char low, + unsigned char high, + unsigned char c ); + +/* Given a value in the range 0..63, return the corresponding Base64 digit. + * + * Operates in constant time (no branches or memory access depending on val). + */ +unsigned char mbedtls_base64_enc_char( unsigned char val ); + +/* Given a Base64 digit, return its value. + * If c is not a Base64 digit ('A'..'Z', 'a'..'z', '0'..'9', '+' or '/'), + * return -1. + * + * Operates in constant time (no branches or memory access depending on c). + */ +signed char mbedtls_base64_dec_value( unsigned char c ); +#endif /* MBEDTLS_TEST_HOOKS */ + +#endif /* MBEDTLS_SSL_INVASIVE_H */ diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index c0548956ef36..d0e116770552 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -1,5 +1,6 @@ /* BEGIN_HEADER */ #include "mbedtls/base64.h" +#include "base64_invasive.h" #include /* END_HEADER */ From ba951f558465358bde42b2dd0587adbe5ab54825 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 6 Aug 2021 14:55:55 +0200 Subject: [PATCH 34/48] Move the list of Base64 digits out of the test data This is part of the definition of the encoding, not a choice of test parameter, so keep it with the test code. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.data | 4 ++-- tests/suites/test_suite_base64.function | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 1f94c54348f0..555666807df7 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -23,10 +23,10 @@ mask_of_range 'A'..'Z' mask_of_range:65:90 enc_char (all digits) -enc_chars:"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" +enc_chars: dec_value (all characters) -dec_chars:"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" +dec_chars: Test case mbedtls_base64_encode #1 buffer just right mbedtls_base64_encode:"":"":0:0 diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index d0e116770552..8775c8dfbc70 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -2,6 +2,12 @@ #include "mbedtls/base64.h" #include "base64_invasive.h" #include + +#if defined(MBEDTLS_TEST_HOOKS) +static const char digits[] = + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; +#endif /* MBEDTLS_TEST_HOOKS */ + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -30,7 +36,7 @@ void mask_of_range( int low_arg, int high_arg ) /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ -void enc_chars( char *chars ) +void enc_chars( ) { for( unsigned value = 0; value < 64; value++ ) { @@ -39,26 +45,26 @@ void enc_chars( char *chars ) unsigned char digit = mbedtls_base64_enc_char( value ); TEST_CF_PUBLIC( &value, sizeof( value ) ); TEST_CF_PUBLIC( &digit, sizeof( digit ) ); - TEST_EQUAL( digit, chars[value] ); + TEST_EQUAL( digit, digits[value] ); } } /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ -void dec_chars( char *chars ) +void dec_chars( ) { char *p; - const size_t chars_len = strlen( chars ); signed char expected; for( unsigned c = 0; c <= 0xff; c++ ) { mbedtls_test_set_step( c ); - p = memchr( chars, c, chars_len ); + /* digits is 0-terminated. sizeof()-1 excludes the trailing 0. */ + p = memchr( digits, c, sizeof( digits ) - 1 ); if( p == NULL ) expected = -1; else - expected = p - chars; + expected = p - digits; TEST_CF_SECRET( &c, sizeof( c ) ); signed char actual = mbedtls_base64_dec_value( c ); TEST_CF_PUBLIC( &c, sizeof( c ) ); From 93365a7f450d5bab3ee5176fd36e11d5a23330fe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 6 Aug 2021 16:54:22 +0200 Subject: [PATCH 35/48] Rename variable to avoid a name clash digits is also a local variable in host_test.function, leading to compilers complaining about that shadowing the global variable in test_suite_base64.function. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.function | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 8775c8dfbc70..67fbb675059c 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -4,7 +4,7 @@ #include #if defined(MBEDTLS_TEST_HOOKS) -static const char digits[] = +static const char base64_digits[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; #endif /* MBEDTLS_TEST_HOOKS */ @@ -45,7 +45,7 @@ void enc_chars( ) unsigned char digit = mbedtls_base64_enc_char( value ); TEST_CF_PUBLIC( &value, sizeof( value ) ); TEST_CF_PUBLIC( &digit, sizeof( digit ) ); - TEST_EQUAL( digit, digits[value] ); + TEST_EQUAL( digit, base64_digits[value] ); } } /* END_CASE */ @@ -59,12 +59,12 @@ void dec_chars( ) for( unsigned c = 0; c <= 0xff; c++ ) { mbedtls_test_set_step( c ); - /* digits is 0-terminated. sizeof()-1 excludes the trailing 0. */ - p = memchr( digits, c, sizeof( digits ) - 1 ); + /* base64_digits is 0-terminated. sizeof()-1 excludes the trailing 0. */ + p = memchr( base64_digits, c, sizeof( base64_digits ) - 1 ); if( p == NULL ) expected = -1; else - expected = p - digits; + expected = p - base64_digits; TEST_CF_SECRET( &c, sizeof( c ) ); signed char actual = mbedtls_base64_dec_value( c ); TEST_CF_PUBLIC( &c, sizeof( c ) ); From ac253ea32be3707169a175e1da857ebc1626ebfe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Oct 2021 21:13:27 +0200 Subject: [PATCH 36/48] Fix copypasta in comment Signed-off-by: Gilles Peskine --- library/base64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/base64.c b/library/base64.c index 085c71f3c1de..a516c1d4a3d5 100644 --- a/library/base64.c +++ b/library/base64.c @@ -49,7 +49,7 @@ unsigned char mbedtls_base64_mask_of_range( unsigned char low, { /* low_mask is: 0 if low <= c, 0x...ff if low > c */ unsigned low_mask = ( (unsigned) c - low ) >> 8; - /* high_mask is: 0 if c <= high, 0x...ff if high > c */ + /* high_mask is: 0 if c <= high, 0x...ff if c > high */ unsigned high_mask = ( (unsigned) high - c ) >> 8; return( ~( low_mask | high_mask ) & 0xff ); } From 66c9b84f932f6671738ee72019ae47b83ad87979 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Oct 2021 21:14:06 +0200 Subject: [PATCH 37/48] Fix typo in documentation Signed-off-by: Gilles Peskine --- ChangeLog.d/base64-ranges.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/base64-ranges.txt b/ChangeLog.d/base64-ranges.txt index 8ffba2deecd4..e3f3862bfb18 100644 --- a/ChangeLog.d/base64-ranges.txt +++ b/ChangeLog.d/base64-ranges.txt @@ -1,4 +1,4 @@ Changes * Improve the performance of base64 constant-flow code. The result is still slower than the original non-constant-flow implementation, but much faster - than the previous constant-flow implemenation. Fixes #4814. + than the previous constant-flow implementation. Fixes #4814. From c2d2f217fbba6ae7f275a5c3741e36bb4a674e56 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Tue, 26 Oct 2021 12:21:45 +0200 Subject: [PATCH 38/48] ssl_client2/ssl_server_2: use PSA_ALG_ANY_HASH as algorithm for opaque key Signed-off-by: Przemyslaw Stekiel --- programs/ssl/ssl_client2.c | 2 +- programs/ssl/ssl_server2.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index a970503c876e..cd9c8bf3568b 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -1692,7 +1692,7 @@ int main( int argc, char *argv[] ) if( opt.key_opaque != 0 ) { if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey, &key_slot, - PSA_ALG_SHA_256 ) ) != 0 ) + PSA_ALG_ANY_HASH ) ) != 0 ) { mbedtls_printf( " failed\n ! " "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index c23d73045fb9..1700405ed397 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -2502,7 +2502,7 @@ int main( int argc, char *argv[] ) if ( mbedtls_pk_get_type( &pkey ) == MBEDTLS_PK_ECKEY ) { if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey, &key_slot, - PSA_ALG_SHA_256 ) ) != 0 ) + PSA_ALG_ANY_HASH ) ) != 0 ) { mbedtls_printf( " failed\n ! " "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); @@ -2513,7 +2513,7 @@ int main( int argc, char *argv[] ) if ( mbedtls_pk_get_type( &pkey2 ) == MBEDTLS_PK_ECKEY ) { if( ( ret = mbedtls_pk_wrap_as_opaque( &pkey2, &key_slot2, - PSA_ALG_SHA_256 ) ) != 0 ) + PSA_ALG_ANY_HASH ) ) != 0 ) { mbedtls_printf( " failed\n ! " "mbedtls_pk_wrap_as_opaque returned -0x%x\n\n", (unsigned int) -ret ); From bb5d48307325ca4f2ee8c3cdcc81de4112222944 Mon Sep 17 00:00:00 2001 From: Przemyslaw Stekiel Date: Tue, 26 Oct 2021 12:25:27 +0200 Subject: [PATCH 39/48] ssl-opt.sh: adapt paramteters of key opaque cases Signed-off-by: Przemyslaw Stekiel --- tests/ssl-opt.sh | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 628fad956091..e0abd3f71ee1 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1435,12 +1435,15 @@ requires_config_enabled MBEDTLS_X509_CRT_PARSE_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SHA256_C run_test "Opaque key for client authentication" \ - "$P_SRV auth_mode=required" \ + "$P_SRV auth_mode=required crt_file=data_files/server5.crt \ + key_file=data_files/server5.key" \ "$P_CLI key_opaque=1 crt_file=data_files/server5.crt \ key_file=data_files/server5.key" \ 0 \ -c "key type: Opaque" \ + -c "Ciphersuite is TLS-ECDHE-ECDSA" \ -s "Verifying peer X.509 certificate... ok" \ + -s "Ciphersuite is TLS-ECDHE-ECDSA" \ -S "error" \ -C "error" @@ -1450,12 +1453,15 @@ requires_config_enabled MBEDTLS_X509_CRT_PARSE_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SHA256_C run_test "Opaque key for server authentication" \ - "$P_SRV auth_mode=required key_opaque=1" \ + "$P_SRV auth_mode=required key_opaque=1 crt_file=data_files/server5.crt \ + key_file=data_files/server5.key" \ "$P_CLI crt_file=data_files/server5.crt \ key_file=data_files/server5.key" \ 0 \ -c "Verifying peer X.509 certificate... ok" \ - -s "key types: RSA - Opaque" \ + -c "Ciphersuite is TLS-ECDHE-ECDSA" \ + -s "key types: Opaque - invalid PK" \ + -s "Ciphersuite is TLS-ECDHE-ECDSA" \ -S "error" \ -C "error" @@ -1465,14 +1471,17 @@ requires_config_enabled MBEDTLS_X509_CRT_PARSE_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SHA256_C run_test "Opaque key for client/server authentication" \ - "$P_SRV auth_mode=required key_opaque=1" \ + "$P_SRV auth_mode=required key_opaque=1 crt_file=data_files/server5.crt \ + key_file=data_files/server5.key" \ "$P_CLI key_opaque=1 crt_file=data_files/server5.crt \ key_file=data_files/server5.key" \ 0 \ -c "key type: Opaque" \ -c "Verifying peer X.509 certificate... ok" \ - -s "key types: RSA - Opaque" \ + -c "Ciphersuite is TLS-ECDHE-ECDSA" \ + -s "key types: Opaque - invalid PK" \ -s "Verifying peer X.509 certificate... ok" \ + -s "Ciphersuite is TLS-ECDHE-ECDSA" \ -S "error" \ -C "error" From 2d5c72be0bd1e32bcf0fa06d23220002da554614 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 13 Sep 2021 07:30:09 +0000 Subject: [PATCH 40/48] TLS1.3: Add Encrypted Extensions Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 1 + library/ssl_tls13_client.c | 124 +++++++++++++++++++++++++++++++++++- library/ssl_tls13_generic.c | 33 ++++++++++ 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index fa2429d07c61..6bdb7acd18b9 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -486,6 +486,7 @@ #define MBEDTLS_SSL_HS_SERVER_HELLO 2 #define MBEDTLS_SSL_HS_HELLO_VERIFY_REQUEST 3 #define MBEDTLS_SSL_HS_NEW_SESSION_TICKET 4 +#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION 8 // NEW IN TLS 1.3 #define MBEDTLS_SSL_HS_CERTIFICATE 11 #define MBEDTLS_SSL_HS_SERVER_KEY_EXCHANGE 12 #define MBEDTLS_SSL_HS_CERTIFICATE_REQUEST 13 diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 979db3144977..686edfe3b9b0 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1395,11 +1395,125 @@ static int ssl_tls1_3_process_server_hello( mbedtls_ssl_context *ssl ) } /* - * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS + * + * EncryptedExtensions message + * + * The EncryptedExtensions message contains any extensions which + * should be protected, i.e., any which are not needed to establish + * the cryptographic context. + */ + +/* + * Overview */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) +static int ssl_tls1_3_read_encrypted_extensions( mbedtls_ssl_context *ssl ); + +/* Main entry point; orchestrates the other functions */ +static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ); + +static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, + size_t buf_len ); +static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); + +/* + * Handler for MBEDTLS_SSL_ENCRYPTED_ENTENSIONS + */ +static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) +{ + int ret; + unsigned char *buf; + size_t buf_len; + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); + + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, + &buf, &buf_len ) ); + + /* Process the message contents */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_parse( ssl, buf, buf_len ) ); + + mbedtls_ssl_tls13_add_hs_msg_to_checksum( + ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); + + MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_postprocess( ssl ) ); + +cleanup: + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse encrypted extensions" ) ); + return( ret ); + +} + +static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, + size_t buf_len ) +{ + int ret = 0; + size_t ext_len; + const unsigned char *ext; + + if( buf_len < 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); + + buf += 2; /* skip extension length */ + ext = buf; + + /* Checking for an extension length that is too short */ + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + /* Checking for an extension length that isn't aligned with the rest + * of the message */ + if( buf_len != 2 + ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", ext, ext_len ); + + while( ext_len ) + { + unsigned int ext_id = MBEDTLS_GET_UINT16_BE(ext, 0); + size_t ext_size = MBEDTLS_GET_UINT16_BE(ext, 2); + + if( ext_size + 4 > ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + /* TBD: The client MUST check EncryptedExtensions for the + * presence of any forbidden extensions and if any are found MUST abort + * the handshake with an "illegal_parameter" alert. + */ + ((void) ext_id); + + ext_len -= 4 + ext_size; + ext += 4 + ext_size; + + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + } + + return( ret ); +} + +static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "%s hasn't been implemented", __func__ ) ); mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); } @@ -1555,6 +1669,10 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) ret = ssl_tls1_3_handshake_wrapup( ssl ); break; + case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: + ret = ssl_tls13_encrypted_extensions_process( ssl ); + break; + default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index b3a4a09ddc55..949fa747410f 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -27,6 +27,39 @@ #include "mbedtls/debug.h" #include "ssl_misc.h" +#include + +int mbedtls_ssl_tls13_fetch_handshake_msg( mbedtls_ssl_context *ssl, + unsigned hs_type, + unsigned char **buf, + size_t *buflen ) +{ + int ret; + + if( ( ret = mbedtls_ssl_read_record( ssl, 0 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + goto cleanup; + } + + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || + ssl->in_msg[0] != hs_type ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate verify message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, + MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; + goto cleanup; + } + + *buf = ssl->in_msg + 4; + *buflen = ssl->in_hslen - 4; + + +cleanup: + + return( ret ); +} int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, From c1fe000cfd8cca1d9b6994519bee84bdfa0bb608 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 16 Sep 2021 03:02:14 +0000 Subject: [PATCH 41/48] TLS1.3: Solve check name issue-macro definition Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 686edfe3b9b0..a27f7239dd12 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1417,7 +1417,7 @@ static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); /* - * Handler for MBEDTLS_SSL_ENCRYPTED_ENTENSIONS + * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) { From e87e5924c9e8bf4bd6c08bb4ed375afd0d954acb Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 24 Sep 2021 07:35:32 +0000 Subject: [PATCH 42/48] Fix some issues such as naming mismatch based on comments. Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 25 ++++++++++--------------- library/ssl_tls13_generic.c | 32 -------------------------------- 2 files changed, 10 insertions(+), 47 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index a27f7239dd12..00c1835dd3d2 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1406,20 +1406,19 @@ static int ssl_tls1_3_process_server_hello( mbedtls_ssl_context *ssl ) /* * Overview */ -static int ssl_tls1_3_read_encrypted_extensions( mbedtls_ssl_context *ssl ); /* Main entry point; orchestrates the other functions */ -static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ); +static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); -static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, +static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t buf_len ); -static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ); +static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ -static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) +static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) { int ret; unsigned char *buf; @@ -1427,17 +1426,17 @@ static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_parse( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, buf_len ) ); - mbedtls_ssl_tls13_add_hs_msg_to_checksum( + mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); - MBEDTLS_SSL_PROC_CHK( ssl_tls13_encrypted_extensions_postprocess( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_postprocess_encrypted_extensions( ssl ) ); cleanup: @@ -1446,7 +1445,7 @@ static int ssl_tls13_encrypted_extensions_process( mbedtls_ssl_context *ssl ) } -static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, +static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t buf_len ) { @@ -1512,7 +1511,7 @@ static int ssl_tls13_encrypted_extensions_parse( mbedtls_ssl_context *ssl, return( ret ); } -static int ssl_tls13_encrypted_extensions_postprocess( mbedtls_ssl_context *ssl ) +static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); @@ -1669,10 +1668,6 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) ret = ssl_tls1_3_handshake_wrapup( ssl ); break; - case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: - ret = ssl_tls13_encrypted_extensions_process( ssl ); - break; - default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 949fa747410f..70c2b021035e 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -29,38 +29,6 @@ #include "ssl_misc.h" #include -int mbedtls_ssl_tls13_fetch_handshake_msg( mbedtls_ssl_context *ssl, - unsigned hs_type, - unsigned char **buf, - size_t *buflen ) -{ - int ret; - - if( ( ret = mbedtls_ssl_read_record( ssl, 0 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - goto cleanup; - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || - ssl->in_msg[0] != hs_type ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate verify message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, - MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; - goto cleanup; - } - - *buf = ssl->in_msg + 4; - *buflen = ssl->in_hslen - 4; - - -cleanup: - - return( ret ); -} - int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, unsigned char **buf, From 140f0459ed6f99d88b96d5451dc46c9abf0e1aa0 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 8 Oct 2021 08:05:53 +0000 Subject: [PATCH 43/48] Encrypted Extension: Align the code style of buffer pointer Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 45 ++++++++++++++++++++----------------- library/ssl_tls13_generic.c | 1 - 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 00c1835dd3d2..acdfa0542e47 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1450,45 +1450,46 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, size_t buf_len ) { int ret = 0; - size_t ext_len; - const unsigned char *ext; - - if( buf_len < 2 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + size_t p_ext_len; + const unsigned char *end = buf + buf_len; + const unsigned char *p = buf; - ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2); + p_ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); - buf += 2; /* skip extension length */ - ext = buf; + p += 2; /* skip extension length */ /* Checking for an extension length that is too short */ - if( ext_len > 0 && ext_len < 4 ) + if( p_ext_len > 0 && p_ext_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } /* Checking for an extension length that isn't aligned with the rest * of the message */ - if( buf_len != 2 + ext_len ) + if( buf_len != 2 + p_ext_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", ext, ext_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, p_ext_len ); - while( ext_len ) + while( p_ext_len ) { - unsigned int ext_id = MBEDTLS_GET_UINT16_BE(ext, 0); - size_t ext_size = MBEDTLS_GET_UINT16_BE(ext, 2); + unsigned int ext_id = MBEDTLS_GET_UINT16_BE(p, 0); + size_t ext_size = MBEDTLS_GET_UINT16_BE(p, 2); - if( ext_size + 4 > ext_len ) + if( ext_size + 4 > p_ext_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } @@ -1498,12 +1499,14 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, */ ((void) ext_id); - ext_len -= 4 + ext_size; - ext += 4 + ext_size; + p_ext_len -= 4 + ext_size; + p += 4 + ext_size; - if( ext_len > 0 && ext_len < 4 ) + if( p_ext_len > 0 && p_ext_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 70c2b021035e..b3a4a09ddc55 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -27,7 +27,6 @@ #include "mbedtls/debug.h" #include "ssl_misc.h" -#include int mbedtls_ssl_tls1_3_fetch_handshake_msg( mbedtls_ssl_context *ssl, unsigned hs_type, From 08da26c58f959ba4e93498d3ed626717b5ee1cdf Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Sat, 9 Oct 2021 10:12:11 +0000 Subject: [PATCH 44/48] Refine encrypted extensions parse function Change arguments of API. Send different messages base on extensions types. Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 83 +++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 36 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index acdfa0542e47..13fb47079c8f 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1411,8 +1411,8 @@ static int ssl_tls1_3_process_server_hello( mbedtls_ssl_context *ssl ) static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - size_t buf_len ); + const unsigned char *buf, + const unsigned char *end ); static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* @@ -1431,7 +1431,7 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); @@ -1445,32 +1445,27 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) } +/* Parse EncryptedExtensions message + * struct { + * Extension extensions<0..2^16-1>; + * } EncryptedExtensions; + */ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - size_t buf_len ) + const unsigned char *buf, + const unsigned char *end ) { int ret = 0; - size_t p_ext_len; - const unsigned char *end = buf + buf_len; + size_t extensions_len; const unsigned char *p = buf; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2); - p_ext_len = MBEDTLS_GET_UINT16_BE(buf, 0); - - p += 2; /* skip extension length */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); + extensions_len = MBEDTLS_GET_UINT16_BE(p, 0); - /* Checking for an extension length that is too short */ - if( p_ext_len > 0 && p_ext_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + p += 2; /* Checking for an extension length that isn't aligned with the rest * of the message */ - if( buf_len != 2 + p_ext_len ) + if( p + extensions_len != end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ @@ -1478,31 +1473,47 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, p_ext_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, extensions_len ); - while( p_ext_len ) + while( p < end ) { - unsigned int ext_id = MBEDTLS_GET_UINT16_BE(p, 0); - size_t ext_size = MBEDTLS_GET_UINT16_BE(p, 2); + unsigned int extension_type; + size_t extension_data_len; - if( ext_size + 4 > p_ext_len ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + /* + * struct { + * ExtensionType extension_type; (2 bytes) + * opaque extension_data<0..2^16-1>; + * } Extension; + */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); + extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); + extension_data_len = MBEDTLS_GET_UINT16_BE( p, 2 ); + p += 4; + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); /* TBD: The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort - * the handshake with an "illegal_parameter" alert. + * the handshake with an "unsupported_extension" alert. */ - ((void) ext_id); + switch( extension_type ) + { + + case MBEDTLS_TLS_EXT_SUPPORTED_GROUPS: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found extensions supported groups" ) ); + break; + + default: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "unsupported extension found: %d ( ignoring )", extension_type) ); + break; + } - p_ext_len -= 4 + ext_size; - p += 4 + ext_size; + extensions_len -= 4 + extension_data_len; + p += extension_data_len; - if( p_ext_len > 0 && p_ext_len < 4 ) + /* Checking for an extension length that is too short */ + if( extensions_len > 0 && extensions_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ From 97799ac27bb095d1966aa0abce294cb076fa7706 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 11 Oct 2021 10:05:54 +0000 Subject: [PATCH 45/48] Encrypted Extensions: Align code style and some check logic Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 74 ++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 13fb47079c8f..f7f7eaabaced 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1408,17 +1408,17 @@ static int ssl_tls1_3_process_server_hello( mbedtls_ssl_context *ssl ) */ /* Main entry point; orchestrates the other functions */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ); +static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ); -static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end ); -static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); +static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ); +static int ssl_tls13_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ); /* * Handler for MBEDTLS_SSL_ENCRYPTED_EXTENSIONS */ -static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) +static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) { int ret; unsigned char *buf; @@ -1431,12 +1431,13 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) &buf, &buf_len ) ); /* Process the message contents */ - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); + MBEDTLS_SSL_PROC_CHK( + ssl_tls13_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); - MBEDTLS_SSL_PROC_CHK( ssl_tls1_3_postprocess_encrypted_extensions( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_encrypted_extensions( ssl ) ); cleanup: @@ -1447,33 +1448,22 @@ static int ssl_tls1_3_process_encrypted_extensions( mbedtls_ssl_context *ssl ) /* Parse EncryptedExtensions message * struct { - * Extension extensions<0..2^16-1>; + * Extension extensions<0..2^16-1>; * } EncryptedExtensions; */ -static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end ) +static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ) { int ret = 0; size_t extensions_len; const unsigned char *p = buf; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); - extensions_len = MBEDTLS_GET_UINT16_BE(p, 0); - + extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); p += 2; - /* Checking for an extension length that isn't aligned with the rest - * of the message */ - if( p + extensions_len != end ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", p, extensions_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions", p, extensions_len ); while( p < end ) { @@ -1482,8 +1472,8 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, /* * struct { - * ExtensionType extension_type; (2 bytes) - * opaque extension_data<0..2^16-1>; + * ExtensionType extension_type; (2 bytes) + * opaque extension_data<0..2^16-1>; * } Extension; */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); @@ -1493,7 +1483,7 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); - /* TBD: The client MUST check EncryptedExtensions for the + /* The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort * the handshake with an "unsupported_extension" alert. */ @@ -1505,27 +1495,31 @@ static int ssl_tls1_3_parse_encrypted_extensions( mbedtls_ssl_context *ssl, break; default: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "unsupported extension found: %d ( ignoring )", extension_type) ); + MBEDTLS_SSL_DEBUG_MSG( + 3, ( "unsupported extension found: %u ", extension_type) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, \ + MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + return ( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); break; } - extensions_len -= 4 + extension_data_len; p += extension_data_len; + } - /* Checking for an extension length that is too short */ - if( extensions_len > 0 && extensions_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad encrypted extensions message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + /* Check that we consumed all the message. */ + if( p != end ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } return( ret ); } -static int ssl_tls1_3_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) +static int ssl_tls13_postprocess_encrypted_extensions( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CERTIFICATE_REQUEST ); return( 0 ); @@ -1643,7 +1637,7 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) break; case MBEDTLS_SSL_ENCRYPTED_EXTENSIONS: - ret = ssl_tls1_3_process_encrypted_extensions( ssl ); + ret = ssl_tls13_process_encrypted_extensions( ssl ); break; case MBEDTLS_SSL_CERTIFICATE_REQUEST: From 8db25fffb48ca5c7c5a67d288451428e1652da8a Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 13 Oct 2021 05:56:18 +0000 Subject: [PATCH 46/48] Encrypted Extensions: Change extensions length check Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index f7f7eaabaced..68f5ae568af9 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1432,7 +1432,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) /* Process the message contents */ MBEDTLS_SSL_PROC_CHK( - ssl_tls13_parse_encrypted_extensions( ssl, buf, ( buf + buf_len ) ) ); + ssl_tls13_parse_encrypted_extensions( ssl, buf, buf + buf_len ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); @@ -1458,14 +1458,17 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, int ret = 0; size_t extensions_len; const unsigned char *p = buf; + const unsigned char *extensions_end; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); p += 2; MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions", p, extensions_len ); + extensions_end = p + extensions_len; + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extensions_len ); - while( p < end ) + while( p < extensions_end ) { unsigned int extension_type; size_t extension_data_len; @@ -1476,12 +1479,12 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, * opaque extension_data<0..2^16-1>; * } Extension; */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); extension_data_len = MBEDTLS_GET_UINT16_BE( p, 2 ); p += 4; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extension_data_len ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, extension_data_len ); /* The client MUST check EncryptedExtensions for the * presence of any forbidden extensions and if any are found MUST abort @@ -1501,18 +1504,17 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, \ MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); return ( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); - break; } p += extension_data_len; } /* Check that we consumed all the message. */ - if( p != end ) + if( p != extensions_end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ - MBEDTLS_ERR_SSL_DECODE_ERROR ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, \ + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } From 7b2d4efee8b8d35b783ddf3a3bb7c263f2c04da0 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 13 Oct 2021 10:19:02 +0000 Subject: [PATCH 47/48] Change the buffer boundary check and alert type Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 2 +- library/ssl_tls13_client.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 6bdb7acd18b9..288d9b3c5cb3 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -486,7 +486,7 @@ #define MBEDTLS_SSL_HS_SERVER_HELLO 2 #define MBEDTLS_SSL_HS_HELLO_VERIFY_REQUEST 3 #define MBEDTLS_SSL_HS_NEW_SESSION_TICKET 4 -#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION 8 // NEW IN TLS 1.3 +#define MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS 8 // NEW IN TLS 1.3 #define MBEDTLS_SSL_HS_CERTIFICATE 11 #define MBEDTLS_SSL_HS_SERVER_KEY_EXCHANGE 12 #define MBEDTLS_SSL_HS_CERTIFICATE_REQUEST 13 diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 68f5ae568af9..2c2d0f3afd9a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1510,11 +1510,11 @@ static int ssl_tls13_parse_encrypted_extensions( mbedtls_ssl_context *ssl, } /* Check that we consumed all the message. */ - if( p != extensions_end ) + if( p != end ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, \ - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, \ + MBEDTLS_ERR_SSL_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } From ab7f50d6389b9f6a6e2d60881d68c28f67126bf5 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 21 Oct 2021 06:23:29 +0000 Subject: [PATCH 48/48] Change macro names and add test script for extensions Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 4 ++-- tests/ssl-opt.sh | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 2c2d0f3afd9a..5ed01aade27a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1427,7 +1427,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse encrypted extensions" ) ); MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls1_3_fetch_handshake_msg( ssl, - MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, + MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, &buf, &buf_len ) ); /* Process the message contents */ @@ -1435,7 +1435,7 @@ static int ssl_tls13_process_encrypted_extensions( mbedtls_ssl_context *ssl ) ssl_tls13_parse_encrypted_extensions( ssl, buf, buf + buf_len ) ); mbedtls_ssl_tls1_3_add_hs_msg_to_checksum( - ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSION, buf, buf_len ); + ssl, MBEDTLS_SSL_HS_ENCRYPTED_EXTENSIONS, buf, buf_len ); MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_encrypted_extensions( ssl ) ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8fbe67739b5d..f9bfec2e1e52 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8808,7 +8808,7 @@ requires_openssl_tls1_3 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL requires_config_disabled MBEDTLS_USE_PSA_CRYPTO run_test "TLS1.3: Test client hello msg work - openssl" \ - "$O_NEXT_SRV -tls1_3 -msg" \ + "$O_NEXT_SRV -tls1_3 -msg -no_middlebox" \ "$P_CLI debug_level=3 min_version=tls1_3 max_version=tls1_3" \ 1 \ -c "SSL - The requested feature is not available" \ @@ -8828,13 +8828,14 @@ run_test "TLS1.3: Test client hello msg work - openssl" \ -c "<= ssl_tls1_3_process_server_hello" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ - -c "=> ssl_tls1_3_process_server_hello" + -c "=> ssl_tls1_3_process_server_hello" \ + -c "<= parse encrypted extensions" requires_gnutls_tls1_3 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL requires_config_disabled MBEDTLS_USE_PSA_CRYPTO run_test "TLS1.3: Test client hello msg work - gnutls" \ - "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3 --debug=4" \ + "$G_NEXT_SRV --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:%DISABLE_TLS13_COMPAT_MODE --debug=4" \ "$P_CLI debug_level=3 min_version=tls1_3 max_version=tls1_3" \ 1 \ -c "SSL - The requested feature is not available" \ @@ -8854,8 +8855,8 @@ run_test "TLS1.3: Test client hello msg work - gnutls" \ -c "<= ssl_tls1_3_process_server_hello" \ -c "server hello, chosen ciphersuite: ( 1301 ) - TLS1-3-AES-128-GCM-SHA256" \ -c "ECDH curve: x25519" \ - -c "=> ssl_tls1_3_process_server_hello" - + -c "=> ssl_tls1_3_process_server_hello" \ + -c "<= parse encrypted extensions" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_MEMORY_DEBUG