From fe96fbec2c5d9d46ed40c4a0184baf6359adb11e Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 20 Feb 2019 10:32:28 +0000 Subject: [PATCH 1/8] Initialize PSA Crypto operation contexts It is now required to initialize PSA Crypto operation contexts before calling psa_*_setup(). Otherwise, one gets a PSA_ERROR_BAD_STATE error. --- library/cipher.c | 2 +- library/ssl_tls.c | 6 +++--- library/x509_crt.c | 2 +- library/x509write_csr.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 63f1f411d..e854cf669 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1236,7 +1236,7 @@ int mbedtls_cipher_crypt( mbedtls_cipher_context_t *ctx, (mbedtls_cipher_context_psa *) ctx->cipher_ctx; psa_status_t status; - psa_cipher_operation_t cipher_op; + psa_cipher_operation_t cipher_op = PSA_CIPHER_OPERATION_INIT; size_t part_len; if( ctx->operation == MBEDTLS_DECRYPT ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f224d5e94..a0d2617c9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6529,7 +6529,7 @@ static void ssl_calc_finished_tls_sha256( unsigned char padbuf[32]; #if defined(MBEDTLS_USE_PSA_CRYPTO) size_t hash_size; - psa_hash_operation_t sha256_psa; + psa_hash_operation_t sha256_psa = PSA_HASH_OPERATION_INIT; psa_status_t status; #else mbedtls_sha256_context sha256; @@ -6605,7 +6605,7 @@ static void ssl_calc_finished_tls_sha384( unsigned char padbuf[48]; #if defined(MBEDTLS_USE_PSA_CRYPTO) size_t hash_size; - psa_hash_operation_t sha384_psa; + psa_hash_operation_t sha384_psa = PSA_HASH_OPERATION_INIT; psa_status_t status; #else mbedtls_sha512_context sha512; @@ -10203,7 +10203,7 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, mbedtls_md_type_t md_alg ) { psa_status_t status; - psa_hash_operation_t hash_operation; + psa_hash_operation_t hash_operation = PSA_HASH_OPERATION_INIT; psa_algorithm_t hash_alg = mbedtls_psa_translate_md( md_alg ); MBEDTLS_SSL_DEBUG_MSG( 1, ( "Perform PSA-based computation of digest of ServerKeyExchange" ) ); diff --git a/library/x509_crt.c b/library/x509_crt.c index 1b1f0a771..1f853baa3 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1908,7 +1908,7 @@ static int x509_crt_check_signature( const mbedtls_x509_crt *child, if( mbedtls_md( md_info, child->tbs.p, child->tbs.len, hash ) != 0 ) return( -1 ); #else - psa_hash_operation_t hash_operation; + psa_hash_operation_t hash_operation = PSA_HASH_OPERATION_INIT; psa_algorithm_t hash_alg = mbedtls_psa_translate_md( child->sig_md ); if( psa_hash_setup( &hash_operation, hash_alg ) != PSA_SUCCESS ) diff --git a/library/x509write_csr.c b/library/x509write_csr.c index f2950ad2f..777a6325f 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -142,7 +142,7 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s size_t len = 0; mbedtls_pk_type_t pk_alg; #if defined(MBEDTLS_USE_PSA_CRYPTO) - psa_hash_operation_t hash_operation; + psa_hash_operation_t hash_operation = PSA_HASH_OPERATION_INIT; size_t hash_len; psa_algorithm_t hash_alg = mbedtls_psa_translate_md( ctx->md_alg ); #endif /* MBEDTLS_USE_PSA_CRYPTO */ From b281f7428465e793df00003a121dbe16d129c0a6 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 20 Feb 2019 10:40:20 +0000 Subject: [PATCH 2/8] psa: example: Initialize operation contexts Add missing initializers to PSA Crypto example. Operation contexts must be initialized before calling psa_*_setup(). --- programs/psa/crypto_examples.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/psa/crypto_examples.c b/programs/psa/crypto_examples.c index 9947a70bc..090875613 100644 --- a/programs/psa/crypto_examples.c +++ b/programs/psa/crypto_examples.c @@ -109,7 +109,7 @@ static psa_status_t cipher_encrypt( psa_key_handle_t key_handle, size_t *output_len ) { psa_status_t status; - psa_cipher_operation_t operation; + psa_cipher_operation_t operation = PSA_CIPHER_OPERATION_INIT; size_t iv_len = 0; memset( &operation, 0, sizeof( operation ) ); @@ -140,7 +140,7 @@ static psa_status_t cipher_decrypt( psa_key_handle_t key_handle, size_t *output_len ) { psa_status_t status; - psa_cipher_operation_t operation; + psa_cipher_operation_t operation = PSA_CIPHER_OPERATION_INIT; memset( &operation, 0, sizeof( operation ) ); status = psa_cipher_decrypt_setup( &operation, key_handle, alg ); From 252ef28dac1eca278b409a05636b587ea0d29f2f Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 15 Feb 2019 14:05:35 +0000 Subject: [PATCH 3/8] psa: Disallow use of invalid MAC contexts Ensure that when doing MAC operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification. --- library/psa_crypto.c | 10 ++ tests/suites/test_suite_psa_crypto.data | 4 + tests/suites/test_suite_psa_crypto.function | 129 ++++++++++++++++++++ 3 files changed, 143 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ad7367b9c..9bfe8d28c 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2238,6 +2238,11 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, { psa_status_t status; + if( operation->alg == 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + /* Fill the output buffer with something that isn't a valid mac * (barring an attack on the mac and deliberately-crafted input), * in case the caller doesn't check the return status properly. */ @@ -2276,6 +2281,11 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, uint8_t actual_mac[PSA_MAC_MAX_SIZE]; psa_status_t status; + if( operation->alg == 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + if( operation->is_sign ) { status = PSA_ERROR_BAD_STATE; diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index d9dd9ef48..489389a84 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -705,6 +705,10 @@ depends_on:MBEDTLS_CMAC_C # Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here mac_setup:PSA_KEY_TYPE_HMAC:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CMAC:PSA_ERROR_NOT_SUPPORTED +PSA MAC: bad order function calls +depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C +mac_bad_order: + PSA MAC sign: RFC4231 Test case 1 - HMAC-SHA-224 depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C mac_sign:PSA_KEY_TYPE_HMAC:"0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b":PSA_ALG_HMAC(PSA_ALG_SHA_224):"4869205468657265":"896fb1128abbdf196832107cd49df33f47b4b1169912ba4f53684b22" diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 929d1b268..37b4d8d69 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2168,6 +2168,8 @@ exit: /* BEGIN_CASE */ void mac_operation_init( ) { + const uint8_t input[1] = { 0 }; + /* Test each valid way of initializing the object, except for `= {0}`, as * Clang 5 complains when `-Wmissing-field-initializers` is used, even * though it's OK by the C standard. We could test for this, but we'd need @@ -2178,6 +2180,17 @@ void mac_operation_init( ) memset( &zero, 0, sizeof( zero ) ); + /* A freshly-initialized MAC operation should not be usable. */ + TEST_EQUAL( psa_mac_update( &func, + input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_mac_update( &init, + input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_mac_update( &zero, + input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + /* A default MAC operation should be abortable without error. */ PSA_ASSERT( psa_mac_abort( &func ) ); PSA_ASSERT( psa_mac_abort( &init ) ); @@ -2220,6 +2233,122 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mac_bad_order( ) +{ + psa_key_handle_t handle = 0; + psa_key_type_t key_type = PSA_KEY_TYPE_HMAC; + psa_algorithm_t alg = PSA_ALG_HMAC(PSA_ALG_SHA_256); + const uint8_t key[] = { + 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, + 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, + 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa }; + psa_key_policy_t policy = PSA_KEY_POLICY_INIT; + psa_mac_operation_t operation = PSA_MAC_OPERATION_INIT; + uint8_t sign_mac[PSA_MAC_MAX_SIZE + 10] = { 0 }; + size_t sign_mac_length = 0; + const uint8_t input[] = { 0xbb, 0xbb, 0xbb, 0xbb }; + const uint8_t verify_mac[] = { + 0x74, 0x65, 0x93, 0x8c, 0xeb, 0x1d, 0xb3, 0x76, 0x5a, 0x38, 0xe7, 0xdd, + 0x85, 0xc5, 0xad, 0x4f, 0x07, 0xe7, 0xd5, 0xb2, 0x64, 0xf0, 0x1a, 0x1a, + 0x2c, 0xf9, 0x18, 0xca, 0x59, 0x7e, 0x5d, 0xf6 }; + + PSA_ASSERT( psa_crypto_init( ) ); + PSA_ASSERT( psa_allocate_key( &handle ) ); + psa_key_policy_set_usage( &policy, + PSA_KEY_USAGE_SIGN | PSA_KEY_USAGE_VERIFY, + alg ); + PSA_ASSERT( psa_set_key_policy( handle, &policy ) ); + + PSA_ASSERT( psa_import_key( handle, key_type, + key, sizeof(key) ) ); + + /* Call update without calling setup beforehand. */ + TEST_EQUAL( psa_mac_update( &operation, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call sign finish without calling setup beforehand. */ + TEST_EQUAL( psa_mac_sign_finish( &operation, sign_mac, sizeof( sign_mac ), + &sign_mac_length), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call verify finish without calling setup beforehand. */ + TEST_EQUAL( psa_mac_verify_finish( &operation, + verify_mac, sizeof( verify_mac ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call update after sign finish. */ + PSA_ASSERT( psa_mac_sign_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + PSA_ASSERT( psa_mac_sign_finish( &operation, + sign_mac, sizeof( sign_mac ), + &sign_mac_length ) ); + TEST_EQUAL( psa_mac_update( &operation, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call update after verify finish. */ + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + PSA_ASSERT( psa_mac_verify_finish( &operation, + verify_mac, sizeof( verify_mac ) ) ); + TEST_EQUAL( psa_mac_update( &operation, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call sign finish twice in a row. */ + PSA_ASSERT( psa_mac_sign_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + PSA_ASSERT( psa_mac_sign_finish( &operation, + sign_mac, sizeof( sign_mac ), + &sign_mac_length ) ); + TEST_EQUAL( psa_mac_sign_finish( &operation, + sign_mac, sizeof( sign_mac ), + &sign_mac_length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Call verify finish twice in a row. */ + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + PSA_ASSERT( psa_mac_verify_finish( &operation, + verify_mac, sizeof( verify_mac ) ) ); + TEST_EQUAL( psa_mac_verify_finish( &operation, + verify_mac, sizeof( verify_mac ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Setup sign but try verify. */ + PSA_ASSERT( psa_mac_sign_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + TEST_EQUAL( psa_mac_verify_finish( &operation, + verify_mac, sizeof( verify_mac ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + + /* Setup verify but try sign. */ + PSA_ASSERT( psa_mac_verify_setup( &operation, + handle, alg ) ); + PSA_ASSERT( psa_mac_update( &operation, input, sizeof( input ) ) ); + TEST_EQUAL( psa_mac_sign_finish( &operation, + sign_mac, sizeof( sign_mac ), + &sign_mac_length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + +exit: + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void mac_sign( int key_type_arg, data_t *key, From ab43997f44b03c2299e3c56f168a0b74c60a6d9f Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 15 Feb 2019 14:12:05 +0000 Subject: [PATCH 4/8] psa: Disallow use of invalid cipher contexts Ensure that when doing cipher operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification. --- library/psa_crypto.c | 6 + tests/suites/test_suite_psa_crypto.data | 4 + tests/suites/test_suite_psa_crypto.function | 161 ++++++++++++++++++++ 3 files changed, 171 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9bfe8d28c..4075c658f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -3073,6 +3073,12 @@ psa_status_t psa_cipher_update( psa_cipher_operation_t *operation, psa_status_t status; int ret; size_t expected_output_size; + + if( operation->alg == 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + if( ! PSA_ALG_IS_STREAM_CIPHER( operation->alg ) ) { /* Take the unprocessed partial block left over from previous diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 489389a84..098e3f925 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -926,6 +926,10 @@ depends_on:MBEDTLS_ARC4_C:MBEDTLS_CIPHER_MODE_CTR # Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here cipher_setup:PSA_KEY_TYPE_ARC4:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED +PSA cipher: bad order function calls +depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC +cipher_bad_order: + PSA symmetric encrypt: AES-CBC-nopad, 16 bytes, good depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC cipher_encrypt:PSA_ALG_CBC_NO_PADDING:PSA_KEY_TYPE_AES:"2b7e151628aed2a6abf7158809cf4f3c":"6bc1bee22e409f96e93d7e117393172a":"a076ec9dfbe47d52afc357336f20743b":PSA_SUCCESS diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 37b4d8d69..d1364b923 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2447,6 +2447,9 @@ exit: /* BEGIN_CASE */ void cipher_operation_init( ) { + const uint8_t input[1] = { 0 }; + unsigned char output[1] = { 0 }; + size_t output_length; /* Test each valid way of initializing the object, except for `= {0}`, as * Clang 5 complains when `-Wmissing-field-initializers` is used, even * though it's OK by the C standard. We could test for this, but we'd need @@ -2457,6 +2460,23 @@ void cipher_operation_init( ) memset( &zero, 0, sizeof( zero ) ); + /* A freshly-initialized cipher operation should not be usable. */ + TEST_EQUAL( psa_cipher_update( &func, + input, sizeof( input ), + output, sizeof( output ), + &output_length ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_cipher_update( &init, + input, sizeof( input ), + output, sizeof( output ), + &output_length ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_cipher_update( &zero, + input, sizeof( input ), + output, sizeof( output ), + &output_length ), + PSA_ERROR_BAD_STATE ); + /* A default cipher operation should be abortable without error. */ PSA_ASSERT( psa_cipher_abort( &func ) ); PSA_ASSERT( psa_cipher_abort( &init ) ); @@ -2497,6 +2517,147 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void cipher_bad_order( ) +{ + psa_key_handle_t handle = 0; + psa_key_type_t key_type = PSA_KEY_TYPE_AES; + psa_algorithm_t alg = PSA_ALG_CBC_PKCS7; + psa_key_policy_t policy = PSA_KEY_POLICY_INIT; + psa_cipher_operation_t operation = PSA_CIPHER_OPERATION_INIT; + unsigned char iv[PSA_BLOCK_CIPHER_BLOCK_SIZE(PSA_KEY_TYPE_AES)] = { 0 }; + const uint8_t key[] = { + 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, + 0xaa, 0xaa, 0xaa, 0xaa }; + const uint8_t text[] = { + 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, 0xbb, + 0xbb, 0xbb, 0xbb, 0xbb }; + uint8_t buffer[PSA_BLOCK_CIPHER_BLOCK_SIZE(PSA_KEY_TYPE_AES)] = { 0 }; + size_t length = 0; + + PSA_ASSERT( psa_crypto_init( ) ); + PSA_ASSERT( psa_allocate_key( &handle ) ); + psa_key_policy_set_usage( &policy, + PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT, + alg ); + PSA_ASSERT( psa_set_key_policy( handle, &policy ) ); + PSA_ASSERT( psa_import_key( handle, key_type, + key, sizeof(key) ) ); + + + /* Generate an IV without calling setup beforehand. */ + TEST_EQUAL( psa_cipher_generate_iv( &operation, + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Generate an IV twice in a row. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_generate_iv( &operation, + buffer, sizeof( buffer ), + &length ) ); + TEST_EQUAL( psa_cipher_generate_iv( &operation, + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Generate an IV after it's already set. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ) ); + TEST_EQUAL( psa_cipher_generate_iv( &operation, + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Set an IV without calling setup beforehand. */ + TEST_EQUAL( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Set an IV after it's already set. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ) ); + TEST_EQUAL( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Set an IV after it's already generated. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_generate_iv( &operation, + buffer, sizeof( buffer ), + &length ) ); + TEST_EQUAL( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call update without calling setup beforehand. */ + TEST_EQUAL( psa_cipher_update( &operation, + text, sizeof( text ), + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call update without an IV where an IV is required. */ + TEST_EQUAL( psa_cipher_update( &operation, + text, sizeof( text ), + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call update after finish. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ) ); + PSA_ASSERT( psa_cipher_finish( &operation, + buffer, sizeof( buffer ), &length ) ); + TEST_EQUAL( psa_cipher_update( &operation, + text, sizeof( text ), + buffer, sizeof( buffer ), + &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call finish without calling setup beforehand. */ + TEST_EQUAL( psa_cipher_finish( &operation, + buffer, sizeof( buffer ), &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call finish without an IV where an IV is required. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + /* Not calling update means we are encrypting an empty buffer, which is OK + * for cipher modes with padding. */ + TEST_EQUAL( psa_cipher_finish( &operation, + buffer, sizeof( buffer ), &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call finish twice in a row. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + PSA_ASSERT( psa_cipher_set_iv( &operation, + iv, sizeof( iv ) ) ); + PSA_ASSERT( psa_cipher_finish( &operation, + buffer, sizeof( buffer ), &length ) ); + TEST_EQUAL( psa_cipher_finish( &operation, + buffer, sizeof( buffer ), &length ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + +exit: + mbedtls_psa_crypto_free( ); +} +/* END_CASE */ + /* BEGIN_CASE */ void cipher_encrypt( int alg_arg, int key_type_arg, data_t *key, From a0f625ac9a8e9461c252acd8039f7b5cac3693ab Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 15 Feb 2019 13:52:25 +0000 Subject: [PATCH 5/8] psa: Disallow use of invalid hash contexts If a hash context has not been set up, fail with PSA_ERROR_BAD_STATE as documented in crypto.h and the PSA Crypto specification. --- library/psa_crypto.c | 6 ++---- tests/suites/test_suite_psa_crypto.function | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 4075c658f..1f96ae079 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1502,8 +1502,7 @@ psa_status_t psa_hash_update( psa_hash_operation_t *operation, break; #endif default: - ret = MBEDTLS_ERR_MD_BAD_INPUT_DATA; - break; + return( PSA_ERROR_BAD_STATE ); } if( ret != 0 ) @@ -1575,8 +1574,7 @@ psa_status_t psa_hash_finish( psa_hash_operation_t *operation, break; #endif default: - ret = MBEDTLS_ERR_MD_BAD_INPUT_DATA; - break; + return( PSA_ERROR_BAD_STATE ); } status = mbedtls_to_psa_error( ret ); diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index d1364b923..6eb9b0abb 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1950,6 +1950,7 @@ exit: /* BEGIN_CASE */ void hash_operation_init( ) { + const uint8_t input[1] = { 0 }; /* Test each valid way of initializing the object, except for `= {0}`, as * Clang 5 complains when `-Wmissing-field-initializers` is used, even * though it's OK by the C standard. We could test for this, but we'd need @@ -1960,6 +1961,14 @@ void hash_operation_init( ) memset( &zero, 0, sizeof( zero ) ); + /* A default hash operation should not be usable. */ + TEST_EQUAL( psa_hash_update( &func, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_hash_update( &init, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + TEST_EQUAL( psa_hash_update( &zero, input, sizeof( input ) ), + PSA_ERROR_BAD_STATE ); + /* A default hash operation should be abortable without error. */ PSA_ASSERT( psa_hash_abort( &func ) ); PSA_ASSERT( psa_hash_abort( &init ) ); @@ -2004,18 +2013,18 @@ void hash_bad_order( ) /* psa_hash_update without calling psa_hash_setup beforehand */ memset( &operation, 0, sizeof( operation ) ); TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), - PSA_ERROR_INVALID_ARGUMENT ); + PSA_ERROR_BAD_STATE ); /* psa_hash_verify without calling psa_hash_setup beforehand */ memset( &operation, 0, sizeof( operation ) ); TEST_EQUAL( psa_hash_verify( &operation, hash, sizeof( hash ) ), - PSA_ERROR_INVALID_ARGUMENT ); + PSA_ERROR_BAD_STATE ); /* psa_hash_finish without calling psa_hash_setup beforehand */ memset( &operation, 0, sizeof( operation ) ); TEST_EQUAL( psa_hash_finish( &operation, hash, sizeof( hash ), &hash_len ), - PSA_ERROR_INVALID_ARGUMENT ); + PSA_ERROR_BAD_STATE ); exit: mbedtls_psa_crypto_free( ); From 11aa7ee1891df72cedfd4453fbe40207a6d2f526 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Tue, 19 Feb 2019 11:44:55 +0000 Subject: [PATCH 6/8] psa: Extend hash bad order test Extend hash bad order test in line with the new bad order tests for MAC and cipher, covering more cases and making comments and test layout consistent. Ensure that when doing hash operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification. --- tests/suites/test_suite_psa_crypto.data | 1 + tests/suites/test_suite_psa_crypto.function | 65 ++++++++++++++++++--- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto.data b/tests/suites/test_suite_psa_crypto.data index 098e3f925..635c5bfac 100644 --- a/tests/suites/test_suite_psa_crypto.data +++ b/tests/suites/test_suite_psa_crypto.data @@ -655,6 +655,7 @@ depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C hash_setup:PSA_ALG_HMAC(PSA_ALG_SHA_256):PSA_ERROR_INVALID_ARGUMENT PSA hash: bad order function calls +depends_on:MBEDTLS_MD_C:MBEDTLS_SHA256_C hash_bad_order: PSA hash verify: bad arguments diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 6eb9b0abb..2499102a5 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1961,7 +1961,7 @@ void hash_operation_init( ) memset( &zero, 0, sizeof( zero ) ); - /* A default hash operation should not be usable. */ + /* A freshly-initialized hash operation should not be usable. */ TEST_EQUAL( psa_hash_update( &func, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); TEST_EQUAL( psa_hash_update( &init, input, sizeof( input ) ), @@ -1999,32 +1999,79 @@ exit: /* BEGIN_CASE */ void hash_bad_order( ) { + psa_algorithm_t alg = PSA_ALG_SHA_256; unsigned char input[] = ""; /* SHA-256 hash of an empty string */ - unsigned char hash[] = { + const unsigned char valid_hash[] = { 0xe3, 0xb0, 0xc4, 0x42, 0x98, 0xfc, 0x1c, 0x14, 0x9a, 0xfb, 0xf4, 0xc8, 0x99, 0x6f, 0xb9, 0x24, 0x27, 0xae, 0x41, 0xe4, 0x64, 0x9b, 0x93, 0x4c, 0xa4, 0x95, 0x99, 0x1b, 0x78, 0x52, 0xb8, 0x55 }; + unsigned char hash[sizeof(valid_hash)] = { 0 }; size_t hash_len; psa_hash_operation_t operation = PSA_HASH_OPERATION_INIT; PSA_ASSERT( psa_crypto_init( ) ); - /* psa_hash_update without calling psa_hash_setup beforehand */ - memset( &operation, 0, sizeof( operation ) ); + /* Call update without calling setup beforehand. */ TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); - /* psa_hash_verify without calling psa_hash_setup beforehand */ - memset( &operation, 0, sizeof( operation ) ); - TEST_EQUAL( psa_hash_verify( &operation, hash, sizeof( hash ) ), + /* Call update after finish. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + PSA_ASSERT( psa_hash_finish( &operation, + hash, sizeof( hash ), &hash_len ) ); + TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); - /* psa_hash_finish without calling psa_hash_setup beforehand */ - memset( &operation, 0, sizeof( operation ) ); + /* Call verify without calling setup beforehand. */ + TEST_EQUAL( psa_hash_verify( &operation, + valid_hash, sizeof( valid_hash ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + + /* Call verify after finish. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + PSA_ASSERT( psa_hash_finish( &operation, + hash, sizeof( hash ), &hash_len ) ); + TEST_EQUAL( psa_hash_verify( &operation, + valid_hash, sizeof( valid_hash ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + + /* Call verify twice in a row. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + PSA_ASSERT( psa_hash_verify( &operation, + valid_hash, sizeof( valid_hash ) ) ); + TEST_EQUAL( psa_hash_verify( &operation, + valid_hash, sizeof( valid_hash ) ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + + /* Call finish without calling setup beforehand. */ TEST_EQUAL( psa_hash_finish( &operation, hash, sizeof( hash ), &hash_len ), PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + + /* Call finish twice in a row. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + PSA_ASSERT( psa_hash_finish( &operation, + hash, sizeof( hash ), &hash_len ) ); + TEST_EQUAL( psa_hash_finish( &operation, + hash, sizeof( hash ), &hash_len ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + + /* Call finish after calling verify. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + PSA_ASSERT( psa_hash_verify( &operation, + valid_hash, sizeof( valid_hash ) ) ); + TEST_EQUAL( psa_hash_finish( &operation, + hash, sizeof( hash ), &hash_len ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); exit: mbedtls_psa_crypto_free( ); From 36ee5d0fbfd5941f9ae7f1aa105a3d980235ffd5 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Tue, 19 Feb 2019 09:25:10 +0000 Subject: [PATCH 7/8] psa: Disallow repeated setup Calling psa_*_setup() twice on a MAC, cipher, or hash context should result in a PSA_ERROR_BAD_STATE error because the operation has already been set up. Fixes #10 --- library/psa_crypto.c | 20 +++++++++++++++- tests/suites/test_suite_psa_crypto.function | 26 +++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 1f96ae079..40c676a5d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1379,7 +1379,13 @@ psa_status_t psa_hash_setup( psa_hash_operation_t *operation, psa_algorithm_t alg ) { int ret; - operation->alg = 0; + + /* A context must be freshly initialized before it can be set up. */ + if( operation->alg != 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + switch( alg ) { #if defined(MBEDTLS_MD2_C) @@ -1998,6 +2004,12 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation, unsigned char truncated = PSA_MAC_TRUNCATED_LENGTH( alg ); psa_algorithm_t full_length_alg = PSA_ALG_FULL_LENGTH_MAC( alg ); + /* A context must be freshly initialized before it can be set up. */ + if( operation->alg != 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + status = psa_mac_init( operation, full_length_alg ); if( status != PSA_SUCCESS ) return( status ); @@ -2909,6 +2921,12 @@ static psa_status_t psa_cipher_setup( psa_cipher_operation_t *operation, PSA_KEY_USAGE_ENCRYPT : PSA_KEY_USAGE_DECRYPT ); + /* A context must be freshly initialized before it can be set up. */ + if( operation->alg != 0 ) + { + return( PSA_ERROR_BAD_STATE ); + } + status = psa_cipher_init( operation, alg ); if( status != PSA_SUCCESS ) return( status ); diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 2499102a5..9ea6cc09b 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -2012,6 +2012,12 @@ void hash_bad_order( ) PSA_ASSERT( psa_crypto_init( ) ); + /* Call setup twice in a row. */ + PSA_ASSERT( psa_hash_setup( &operation, alg ) ); + TEST_EQUAL( psa_hash_setup( &operation, alg ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_hash_abort( &operation ) ); + /* Call update without calling setup beforehand. */ TEST_EQUAL( psa_hash_update( &operation, input, sizeof( input ) ), PSA_ERROR_BAD_STATE ); @@ -2336,6 +2342,14 @@ void mac_bad_order( ) PSA_ERROR_BAD_STATE ); PSA_ASSERT( psa_mac_abort( &operation ) ); + /* Call setup twice in a row. */ + PSA_ASSERT( psa_mac_sign_setup( &operation, + handle, alg ) ); + TEST_EQUAL( psa_mac_sign_setup( &operation, + handle, alg ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_mac_abort( &operation ) ); + /* Call update after sign finish. */ PSA_ASSERT( psa_mac_sign_setup( &operation, handle, alg ) ); @@ -2601,6 +2615,18 @@ void cipher_bad_order( ) key, sizeof(key) ) ); + /* Call encrypt setup twice in a row. */ + PSA_ASSERT( psa_cipher_encrypt_setup( &operation, handle, alg ) ); + TEST_EQUAL( psa_cipher_encrypt_setup( &operation, handle, alg ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + + /* Call decrypt setup twice in a row. */ + PSA_ASSERT( psa_cipher_decrypt_setup( &operation, handle, alg ) ); + TEST_EQUAL( psa_cipher_decrypt_setup( &operation, handle, alg ), + PSA_ERROR_BAD_STATE ); + PSA_ASSERT( psa_cipher_abort( &operation ) ); + /* Generate an IV without calling setup beforehand. */ TEST_EQUAL( psa_cipher_generate_iv( &operation, buffer, sizeof( buffer ), From e236c2a13c000c9673be56933d3704f1305174fa Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 20 Feb 2019 15:37:15 +0000 Subject: [PATCH 8/8] psa: Don't abort when operations are invalid In places where we detect a context is in a bad state and there is no sensitive data to clear, simply return PSA_ERROR_BAD_STATE and don't abort on behalf of the application. The application will choose what to do when it gets a bad state error. The motivation for this change is that an application should decide what to do when it misuses the API and encounters a PSA_ERROR_BAD_STATE error. The library should not attempt to abort on behalf of the application, as that may not be the correct thing to do in all circumstances. --- library/psa_crypto.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 40c676a5d..38f50b3b6 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2128,9 +2128,9 @@ psa_status_t psa_mac_update( psa_mac_operation_t *operation, { psa_status_t status = PSA_ERROR_BAD_STATE; if( ! operation->key_set ) - goto cleanup; + return( PSA_ERROR_BAD_STATE ); if( operation->iv_required && ! operation->iv_set ) - goto cleanup; + return( PSA_ERROR_BAD_STATE ); operation->has_input = 1; #if defined(MBEDTLS_CMAC_C) @@ -2153,10 +2153,9 @@ psa_status_t psa_mac_update( psa_mac_operation_t *operation, { /* This shouldn't happen if `operation` was initialized by * a setup function. */ - status = PSA_ERROR_BAD_STATE; + return( PSA_ERROR_BAD_STATE ); } -cleanup: if( status != PSA_SUCCESS ) psa_mac_abort( operation ); return( status ); @@ -2264,13 +2263,11 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation, if( ! operation->is_sign ) { - status = PSA_ERROR_BAD_STATE; - goto cleanup; + return( PSA_ERROR_BAD_STATE ); } status = psa_mac_finish_internal( operation, mac, mac_size ); -cleanup: if( status == PSA_SUCCESS ) { status = psa_mac_abort( operation ); @@ -2298,8 +2295,7 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation, if( operation->is_sign ) { - status = PSA_ERROR_BAD_STATE; - goto cleanup; + return( PSA_ERROR_BAD_STATE ); } if( operation->mac_size != mac_length ) { @@ -3028,8 +3024,7 @@ psa_status_t psa_cipher_generate_iv( psa_cipher_operation_t *operation, int ret; if( operation->iv_set || ! operation->iv_required ) { - status = PSA_ERROR_BAD_STATE; - goto exit; + return( PSA_ERROR_BAD_STATE ); } if( iv_size < operation->iv_size ) { @@ -3061,8 +3056,7 @@ psa_status_t psa_cipher_set_iv( psa_cipher_operation_t *operation, int ret; if( operation->iv_set || ! operation->iv_required ) { - status = PSA_ERROR_BAD_STATE; - goto exit; + return( PSA_ERROR_BAD_STATE ); } if( iv_length != operation->iv_size ) { @@ -3136,13 +3130,11 @@ psa_status_t psa_cipher_finish( psa_cipher_operation_t *operation, if( ! operation->key_set ) { - status = PSA_ERROR_BAD_STATE; - goto error; + return( PSA_ERROR_BAD_STATE ); } if( operation->iv_required && ! operation->iv_set ) { - status = PSA_ERROR_BAD_STATE; - goto error; + return( PSA_ERROR_BAD_STATE ); } if( operation->ctx.cipher.operation == MBEDTLS_ENCRYPT &&