From 3d404d677e8053b328b38b064242babdc24bb5a6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:47:36 +0200 Subject: [PATCH 1/5] Test PSA_MAC_FINAL_SIZE in mac_sign exactly We expect PSA_MAC_FINAL_SIZE to be exact in this implementation, so check it here. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index f4b9a8f67..fb0f2b2e4 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3038,7 +3038,8 @@ void mac_sign( int key_type_arg, memset( actual_mac, '+', sizeof( actual_mac ) ); TEST_ASSERT( mac_buffer_size <= PSA_MAC_MAX_SIZE ); - TEST_ASSERT( expected_mac->len <= mac_buffer_size ); + /* We expect PSA_MAC_FINAL_SIZE to be exact. */ + TEST_ASSERT( expected_mac->len == mac_buffer_size ); PSA_ASSERT( psa_crypto_init( ) ); From 5e65cec5e81cfd203e6f5b8c5c90ce70fae8bd85 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:38:39 +0200 Subject: [PATCH 2/5] Simplify output bounds check in mac_sign test Rely on Asan to detect a potential buffer overflow, instead of doing a manual check. This makes the code simpler and Asan can detect underflows as well as overflows. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index fb0f2b2e4..5b0054d64 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3028,15 +3028,11 @@ void mac_sign( int key_type_arg, psa_algorithm_t alg = alg_arg; psa_mac_operation_t operation = PSA_MAC_OPERATION_INIT; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; - /* Leave a little extra room in the output buffer. At the end of the - * test, we'll check that the implementation didn't overwrite onto - * this extra room. */ - uint8_t actual_mac[PSA_MAC_MAX_SIZE + 10]; + uint8_t *actual_mac = NULL; size_t mac_buffer_size = PSA_MAC_FINAL_SIZE( key_type, PSA_BYTES_TO_BITS( key->len ), alg ); size_t mac_length = 0; - memset( actual_mac, '+', sizeof( actual_mac ) ); TEST_ASSERT( mac_buffer_size <= PSA_MAC_MAX_SIZE ); /* We expect PSA_MAC_FINAL_SIZE to be exact. */ TEST_ASSERT( expected_mac->len == mac_buffer_size ); @@ -3049,6 +3045,8 @@ void mac_sign( int key_type_arg, PSA_ASSERT( psa_import_key( &attributes, key->x, key->len, &handle ) ); + ASSERT_ALLOC( actual_mac, mac_buffer_size ); + /* Calculate the MAC. */ PSA_ASSERT( psa_mac_sign_setup( &operation, handle, alg ) ); @@ -3062,13 +3060,10 @@ void mac_sign( int key_type_arg, ASSERT_COMPARE( expected_mac->x, expected_mac->len, actual_mac, mac_length ); - /* Verify that the end of the buffer is untouched. */ - TEST_ASSERT( mem_is_char( actual_mac + mac_length, '+', - sizeof( actual_mac ) - mac_length ) ); - exit: psa_destroy_key( handle ); PSA_DONE( ); + mbedtls_free( actual_mac ); } /* END_CASE */ From 8b356b5652ddf4570e175fca343fe22532ac95dc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:44:59 +0200 Subject: [PATCH 3/5] Test other output sizes for psa_mac_sign_finish Test psa_mac_sign_finish with a smaller or larger buffer. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 46 +++++++++++++++------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 5b0054d64..4b70115cc 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3032,6 +3032,13 @@ void mac_sign( int key_type_arg, size_t mac_buffer_size = PSA_MAC_FINAL_SIZE( key_type, PSA_BYTES_TO_BITS( key->len ), alg ); size_t mac_length = 0; + const size_t output_sizes_to_test[] = { + 0, + 1, + expected_mac->len - 1, + expected_mac->len, + expected_mac->len + 1, + }; TEST_ASSERT( mac_buffer_size <= PSA_MAC_MAX_SIZE ); /* We expect PSA_MAC_FINAL_SIZE to be exact. */ @@ -3045,20 +3052,35 @@ void mac_sign( int key_type_arg, PSA_ASSERT( psa_import_key( &attributes, key->x, key->len, &handle ) ); - ASSERT_ALLOC( actual_mac, mac_buffer_size ); + for( size_t i = 0; i < ARRAY_LENGTH( output_sizes_to_test ); i++ ) + { + const size_t output_size = output_sizes_to_test[i]; + psa_status_t expected_status = + ( output_size >= expected_mac->len ? PSA_SUCCESS : + PSA_ERROR_BUFFER_TOO_SMALL ); - /* Calculate the MAC. */ - PSA_ASSERT( psa_mac_sign_setup( &operation, - handle, alg ) ); - PSA_ASSERT( psa_mac_update( &operation, - input->x, input->len ) ); - PSA_ASSERT( psa_mac_sign_finish( &operation, - actual_mac, mac_buffer_size, - &mac_length ) ); + test_set_step( output_size ); + ASSERT_ALLOC( actual_mac, output_size ); - /* Compare with the expected value. */ - ASSERT_COMPARE( expected_mac->x, expected_mac->len, - actual_mac, mac_length ); + /* Calculate the MAC. */ + PSA_ASSERT( psa_mac_sign_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, + input->x, input->len ) ); + TEST_EQUAL( psa_mac_sign_finish( &operation, + actual_mac, output_size, + &mac_length ), + expected_status ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + if( expected_status == PSA_SUCCESS ) + { + ASSERT_COMPARE( expected_mac->x, expected_mac->len, + actual_mac, mac_length ); + } + mbedtls_free( actual_mac ); + actual_mac = NULL; + } exit: psa_destroy_key( handle ); From 090e16cb8b5c46bb0507c6a97f5f00232ff3c420 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 25 Aug 2020 23:59:40 +0200 Subject: [PATCH 4/5] Don't destroy the key during a MAC verification operation An early draft of the PSA crypto specification required multipart operations to keep working after destroying the key. This is no longer the case: instead, now, operations are guaranteed to fail. Mbed TLS does not comply yet, and still allows the operation to keep going. Stop testing Mbed TLS's non-compliant behavior. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 4b70115cc..1ff083c0c 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3114,7 +3114,6 @@ void mac_verify( int key_type_arg, PSA_ASSERT( psa_mac_verify_setup( &operation, handle, alg ) ); - PSA_ASSERT( psa_destroy_key( handle ) ); PSA_ASSERT( psa_mac_update( &operation, input->x, input->len ) ); PSA_ASSERT( psa_mac_verify_finish( &operation, From 29c4a6cf9f03e00e6db5174fe462326dadf264d4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 26 Aug 2020 00:01:39 +0200 Subject: [PATCH 5/5] Add negative tests for MAC verification Add negative tests for psa_mac_verify_finish: too large, too small, or a changed byte. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto.function | 41 +++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 1ff083c0c..b0b4ed6a2 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -3101,6 +3101,7 @@ void mac_verify( int key_type_arg, psa_algorithm_t alg = alg_arg; psa_mac_operation_t operation = PSA_MAC_OPERATION_INIT; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; + uint8_t *perturbed_mac = NULL; TEST_ASSERT( expected_mac->len <= PSA_MAC_MAX_SIZE ); @@ -3112,6 +3113,7 @@ void mac_verify( int key_type_arg, PSA_ASSERT( psa_import_key( &attributes, key->x, key->len, &handle ) ); + /* Test the correct MAC. */ PSA_ASSERT( psa_mac_verify_setup( &operation, handle, alg ) ); PSA_ASSERT( psa_mac_update( &operation, @@ -3120,9 +3122,48 @@ void mac_verify( int key_type_arg, expected_mac->x, expected_mac->len ) ); + /* Test a MAC that's too short. */ + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, + input->x, input->len ) ); + TEST_EQUAL( psa_mac_verify_finish( &operation, + expected_mac->x, + expected_mac->len - 1 ), + PSA_ERROR_INVALID_SIGNATURE ); + + /* Test a MAC that's too long. */ + ASSERT_ALLOC( perturbed_mac, expected_mac->len + 1 ); + memcpy( perturbed_mac, expected_mac->x, expected_mac->len ); + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, + input->x, input->len ) ); + TEST_EQUAL( psa_mac_verify_finish( &operation, + perturbed_mac, + expected_mac->len + 1 ), + PSA_ERROR_INVALID_SIGNATURE ); + + /* Test changing one byte. */ + for( size_t i = 0; i < expected_mac->len; i++ ) + { + test_set_step( i ); + perturbed_mac[i] ^= 1; + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, + input->x, input->len ) ); + TEST_EQUAL( psa_mac_verify_finish( &operation, + perturbed_mac, + expected_mac->len ), + PSA_ERROR_INVALID_SIGNATURE ); + perturbed_mac[i] ^= 1; + } + exit: psa_destroy_key( handle ); PSA_DONE( ); + mbedtls_free( perturbed_mac ); } /* END_CASE */