Skip to content

Disallow use of invalid contexts #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 21, 2019
Merged

Disallow use of invalid contexts #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fe96fbe
Initialize PSA Crypto operation contexts
Patater Feb 20, 2019
b281f74
psa: example: Initialize operation contexts
Patater Feb 20, 2019
252ef28
psa: Disallow use of invalid MAC contexts
Patater Feb 15, 2019
ab43997
psa: Disallow use of invalid cipher contexts
Patater Feb 15, 2019
a0f625a
psa: Disallow use of invalid hash contexts
Patater Feb 15, 2019
11aa7ee
psa: Extend hash bad order test
Patater Feb 19, 2019
36ee5d0
psa: Disallow repeated setup
Patater Feb 19, 2019
e236c2a
psa: Don't abort when operations are invalid
Patater Feb 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down
68 changes: 46 additions & 22 deletions library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -1502,8 +1508,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 )
Expand Down Expand Up @@ -1575,8 +1580,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 );

Expand Down Expand Up @@ -2000,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 );
Expand Down Expand Up @@ -2118,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)
Expand All @@ -2143,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 );
Expand Down Expand Up @@ -2238,6 +2247,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. */
Expand All @@ -2249,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 );
Expand All @@ -2276,10 +2288,14 @@ 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;
goto cleanup;
return( PSA_ERROR_BAD_STATE );
}
if( operation->mac_size != mac_length )
{
Expand Down Expand Up @@ -2901,6 +2917,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 );
Expand Down Expand Up @@ -3002,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 )
{
Expand Down Expand Up @@ -3035,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 )
{
Expand All @@ -3063,6 +3083,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
Expand Down Expand Up @@ -3104,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 &&
Expand Down
6 changes: 3 additions & 3 deletions library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using PSA_HASH_OPERATION_INIT, psa_hash_operation_init(); or simply = 0 are all valid ways to do initialisation, right?

What are the advantages of different methods? In particular, why are we using on line 1449 and 1492 psa_hash_operation_init(); and PSA_HASH_OPERATION_INIT here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{0} and INIT (macro) can be used to initialize global variables, but init() (function) can't. {0} is generic but some compilers warn that this implicitly initializes other fields to 0. init() is the only one that's typed, so you'll get an error if you use it in the wrong context. memset to 0 is also defined as equivalent because it allows applications to use e.g. calloc, and in practice it produces the same value as {0} on virtually all platforms.

In most cases, the choice is indifferent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PSA_HASH_OPERATION_INIT is all static, so you can use it for static globals. The inline function can be used where functions can be used, which is a few less places, but it uses the static initializer under-the-hood. = {0} is usually ok, but some compilers will complain about missing field initializers.

I personally prefer using the macro everywhere, but previous authors inside library/ssl_tls.c liked the function. There is no functional difference in this case.

psa_status_t status;
#else
mbedtls_sha256_context sha256;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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" ) );
Expand Down
2 changes: 1 addition & 1 deletion library/x509_crt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down
2 changes: 1 addition & 1 deletion library/x509write_csr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
4 changes: 2 additions & 2 deletions programs/psa/crypto_examples.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Expand Down Expand Up @@ -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 );
Expand Down
9 changes: 9 additions & 0 deletions tests/suites/test_suite_psa_crypto.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -705,6 +706,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"
Expand Down Expand Up @@ -922,6 +927,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
Expand Down
Loading