From a310b41ebecf9724934666941df6d29ae2cbe8c2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 00:51:21 +0100 Subject: [PATCH 01/15] Add null-pointer support information to init/free --- include/mbedtls/pk.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index df3a03c7c..e2529e4ce 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -197,23 +197,35 @@ typedef size_t (*mbedtls_pk_rsa_alt_key_len_func)( void *ctx ); const mbedtls_pk_info_t *mbedtls_pk_info_from_type( mbedtls_pk_type_t pk_type ); /** - * \brief Initialize a mbedtls_pk_context (as NONE) + * \brief Initialize a #mbedtls_pk_context (as NONE). + * + * \param ctx The context to initialize. + * This must not be \c NULL. */ void mbedtls_pk_init( mbedtls_pk_context *ctx ); /** - * \brief Free a mbedtls_pk_context + * \brief Free the components of a #mbedtls_pk_context. + * + * \param ctx The context to clear. + * If this is \c NULL, this function does nothing. */ void mbedtls_pk_free( mbedtls_pk_context *ctx ); #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) /** * \brief Initialize a restart context + * + * \param ctx The context to initialize. + * This must not be \c NULL. */ void mbedtls_pk_restart_init( mbedtls_pk_restart_ctx *ctx ); /** * \brief Free the components of a restart context + * + * \param ctx The context to clear. + * If this is \c NULL, this function does nothing. */ void mbedtls_pk_restart_free( mbedtls_pk_restart_ctx *ctx ); #endif /* MBEDTLS_ECDSA_C && MBEDTLS_ECP_RESTARTABLE */ From e97dc60b42481b77dbb2f1ae2fdae4a22fc385c1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 00:51:38 +0100 Subject: [PATCH 02/15] Implement parameter validation in pk, pkparse and pkwrite Add checks for null pointers under MBEDTLS_CHECK_PARAMS. In functions that perform operations with a context, only check if the context pointer is non-null under MBEDTLS_CHECK_PARAMS. In the default configuration, unconditionally dereference the context pointer. In functions that query a context, support NULL as a pointer-to-context argument, and return the same value as for a context which has been initialized but not set up. --- library/pk.c | 62 ++++++++++++++++++++++++++++++++++++----------- library/pkparse.c | 33 +++++++++++++++++++++++-- library/pkwrite.c | 28 ++++++++++++++++++++- 3 files changed, 106 insertions(+), 17 deletions(-) diff --git a/library/pk.c b/library/pk.c index e0e8dbad2..d8bce8f46 100644 --- a/library/pk.c +++ b/library/pk.c @@ -44,13 +44,18 @@ #include #include +/* Parameter validation macros based on platform_util.h */ +#define PK_VALIDATE_RET( cond ) \ + MBEDTLS_INTERNAL_VALIDATE_RET( cond, MBEDTLS_ERR_PK_BAD_INPUT_DATA ) +#define PK_VALIDATE( cond ) \ + MBEDTLS_INTERNAL_VALIDATE( cond ) + /* * Initialise a mbedtls_pk_context */ void mbedtls_pk_init( mbedtls_pk_context *ctx ) { - if( ctx == NULL ) - return; + PK_VALIDATE( ctx != NULL ); ctx->pk_info = NULL; ctx->pk_ctx = NULL; @@ -75,6 +80,7 @@ void mbedtls_pk_free( mbedtls_pk_context *ctx ) */ void mbedtls_pk_restart_init( mbedtls_pk_restart_ctx *ctx ) { + PK_VALIDATE( ctx != NULL ); ctx->pk_info = NULL; ctx->rs_ctx = NULL; } @@ -84,7 +90,8 @@ void mbedtls_pk_restart_init( mbedtls_pk_restart_ctx *ctx ) */ void mbedtls_pk_restart_free( mbedtls_pk_restart_ctx *ctx ) { - if( ctx == NULL || ctx->pk_info == NULL || + PK_VALIDATE( ctx != NULL ); + if( ctx->pk_info == NULL || ctx->pk_info->rs_free_func == NULL ) { return; @@ -128,7 +135,8 @@ const mbedtls_pk_info_t * mbedtls_pk_info_from_type( mbedtls_pk_type_t pk_type ) */ int mbedtls_pk_setup( mbedtls_pk_context *ctx, const mbedtls_pk_info_t *info ) { - if( ctx == NULL || info == NULL || ctx->pk_info != NULL ) + PK_VALIDATE_RET( ctx != NULL ); + if( info == NULL || ctx->pk_info != NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ( ctx->pk_ctx = info->ctx_alloc_func() ) == NULL ) @@ -151,7 +159,8 @@ int mbedtls_pk_setup_rsa_alt( mbedtls_pk_context *ctx, void * key, mbedtls_rsa_alt_context *rsa_alt; const mbedtls_pk_info_t *info = &mbedtls_rsa_alt_info; - if( ctx == NULL || ctx->pk_info != NULL ) + PK_VALIDATE_RET( ctx != NULL ); + if( ctx->pk_info != NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ( ctx->pk_ctx = info->ctx_alloc_func() ) == NULL ) @@ -175,7 +184,6 @@ int mbedtls_pk_setup_rsa_alt( mbedtls_pk_context *ctx, void * key, */ int mbedtls_pk_can_do( const mbedtls_pk_context *ctx, mbedtls_pk_type_t type ) { - /* null or NONE context can't do anything */ if( ctx == NULL || ctx->pk_info == NULL ) return( 0 ); @@ -232,7 +240,11 @@ int mbedtls_pk_verify_restartable( mbedtls_pk_context *ctx, const unsigned char *sig, size_t sig_len, mbedtls_pk_restart_ctx *rs_ctx ) { - if( ctx == NULL || ctx->pk_info == NULL || + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( sig != NULL ); + + if( ctx->pk_info == NULL || pk_hashlen_helper( md_alg, &hash_len ) != 0 ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -285,7 +297,11 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, const unsigned char *hash, size_t hash_len, const unsigned char *sig, size_t sig_len ) { - if( ctx == NULL || ctx->pk_info == NULL ) + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( sig != NULL ); + + if( ctx->pk_info == NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ! mbedtls_pk_can_do( ctx, type ) ) @@ -345,7 +361,11 @@ int mbedtls_pk_sign_restartable( mbedtls_pk_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng, mbedtls_pk_restart_ctx *rs_ctx ) { - if( ctx == NULL || ctx->pk_info == NULL || + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( sig != NULL ); + + if( ctx->pk_info == NULL || pk_hashlen_helper( md_alg, &hash_len ) != 0 ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -399,7 +419,12 @@ int mbedtls_pk_decrypt( mbedtls_pk_context *ctx, unsigned char *output, size_t *olen, size_t osize, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - if( ctx == NULL || ctx->pk_info == NULL ) + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( input != NULL || ilen == 0 ); + PK_VALIDATE_RET( output != NULL || osize == 0 ); + PK_VALIDATE_RET( olen != NULL ); + + if( ctx->pk_info == NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ctx->pk_info->decrypt_func == NULL ) @@ -417,7 +442,12 @@ int mbedtls_pk_encrypt( mbedtls_pk_context *ctx, unsigned char *output, size_t *olen, size_t osize, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - if( ctx == NULL || ctx->pk_info == NULL ) + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( input != NULL || ilen == 0 ); + PK_VALIDATE_RET( output != NULL || osize == 0 ); + PK_VALIDATE_RET( olen != NULL ); + + if( ctx->pk_info == NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ctx->pk_info->encrypt_func == NULL ) @@ -432,8 +462,11 @@ int mbedtls_pk_encrypt( mbedtls_pk_context *ctx, */ int mbedtls_pk_check_pair( const mbedtls_pk_context *pub, const mbedtls_pk_context *prv ) { - if( pub == NULL || pub->pk_info == NULL || - prv == NULL || prv->pk_info == NULL || + PK_VALIDATE_RET( pub != NULL ); + PK_VALIDATE_RET( prv != NULL ); + + if( pub->pk_info == NULL || + prv->pk_info == NULL || prv->pk_info->check_pair_func == NULL ) { return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -469,7 +502,8 @@ size_t mbedtls_pk_get_bitlen( const mbedtls_pk_context *ctx ) */ int mbedtls_pk_debug( const mbedtls_pk_context *ctx, mbedtls_pk_debug_item *items ) { - if( ctx == NULL || ctx->pk_info == NULL ) + PK_VALIDATE_RET( ctx != NULL ); + if( ctx->pk_info == NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); if( ctx->pk_info->debug_func == NULL ) diff --git a/library/pkparse.c b/library/pkparse.c index 86d9fb004..7c14e34ec 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -61,6 +61,12 @@ #define mbedtls_free free #endif +/* Parameter validation macros based on platform_util.h */ +#define PK_VALIDATE_RET( cond ) \ + MBEDTLS_INTERNAL_VALIDATE_RET( cond, MBEDTLS_ERR_PK_BAD_INPUT_DATA ) +#define PK_VALIDATE( cond ) \ + MBEDTLS_INTERNAL_VALIDATE( cond ) + #if defined(MBEDTLS_FS_IO) /* * Load all data from a file into a given buffer. @@ -74,6 +80,10 @@ int mbedtls_pk_load_file( const char *path, unsigned char **buf, size_t *n ) FILE *f; long size; + PK_VALIDATE_RET( path != NULL ); + PK_VALIDATE_RET( buf != NULL ); + PK_VALIDATE_RET( n != NULL ); + if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_PK_FILE_IO_ERROR ); @@ -124,6 +134,8 @@ int mbedtls_pk_parse_keyfile( mbedtls_pk_context *ctx, size_t n; unsigned char *buf; + PK_VALIDATE_RET( ctx != NULL ); + if( ( ret = mbedtls_pk_load_file( path, &buf, &n ) ) != 0 ) return( ret ); @@ -148,6 +160,8 @@ int mbedtls_pk_parse_public_keyfile( mbedtls_pk_context *ctx, const char *path ) size_t n; unsigned char *buf; + PK_VALIDATE_RET( ctx != NULL ); + if( ( ret = mbedtls_pk_load_file( path, &buf, &n ) ) != 0 ) return( ret ); @@ -605,6 +619,11 @@ int mbedtls_pk_parse_subpubkey( unsigned char **p, const unsigned char *end, mbedtls_pk_type_t pk_alg = MBEDTLS_PK_NONE; const mbedtls_pk_info_t *pk_info; + PK_VALIDATE_RET( p != NULL ); + PK_VALIDATE_RET( *p != NULL ); + PK_VALIDATE_RET( end != NULL ); + PK_VALIDATE_RET( pk != NULL ); + if( ( ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 ) { @@ -1145,12 +1164,17 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, { int ret; const mbedtls_pk_info_t *pk_info; - #if defined(MBEDTLS_PEM_PARSE_C) size_t len; mbedtls_pem_context pem; +#endif - mbedtls_pem_init( &pem ); + PK_VALIDATE_RET( pk != NULL ); + PK_VALIDATE_RET( key != NULL || keylen == 0 ); + PK_VALIDATE_RET( pwd != NULL || pwdlen == 0 ); + +#if defined(MBEDTLS_PEM_PARSE_C) + mbedtls_pem_init( &pem ); #if defined(MBEDTLS_RSA_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ @@ -1360,7 +1384,12 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, #if defined(MBEDTLS_PEM_PARSE_C) size_t len; mbedtls_pem_context pem; +#endif + PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( key != NULL || keylen == 0 ); + +#if defined(MBEDTLS_PEM_PARSE_C) mbedtls_pem_init( &pem ); #if defined(MBEDTLS_RSA_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ diff --git a/library/pkwrite.c b/library/pkwrite.c index 8eabd889b..51d0c56f1 100644 --- a/library/pkwrite.c +++ b/library/pkwrite.c @@ -30,6 +30,7 @@ #include "mbedtls/pk.h" #include "mbedtls/asn1write.h" #include "mbedtls/oid.h" +#include "mbedtls/platform_util.h" #include @@ -54,6 +55,12 @@ #define mbedtls_free free #endif +/* Parameter validation macros based on platform_util.h */ +#define PK_VALIDATE_RET( cond ) \ + MBEDTLS_INTERNAL_VALIDATE_RET( cond, MBEDTLS_ERR_PK_BAD_INPUT_DATA ) +#define PK_VALIDATE( cond ) \ + MBEDTLS_INTERNAL_VALIDATE( cond ) + #if defined(MBEDTLS_RSA_C) /* * RSAPublicKey ::= SEQUENCE { @@ -151,6 +158,11 @@ int mbedtls_pk_write_pubkey( unsigned char **p, unsigned char *start, int ret; size_t len = 0; + PK_VALIDATE_RET( p != NULL ); + PK_VALIDATE_RET( *p != NULL ); + PK_VALIDATE_RET( start != NULL ); + PK_VALIDATE_RET( key != NULL ); + #if defined(MBEDTLS_RSA_C) if( mbedtls_pk_get_type( key ) == MBEDTLS_PK_RSA ) MBEDTLS_ASN1_CHK_ADD( len, pk_write_rsa_pubkey( p, start, mbedtls_pk_rsa( *key ) ) ); @@ -173,6 +185,9 @@ int mbedtls_pk_write_pubkey_der( mbedtls_pk_context *key, unsigned char *buf, si size_t len = 0, par_len = 0, oid_len; const char *oid; + PK_VALIDATE_RET( key != NULL ); + PK_VALIDATE_RET( buf != NULL || size == 0 ); + c = buf + size; MBEDTLS_ASN1_CHK_ADD( len, mbedtls_pk_write_pubkey( &c, buf, key ) ); @@ -217,9 +232,14 @@ int mbedtls_pk_write_pubkey_der( mbedtls_pk_context *key, unsigned char *buf, si int mbedtls_pk_write_key_der( mbedtls_pk_context *key, unsigned char *buf, size_t size ) { int ret; - unsigned char *c = buf + size; + unsigned char *c; size_t len = 0; + PK_VALIDATE_RET( key != NULL ); + PK_VALIDATE_RET( buf != NULL || size == 0 ); + + c = buf + size; + #if defined(MBEDTLS_RSA_C) if( mbedtls_pk_get_type( key ) == MBEDTLS_PK_RSA ) { @@ -457,6 +477,9 @@ int mbedtls_pk_write_pubkey_pem( mbedtls_pk_context *key, unsigned char *buf, si unsigned char output_buf[PUB_DER_MAX_BYTES]; size_t olen = 0; + PK_VALIDATE_RET( key != NULL ); + PK_VALIDATE_RET( buf != NULL || size == 0 ); + if( ( ret = mbedtls_pk_write_pubkey_der( key, output_buf, sizeof(output_buf) ) ) < 0 ) { @@ -480,6 +503,9 @@ int mbedtls_pk_write_key_pem( mbedtls_pk_context *key, unsigned char *buf, size_ const char *begin, *end; size_t olen = 0; + PK_VALIDATE_RET( key != NULL ); + PK_VALIDATE_RET( buf != NULL || size == 0 ); + if( ( ret = mbedtls_pk_write_key_der( key, output_buf, sizeof(output_buf) ) ) < 0 ) return( ret ); From 78438e410911a9453eaecad5727f44a335568565 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 00:55:07 +0100 Subject: [PATCH 03/15] Test parameter validation for pk, pkparse and pkwrite --- tests/suites/test_suite_pk.data | 3 + tests/suites/test_suite_pk.function | 316 ++++++++++++++++++++++++++++ 2 files changed, 319 insertions(+) diff --git a/tests/suites/test_suite_pk.data b/tests/suites/test_suite_pk.data index 478cde7be..d029b995f 100644 --- a/tests/suites/test_suite_pk.data +++ b/tests/suites/test_suite_pk.data @@ -1,3 +1,6 @@ +PK invalid parameters +invalid_parameters: + PK utils: RSA depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME pk_utils:MBEDTLS_PK_RSA:512:64:"RSA" diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 4813f71f7..b2f9a9942 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -69,6 +69,322 @@ size_t mbedtls_rsa_key_len_func( void *ctx ) * END_DEPENDENCIES */ +/* BEGIN_CASE */ +void valid_parameters( ) +{ + mbedtls_pk_context pk; + size_t len; + + mbedtls_pk_init( &pk ); + + TEST_VALID_PARAM( mbedtls_pk_setup( &pk, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_VALID_PARAM( mbedtls_pk_get_bitlen( NULL ) == 0 ); + + TEST_VALID_PARAM( mbedtls_pk_get_len( NULL ) == 0 ); + + TEST_VALID_PARAM( mbedtls_pk_can_do( NULL, MBEDTLS_PK_NONE ) == 0 ); + + TEST_VALID_PARAM( mbedtls_pk_encrypt( &pk, + NULL, 0, + NULL, &len, 0, + rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_VALID_PARAM( mbedtls_pk_decrypt( &pk, + NULL, 0, + NULL, &len, 0, + rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_PK_PARSE_C) + TEST_VALID_PARAM( mbedtls_pk_parse_key( &pk, NULL, 0, NULL, 1 ) == + MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + + TEST_VALID_PARAM( mbedtls_pk_parse_public_key( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); +#endif /* MBEDTLS_PK_PARSE_C */ + +#if defined(MBEDTLS_PK_WRITE_C) + TEST_VALID_PARAM( mbedtls_pk_write_key_der( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); + + TEST_VALID_PARAM( mbedtls_pk_write_pubkey_der( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); +#endif /* MBEDTLS_PK_WRITE_C */ +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +void invalid_parameters( ) +{ + size_t len; + unsigned char *null_buf = NULL; + unsigned char buf[1]; + unsigned char *p = buf; + char str[1] = {0}; + mbedtls_pk_context pk; + void *options = buf; + + mbedtls_pk_init( &pk ); + + TEST_INVALID_PARAM( mbedtls_pk_init( NULL ) ); + +#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) + TEST_INVALID_PARAM( mbedtls_pk_restart_init( NULL ) ); + + TEST_INVALID_PARAM( mbedtls_pk_restart_free( NULL ) ); +#endif + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_setup( NULL, NULL ) ); + +#if defined(MBEDTLS_PK_RSA_ALT_SUPPORT) + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_setup_rsa_alt( NULL, buf, + NULL, NULL, NULL ) ); +#endif + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_restartable( NULL, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + buf, sizeof( buf ), + NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_restartable( &pk, + MBEDTLS_MD_NONE, + NULL, sizeof( buf ), + buf, sizeof( buf ), + NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_restartable( &pk, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + NULL, sizeof( buf ), + NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify( NULL, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify( &pk, + MBEDTLS_MD_NONE, + NULL, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify( &pk, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + NULL, sizeof( buf ) ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, + NULL, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, + &pk, + MBEDTLS_MD_NONE, + NULL, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, + &pk, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + NULL, sizeof( buf ) ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign_restartable( NULL, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + buf, &len, + rnd_std_rand, NULL, + NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign_restartable( &pk, + MBEDTLS_MD_NONE, + NULL, sizeof( buf ), + buf, &len, + rnd_std_rand, NULL, + NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign_restartable( &pk, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + NULL, &len, + rnd_std_rand, NULL, + NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign( NULL, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + buf, &len, + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign( &pk, + MBEDTLS_MD_NONE, + NULL, sizeof( buf ), + buf, &len, + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign( &pk, + MBEDTLS_MD_NONE, + buf, sizeof( buf ), + NULL, &len, + rnd_std_rand, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_decrypt( NULL, + buf, sizeof( buf ), + buf, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_decrypt( &pk, + NULL, sizeof( buf ), + buf, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_decrypt( &pk, + buf, sizeof( buf ), + NULL, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_decrypt( &pk, + buf, sizeof( buf ), + buf, NULL, sizeof( buf ), + rnd_std_rand, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_encrypt( NULL, + buf, sizeof( buf ), + buf, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_encrypt( &pk, + NULL, sizeof( buf ), + buf, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_encrypt( &pk, + buf, sizeof( buf ), + NULL, &len, sizeof( buf ), + rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_encrypt( &pk, + buf, sizeof( buf ), + buf, NULL, sizeof( buf ), + rnd_std_rand, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_check_pair( NULL, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_check_pair( &pk, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_debug( NULL, NULL ) ); + +#if defined(MBEDTLS_PK_PARSE_C) + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_load_file( NULL, &p, &len ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_load_file( str, NULL, &len ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_load_file( str, &p, NULL ) ); + +#if defined(MBEDTLS_FS_IO) + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_keyfile( NULL, str, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_keyfile( &pk, NULL, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_public_keyfile( NULL, str ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_public_keyfile( &pk, NULL ) ); +#endif + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_subpubkey( NULL, buf, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_subpubkey( &null_buf, buf, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_subpubkey( &p, NULL, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_subpubkey( &p, buf, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_key( NULL, + buf, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_key( &pk, + NULL, sizeof( buf ), + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_key( &pk, + buf, sizeof( buf ), + NULL, sizeof( buf ) ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_public_key( NULL, + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_parse_public_key( &pk, + NULL, sizeof( buf ) ) ); +#endif /* MBEDTLS_PK_PARSE_C */ + +#if defined(MBEDTLS_PK_WRITE_C) + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey( NULL, p, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey( &null_buf, p, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey( &p, NULL, &pk ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey( &p, p, NULL ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey_der( NULL, + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey_der( &pk, + NULL, sizeof( buf ) ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_key_der( NULL, + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_key_der( &pk, + NULL, sizeof( buf ) ) ); + +#if defined(MBEDTLS_PEM_WRITE_C) + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey_pem( NULL, + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_pubkey_pem( &pk, + NULL, sizeof( buf ) ) ); + + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_key_pem( NULL, + buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_write_key_pem( &pk, + NULL, sizeof( buf ) ) ); +#endif /* MBEDTLS_PEM_WRITE_C */ + +#endif /* MBEDTLS_PK_WRITE_C */ +} +/* END_CASE */ + /* BEGIN_CASE */ void pk_utils( int type, int size, int len, char * name ) { From e146e7dbaeceaa62b3d8258f2433819c2dfc32ca Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 13:20:21 +0100 Subject: [PATCH 04/15] Don't use TEST_VALID_PARAM with a value TEST_VALID_PARAM is only for functions that return void. This commit fixes the build with clang -Wunused-comparison. --- tests/suites/test_suite_pk.function | 46 ++++++++++++++--------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index b2f9a9942..936bcdfb2 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -77,41 +77,41 @@ void valid_parameters( ) mbedtls_pk_init( &pk ); - TEST_VALID_PARAM( mbedtls_pk_setup( &pk, NULL ) == - MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + TEST_ASSERT( mbedtls_pk_setup( &pk, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); - TEST_VALID_PARAM( mbedtls_pk_get_bitlen( NULL ) == 0 ); + TEST_ASSERT( mbedtls_pk_get_bitlen( NULL ) == 0 ); - TEST_VALID_PARAM( mbedtls_pk_get_len( NULL ) == 0 ); + TEST_ASSERT( mbedtls_pk_get_len( NULL ) == 0 ); - TEST_VALID_PARAM( mbedtls_pk_can_do( NULL, MBEDTLS_PK_NONE ) == 0 ); + TEST_ASSERT( mbedtls_pk_can_do( NULL, MBEDTLS_PK_NONE ) == 0 ); - TEST_VALID_PARAM( mbedtls_pk_encrypt( &pk, - NULL, 0, - NULL, &len, 0, - rnd_std_rand, NULL ) == - MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + TEST_ASSERT( mbedtls_pk_encrypt( &pk, + NULL, 0, + NULL, &len, 0, + rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); - TEST_VALID_PARAM( mbedtls_pk_decrypt( &pk, - NULL, 0, - NULL, &len, 0, - rnd_std_rand, NULL ) == - MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + TEST_ASSERT( mbedtls_pk_decrypt( &pk, + NULL, 0, + NULL, &len, 0, + rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); #if defined(MBEDTLS_PK_PARSE_C) - TEST_VALID_PARAM( mbedtls_pk_parse_key( &pk, NULL, 0, NULL, 1 ) == - MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + TEST_ASSERT( mbedtls_pk_parse_key( &pk, NULL, 0, NULL, 1 ) == + MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); - TEST_VALID_PARAM( mbedtls_pk_parse_public_key( &pk, NULL, 0 ) == - MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + TEST_ASSERT( mbedtls_pk_parse_public_key( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); #endif /* MBEDTLS_PK_PARSE_C */ #if defined(MBEDTLS_PK_WRITE_C) - TEST_VALID_PARAM( mbedtls_pk_write_key_der( &pk, NULL, 0 ) == - MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); + TEST_ASSERT( mbedtls_pk_write_key_der( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); - TEST_VALID_PARAM( mbedtls_pk_write_pubkey_der( &pk, NULL, 0 ) == - MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); + TEST_ASSERT( mbedtls_pk_write_pubkey_der( &pk, NULL, 0 ) == + MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); #endif /* MBEDTLS_PK_WRITE_C */ } /* END_CASE */ From 1f19fa6f62346d57d5dac41512cc107c8a7fc7fb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 14:18:39 +0100 Subject: [PATCH 05/15] PK: Fix free(NULL) in library and tests free() functions are documented as no-ops on NULL. Implement and test this correctly. --- library/pk.c | 3 +-- tests/suites/test_suite_pk.function | 8 ++++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/library/pk.c b/library/pk.c index d8bce8f46..38ab7747c 100644 --- a/library/pk.c +++ b/library/pk.c @@ -90,8 +90,7 @@ void mbedtls_pk_restart_init( mbedtls_pk_restart_ctx *ctx ) */ void mbedtls_pk_restart_free( mbedtls_pk_restart_ctx *ctx ) { - PK_VALIDATE( ctx != NULL ); - if( ctx->pk_info == NULL || + if( ctx == NULL || ctx->pk_info == NULL || ctx->pk_info->rs_free_func == NULL ) { return; diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 936bcdfb2..b8069b540 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -77,6 +77,12 @@ void valid_parameters( ) mbedtls_pk_init( &pk ); + TEST_VALID_PARAM( mbedtls_pk_free( NULL ) ); + +#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) + TEST_VALID_PARAM( mbedtls_pk_restart_free( NULL ) ); +#endif + TEST_ASSERT( mbedtls_pk_setup( &pk, NULL ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -133,8 +139,6 @@ void invalid_parameters( ) #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) TEST_INVALID_PARAM( mbedtls_pk_restart_init( NULL ) ); - - TEST_INVALID_PARAM( mbedtls_pk_restart_free( NULL ) ); #endif TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, From 159171b72ac7080c3692587565b19142ac9fd32f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:03:28 +0100 Subject: [PATCH 06/15] PK parse/write: support keylen=0 correctly A 0-length buffer for the key is a legitimate edge case. Ensure that it works, even with buf=NULL. Document the key and keylen parameters. There are already test cases for parsing an empty buffer. A subsequent commit will add tests for writing to an empty buffer. --- include/mbedtls/pk.h | 34 ++++++++++++++++++++++------------ library/pkparse.c | 22 +++++++++++----------- library/pkwrite.c | 8 ++++++-- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index e2529e4ce..310aeef5f 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -530,9 +530,13 @@ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx ); * \brief Parse a private key in PEM or DER format * * \param ctx key to be initialized - * \param key input buffer - * \param keylen size of the buffer - * (including the terminating null byte for PEM data) + * \param key Input buffer to parse. + * The buffer must contain the input exactly, with no + * extra trailing material. For PEM, the buffer must + * contain a null-terminated string. + * \param keylen Size of \b key in bytes. + * For PEM data, this includes the terminating null byte, + * so \p keylen must be equal to `strlen(key) + 1`. * \param pwd password for decryption (optional) * \param pwdlen size of the password * @@ -553,9 +557,13 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *ctx, * \brief Parse a public key in PEM or DER format * * \param ctx key to be initialized - * \param key input buffer - * \param keylen size of the buffer - * (including the terminating null byte for PEM data) + * \param key Input buffer to parse. + * The buffer must contain the input exactly, with no + * extra trailing material. For PEM, the buffer must + * contain a null-terminated string. + * \param keylen Size of \b key in bytes. + * For PEM data, this includes the terminating null byte, + * so \p keylen must be equal to `strlen(key) + 1`. * * \note On entry, ctx must be empty, either freshly initialised * with mbedtls_pk_init() or reset with mbedtls_pk_free(). If you need a @@ -643,9 +651,10 @@ int mbedtls_pk_write_pubkey_der( mbedtls_pk_context *ctx, unsigned char *buf, si /** * \brief Write a public key to a PEM string * - * \param ctx public key to write away - * \param buf buffer to write to - * \param size size of the buffer + * \param ctx Context containing the public key to write. + * \param buf Buffer to write to. The output includes a + * terminating null byte. + * \param size Size of the buffer in bytes. * * \return 0 if successful, or a specific error code */ @@ -654,9 +663,10 @@ int mbedtls_pk_write_pubkey_pem( mbedtls_pk_context *ctx, unsigned char *buf, si /** * \brief Write a private key to a PKCS#1 or SEC1 PEM string * - * \param ctx private to write away - * \param buf buffer to write to - * \param size size of the buffer + * \param ctx Context containing the private key to write. + * \param buf Buffer to write to. The output includes a + * terminating null byte. + * \param size Size of the buffer in bytes. * * \return 0 if successful, or a specific error code */ diff --git a/library/pkparse.c b/library/pkparse.c index 7c14e34ec..127f9b840 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -1170,15 +1170,16 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, #endif PK_VALIDATE_RET( pk != NULL ); - PK_VALIDATE_RET( key != NULL || keylen == 0 ); - PK_VALIDATE_RET( pwd != NULL || pwdlen == 0 ); + if( keylen == 0 ) + return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + PK_VALIDATE_RET( key != NULL ); #if defined(MBEDTLS_PEM_PARSE_C) mbedtls_pem_init( &pem ); #if defined(MBEDTLS_RSA_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, @@ -1209,7 +1210,7 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, #if defined(MBEDTLS_ECP_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, @@ -1239,7 +1240,7 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, #endif /* MBEDTLS_ECP_C */ /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, @@ -1262,7 +1263,7 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, #if defined(MBEDTLS_PKCS12_C) || defined(MBEDTLS_PKCS5_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, @@ -1300,9 +1301,6 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk, { unsigned char *key_copy; - if( keylen == 0 ) - return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); - if( ( key_copy = mbedtls_calloc( 1, keylen ) ) == NULL ) return( MBEDTLS_ERR_PK_ALLOC_FAILED ); @@ -1387,13 +1385,15 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, #endif PK_VALIDATE_RET( ctx != NULL ); + if( keylen == 0 ) + return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); PK_VALIDATE_RET( key != NULL || keylen == 0 ); #if defined(MBEDTLS_PEM_PARSE_C) mbedtls_pem_init( &pem ); #if defined(MBEDTLS_RSA_C) /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, @@ -1424,7 +1424,7 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, #endif /* MBEDTLS_RSA_C */ /* Avoid calling mbedtls_pem_read_buffer() on non-null-terminated string */ - if( keylen == 0 || key[keylen - 1] != '\0' ) + if( key[keylen - 1] != '\0' ) ret = MBEDTLS_ERR_PEM_NO_HEADER_FOOTER_PRESENT; else ret = mbedtls_pem_read_buffer( &pem, diff --git a/library/pkwrite.c b/library/pkwrite.c index 51d0c56f1..8d1da2f75 100644 --- a/library/pkwrite.c +++ b/library/pkwrite.c @@ -186,7 +186,9 @@ int mbedtls_pk_write_pubkey_der( mbedtls_pk_context *key, unsigned char *buf, si const char *oid; PK_VALIDATE_RET( key != NULL ); - PK_VALIDATE_RET( buf != NULL || size == 0 ); + if( size == 0 ) + return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + PK_VALIDATE_RET( buf != NULL ); c = buf + size; @@ -236,7 +238,9 @@ int mbedtls_pk_write_key_der( mbedtls_pk_context *key, unsigned char *buf, size_ size_t len = 0; PK_VALIDATE_RET( key != NULL ); - PK_VALIDATE_RET( buf != NULL || size == 0 ); + if( size == 0 ) + return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + PK_VALIDATE_RET( buf != NULL ); c = buf + size; From cc274c2ebf784290533b04be16a72045a28ecdfe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:08:01 +0100 Subject: [PATCH 07/15] Do run the valid parameters test function --- tests/suites/test_suite_pk.data | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_pk.data b/tests/suites/test_suite_pk.data index d029b995f..0da32418a 100644 --- a/tests/suites/test_suite_pk.data +++ b/tests/suites/test_suite_pk.data @@ -1,6 +1,9 @@ PK invalid parameters invalid_parameters: +PK valid parameters +valid_parameters: + PK utils: RSA depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME pk_utils:MBEDTLS_PK_RSA:512:64:"RSA" From 998fbfbe6872e4d163d02d81f4c3966e86eb5262 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:08:51 +0100 Subject: [PATCH 08/15] Properly test pk_write with an empty output buffer This needs a real key to test properly. --- tests/suites/test_suite_pk.data | 4 ++++ tests/suites/test_suite_pk.function | 32 +++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_pk.data b/tests/suites/test_suite_pk.data index 0da32418a..e41dfa710 100644 --- a/tests/suites/test_suite_pk.data +++ b/tests/suites/test_suite_pk.data @@ -4,6 +4,10 @@ invalid_parameters: PK valid parameters valid_parameters: +PK write valid parameters +depends_on:MBEDTLS_RSA_C +valid_parameters_pkwrite:"308204a20201000282010100a9021f3d406ad555538bfd36ee82652e15615e89bfb8e84590dbee881652d3f143504796125964876bfd2be046f973beddcf92e1915bed66a06f8929794580d0836ad54143775f397c09044782b0573970eda3ec15191ea8330847c10542a9fd4cc3b4dfdd061f4d1051406773130f40f86d81255f0ab153c6307e1539acf95aee7f929ea6055be7139785b52392d9d42406d50925897507dda61a8f3f0919bead652c64eb959bdcfe415e17a6da6c5b69cc02ba142c16249c4adccdd0f7526773f12da023fd7ef431ca2d70ca890b04db2ea64f706e9ecebd5889e253599e6e5a9265e2883f0c9419a3dde5e89d9513ed29dbab7012dc5aca6b17ab528254b10203010001028201001689f5e89142ae18a6ffb0513715a4b0b4a13b9e5b3729a2bd62d738c6e15cea7bf3a4d85ab2193a0628c9452bb1f0c1af8b132789df1c95e72778bf5330f5b0d915d242d5e0818e85001ed5fa93d1ce13455deb0a15438562e8e3c8d60ec1e4c9ebff9f2b36b9cde9332cc79f0d17a7ae79cc1353cd75409ad9b4b6d7ee3d82af6f3207656cf2ac98947c15c398db0cebf8dc3eef5398269480cdd09411b960273ae3f364da09af849f24aa87346c58618ea91d9d6cd1d3932c80dbfc1f0a4166a9036911999ca27761079f0ce02db02c1c909ff9b4278578d7bb1b54b2b7082fc9e864b6b394e331c0d11a9a68255565b6dd477f4119c5809839520700711102818100d7db987ad86de6a9b0749fb5da80bacde3bebd72dcc83f60a27db74f927ac3661386577bfce5b4a00ad024682401d6aad29713c8e223b53415305ca07559821099b187fdd1bad3dc4dec9da96f5fa6128331e8f7d89f1e1a788698d1a27256dc7cd392f04e531a9e38e7265bf4fd7eec01e7835e9b1a0dd8923e440381be1c2702818100c87025fff7a493c623404966fbc8b32ed164ca620ad1a0ad11ef42fd12118456017856a8b42e5d4ad36104e9dc9f8a2f3003c3957ffddb20e2f4e3fc3cf2cdddae01f57a56de4fd24b91ab6d3e5cc0e8af0473659594a6bbfdaacf958f19c8d508eac12d8977616af6877106288093d37904a139220c1bc278ea56edc086976702818043e708685c7cf5fa9b4f948e1856366d5e1f3a694f9a8e954f884c89f3823ac5798ee12657bfcaba2dac9c47464c6dc2fecc17a531be19da706fee336bb6e47b645dbc71d3eff9856bddeb1ac9b644ffbdd58d7ba9e1240f1faaf797ba8a4d58becbaf85789e1bd979fcfccc209d3db7f0416bc9eef09b3a6d86b8ce8199d4310281804f4b86ccffe49d0d8ace98fb63ea9f708b284ba483d130b6a75cb76cb4e4372d6b41774f20912319420ca4cbfc1b25a8cb5f01d6381f6ebc50ed3ef08010327f5ba2acc1ac7220b3fa6f7399314db2879b0db0b5647abd87abb01295815a5b086491b2c0d81c616ed67ef8a8ce0727f446711d7323d4147b5828a52143c43b4b028180540756beba83c20a0bda11d6dec706a71744ff28090cec079dffb507d82828038fe657f61496a20317f779cb683ce8196c29a6fe28839a282eef4de57773be56808b0c3e2ac7747e2b200b2fbf20b55258cd24622a1ce0099de098ab0855106ae087f08b0c8c346d81619400c1b4838e33ed9ff90f05db8fccf8fb7ab881ca12" + PK utils: RSA depends_on:MBEDTLS_RSA_C:MBEDTLS_GENPRIME pk_utils:MBEDTLS_PK_RSA:512:64:"RSA" diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index b8069b540..20b5457a0 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -2,6 +2,8 @@ #include "mbedtls/pk.h" /* For error codes */ +#include "mbedtls/asn1.h" +#include "mbedtls/base64.h" #include "mbedtls/ecp.h" #include "mbedtls/rsa.h" @@ -111,14 +113,36 @@ void valid_parameters( ) TEST_ASSERT( mbedtls_pk_parse_public_key( &pk, NULL, 0 ) == MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); #endif /* MBEDTLS_PK_PARSE_C */ +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_PK_WRITE_C */ +void valid_parameters_pkwrite( data_t *key_data ) +{ + mbedtls_pk_context pk; + + /* For the write tests to be effective, we need a valid key pair. */ + mbedtls_pk_init( &pk ); + TEST_ASSERT( mbedtls_pk_parse_key( &pk, + key_data->x, key_data->len, + NULL, 0 ) == 0 ); -#if defined(MBEDTLS_PK_WRITE_C) TEST_ASSERT( mbedtls_pk_write_key_der( &pk, NULL, 0 ) == - MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); + MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); TEST_ASSERT( mbedtls_pk_write_pubkey_der( &pk, NULL, 0 ) == - MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); -#endif /* MBEDTLS_PK_WRITE_C */ + MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + +#if defined(MBEDTLS_PEM_WRITE_C) + TEST_ASSERT( mbedtls_pk_write_key_pem( &pk, NULL, 0 ) == + MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL ); + + TEST_ASSERT( mbedtls_pk_write_pubkey_pem( &pk, NULL, 0 ) == + MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL ); +#endif /* MBEDTLS_PEM_WRITE_C */ + +exit: + mbedtls_pk_free( &pk ); } /* END_CASE */ From ee3cfec3cc87b71ea0f9a4e3b5524a225a9e4809 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:10:02 +0100 Subject: [PATCH 09/15] PK sign/verify: hash=NULL is ok if md_alg=0 and hash_len=0 --- library/pk.c | 9 ++-- tests/suites/test_suite_pk.function | 76 +++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/library/pk.c b/library/pk.c index 38ab7747c..66301ee2d 100644 --- a/library/pk.c +++ b/library/pk.c @@ -240,7 +240,8 @@ int mbedtls_pk_verify_restartable( mbedtls_pk_context *ctx, mbedtls_pk_restart_ctx *rs_ctx ) { PK_VALIDATE_RET( ctx != NULL ); - PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( ( md_alg == MBEDTLS_MD_NONE && hash_len == 0 ) || + hash != NULL ); PK_VALIDATE_RET( sig != NULL ); if( ctx->pk_info == NULL || @@ -297,7 +298,8 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, const unsigned char *sig, size_t sig_len ) { PK_VALIDATE_RET( ctx != NULL ); - PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( ( md_alg == MBEDTLS_MD_NONE && hash_len == 0 ) || + hash != NULL ); PK_VALIDATE_RET( sig != NULL ); if( ctx->pk_info == NULL ) @@ -361,7 +363,8 @@ int mbedtls_pk_sign_restartable( mbedtls_pk_context *ctx, mbedtls_pk_restart_ctx *rs_ctx ) { PK_VALIDATE_RET( ctx != NULL ); - PK_VALIDATE_RET( hash != NULL ); + PK_VALIDATE_RET( ( md_alg == MBEDTLS_MD_NONE && hash_len == 0 ) || + hash != NULL ); PK_VALIDATE_RET( sig != NULL ); if( ctx->pk_info == NULL || diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 20b5457a0..1f5d7d61a 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -75,7 +75,9 @@ size_t mbedtls_rsa_key_len_func( void *ctx ) void valid_parameters( ) { mbedtls_pk_context pk; + unsigned char buf[1]; size_t len; + void *options = NULL; mbedtls_pk_init( &pk ); @@ -94,6 +96,49 @@ void valid_parameters( ) TEST_ASSERT( mbedtls_pk_can_do( NULL, MBEDTLS_PK_NONE ) == 0 ); + TEST_ASSERT( mbedtls_pk_sign_restartable( &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, &len, + rnd_std_rand, NULL, + NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_sign_restartable( &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, &len, + rnd_std_rand, NULL, + NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_sign( &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, &len, + rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, sizeof( buf ), + NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_verify( &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, sizeof( buf ) ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, + &pk, + MBEDTLS_MD_NONE, + NULL, 0, + buf, sizeof( buf ) ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + TEST_ASSERT( mbedtls_pk_encrypt( &pk, NULL, 0, NULL, &len, 0, @@ -155,6 +200,7 @@ void invalid_parameters( ) unsigned char *p = buf; char str[1] = {0}; mbedtls_pk_context pk; + mbedtls_md_type_t valid_md = MBEDTLS_MD_SHA256; void *options = buf; mbedtls_pk_init( &pk ); @@ -186,6 +232,12 @@ void invalid_parameters( ) NULL, sizeof( buf ), buf, sizeof( buf ), NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_restartable( &pk, + valid_md, + NULL, 0, + buf, sizeof( buf ), + NULL ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_verify_restartable( &pk, MBEDTLS_MD_NONE, @@ -203,6 +255,11 @@ void invalid_parameters( ) MBEDTLS_MD_NONE, NULL, sizeof( buf ), buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify( &pk, + valid_md, + NULL, 0, + buf, sizeof( buf ) ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_verify( &pk, MBEDTLS_MD_NONE, @@ -221,6 +278,12 @@ void invalid_parameters( ) MBEDTLS_MD_NONE, NULL, sizeof( buf ), buf, sizeof( buf ) ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, + &pk, + valid_md, + NULL, 0, + buf, sizeof( buf ) ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_verify_ext( MBEDTLS_PK_NONE, options, &pk, @@ -242,6 +305,13 @@ void invalid_parameters( ) buf, &len, rnd_std_rand, NULL, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign_restartable( &pk, + valid_md, + NULL, 0, + buf, &len, + rnd_std_rand, NULL, + NULL ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_sign_restartable( &pk, MBEDTLS_MD_NONE, @@ -262,6 +332,12 @@ void invalid_parameters( ) NULL, sizeof( buf ), buf, &len, rnd_std_rand, NULL ) ); + TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, + mbedtls_pk_sign( &pk, + valid_md, + NULL, 0, + buf, &len, + rnd_std_rand, NULL ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_sign( &pk, MBEDTLS_MD_NONE, From d54b97503b42198e3b7d9fe5d7cba82ebb03469e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:12:01 +0100 Subject: [PATCH 10/15] pk parse: the password is optional For mbedtls_pk_parse_key and mbedtls_pk_parse_keyfile, the password is optional. Clarify what this means: NULL is ok and means no password. Validate parameters and test accordingly. --- include/mbedtls/pk.h | 15 ++++++++++++--- tests/suites/test_suite_pk.function | 4 ---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 310aeef5f..716070454 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -537,8 +537,13 @@ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx ); * \param keylen Size of \b key in bytes. * For PEM data, this includes the terminating null byte, * so \p keylen must be equal to `strlen(key) + 1`. - * \param pwd password for decryption (optional) - * \param pwdlen size of the password + * \param pwd Optional password for decryption. + * Pass \c NULL if expecting a non-encrypted key. + * Pass a string of \p pwdlen bytes if expecting an encrypted + * key; a non-encrypted key will also be accepted. + * The empty password is not supported. + * \param pwdlen Size of the password in bytes. + * Ignored if \p pwd is \c NULL. * * \note On entry, ctx must be empty, either freshly initialised * with mbedtls_pk_init() or reset with mbedtls_pk_free(). If you need a @@ -583,7 +588,11 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, * * \param ctx key to be initialized * \param path filename to read the private key from - * \param password password to decrypt the file (can be NULL) + * \param password Optional password to decrypt the file. + * Pass \c NULL if expecting a non-encrypted key. + * Pass a null-terminated string if expecting an encrypted + * key; a non-encrypted key will also be accepted. + * The empty password is not supported. * * \note On entry, ctx must be empty, either freshly initialised * with mbedtls_pk_init() or reset with mbedtls_pk_free(). If you need a diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 1f5d7d61a..bf3cf5dac 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -432,10 +432,6 @@ void invalid_parameters( ) mbedtls_pk_parse_key( &pk, NULL, sizeof( buf ), buf, sizeof( buf ) ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, - mbedtls_pk_parse_key( &pk, - buf, sizeof( buf ), - NULL, sizeof( buf ) ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_parse_public_key( NULL, From 8c71b3ecb355d4967171236033ef14c270cb0d81 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:37:02 +0100 Subject: [PATCH 11/15] pk_parse*keyfile: explicitly validate path=NULL Don't rely on the check in pk_load_file, that's fragile. --- library/pkparse.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/pkparse.c b/library/pkparse.c index 127f9b840..ae210bca6 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -135,6 +135,7 @@ int mbedtls_pk_parse_keyfile( mbedtls_pk_context *ctx, unsigned char *buf; PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( path != NULL ); if( ( ret = mbedtls_pk_load_file( path, &buf, &n ) ) != 0 ) return( ret ); @@ -161,6 +162,7 @@ int mbedtls_pk_parse_public_keyfile( mbedtls_pk_context *ctx, const char *path ) unsigned char *buf; PK_VALIDATE_RET( ctx != NULL ); + PK_VALIDATE_RET( path != NULL ); if( ( ret = mbedtls_pk_load_file( path, &buf, &n ) ) != 0 ) return( ret ); From 6af45ec53e2c2e87e86e10079bc3d921b1ebde39 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 19 Dec 2018 17:52:05 +0100 Subject: [PATCH 12/15] PK: document context validity requirements Document when a context must be initialized or not, when it must be set up or not, and whether it needs a private key or a public key will do. The implementation is sometimes more liberal than the documentation, accepting a non-set-up context as a context that can't perform the requested information. This preserves backward compatibility. --- include/mbedtls/pk.h | 77 +++++++++++++++++------------ library/pk.c | 5 ++ tests/suites/test_suite_pk.function | 6 ++- 3 files changed, 55 insertions(+), 33 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 716070454..91950f940 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -207,7 +207,7 @@ void mbedtls_pk_init( mbedtls_pk_context *ctx ); /** * \brief Free the components of a #mbedtls_pk_context. * - * \param ctx The context to clear. + * \param ctx The context to clear. It must have been initialized. * If this is \c NULL, this function does nothing. */ void mbedtls_pk_free( mbedtls_pk_context *ctx ); @@ -224,7 +224,7 @@ void mbedtls_pk_restart_init( mbedtls_pk_restart_ctx *ctx ); /** * \brief Free the components of a restart context * - * \param ctx The context to clear. + * \param ctx The context to clear. It must have been initialized. * If this is \c NULL, this function does nothing. */ void mbedtls_pk_restart_free( mbedtls_pk_restart_ctx *ctx ); @@ -234,7 +234,8 @@ void mbedtls_pk_restart_free( mbedtls_pk_restart_ctx *ctx ); * \brief Initialize a PK context with the information given * and allocates the type-specific PK subcontext. * - * \param ctx Context to initialize. Must be empty (type NONE). + * \param ctx Context to initialize. It must not have been set + * up yet (type #MBEDTLS_PK_NONE). * \param info Information to use * * \return 0 on success, @@ -250,7 +251,8 @@ int mbedtls_pk_setup( mbedtls_pk_context *ctx, const mbedtls_pk_info_t *info ); /** * \brief Initialize an RSA-alt context * - * \param ctx Context to initialize. Must be empty (type NONE). + * \param ctx Context to initialize. It must not have been set + * up yet (type #MBEDTLS_PK_NONE). * \param key RSA key pointer * \param decrypt_func Decryption function * \param sign_func Signing function @@ -270,7 +272,7 @@ int mbedtls_pk_setup_rsa_alt( mbedtls_pk_context *ctx, void * key, /** * \brief Get the size in bits of the underlying key * - * \param ctx Context to use + * \param ctx The context to query. It must have been initialized. * * \return Key size in bits, or 0 on error */ @@ -278,7 +280,8 @@ size_t mbedtls_pk_get_bitlen( const mbedtls_pk_context *ctx ); /** * \brief Get the length in bytes of the underlying key - * \param ctx Context to use + * + * \param ctx The context to query. It must have been initialized. * * \return Key length in bytes, or 0 on error */ @@ -290,18 +293,21 @@ static inline size_t mbedtls_pk_get_len( const mbedtls_pk_context *ctx ) /** * \brief Tell if a context can do the operation given by type * - * \param ctx Context to test - * \param type Target type + * \param ctx The context to query. It must have been initialized. + * \param type The desired type. * - * \return 0 if context can't do the operations, - * 1 otherwise. + * \return 1 if the context can do operations on the given type. + * \return 0 if the context cannot do the operations on the given + * type. This is always the case for a context that has + * been initialized but not set up, or that has been + * cleared with mbedtls_pk_free(). */ int mbedtls_pk_can_do( const mbedtls_pk_context *ctx, mbedtls_pk_type_t type ); /** * \brief Verify signature (including padding if relevant). * - * \param ctx PK context to use + * \param ctx The PK context to use. It must have been set up. * \param md_alg Hash algorithm used (see notes) * \param hash Hash of the message to sign * \param hash_len Hash length or 0 (see notes) @@ -334,7 +340,7 @@ int mbedtls_pk_verify( mbedtls_pk_context *ctx, mbedtls_md_type_t md_alg, * \c mbedtls_ecp_set_max_ops() to reduce blocking for ECC * operations. For RSA, same as \c mbedtls_pk_verify(). * - * \param ctx PK context to use + * \param ctx The PK context to use. It must have been set up. * \param md_alg Hash algorithm used (see notes) * \param hash Hash of the message to sign * \param hash_len Hash length or 0 (see notes) @@ -358,7 +364,7 @@ int mbedtls_pk_verify_restartable( mbedtls_pk_context *ctx, * * \param type Signature type (inc. possible padding type) to verify * \param options Pointer to type-specific options, or NULL - * \param ctx PK context to use + * \param ctx The PK context to use. It must have been set up. * \param md_alg Hash algorithm used (see notes) * \param hash Hash of the message to sign * \param hash_len Hash length or 0 (see notes) @@ -389,7 +395,8 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, /** * \brief Make signature, including padding if relevant. * - * \param ctx PK context to use - must hold a private key + * \param ctx The PK context to use. It must have been set up + * with a private key. * \param md_alg Hash algorithm used (see notes) * \param hash Hash of the message to sign * \param hash_len Hash length or 0 (see notes) @@ -423,7 +430,8 @@ int mbedtls_pk_sign( mbedtls_pk_context *ctx, mbedtls_md_type_t md_alg, * \c mbedtls_ecp_set_max_ops() to reduce blocking for ECC * operations. For RSA, same as \c mbedtls_pk_sign(). * - * \param ctx PK context to use - must hold a private key + * \param ctx The PK context to use. It must have been set up + * with a private key. * \param md_alg Hash algorithm used (see notes) * \param hash Hash of the message to sign * \param hash_len Hash length or 0 (see notes) @@ -447,7 +455,8 @@ int mbedtls_pk_sign_restartable( mbedtls_pk_context *ctx, /** * \brief Decrypt message (including padding if relevant). * - * \param ctx PK context to use - must hold a private key + * \param ctx The PK context to use. It must have been set up + * with a private key. * \param input Input to decrypt * \param ilen Input size * \param output Decrypted output @@ -468,7 +477,7 @@ int mbedtls_pk_decrypt( mbedtls_pk_context *ctx, /** * \brief Encrypt message (including padding if relevant). * - * \param ctx PK context to use + * \param ctx The PK context to use. It must have been set up. * \param input Message to encrypt * \param ilen Message size * \param output Encrypted output @@ -499,7 +508,7 @@ int mbedtls_pk_check_pair( const mbedtls_pk_context *pub, const mbedtls_pk_conte /** * \brief Export debug information * - * \param ctx Context to use + * \param ctx The PK context to use. It must have been initialized. * \param items Place to write debug items * * \return 0 on success or MBEDTLS_ERR_PK_BAD_INPUT_DATA @@ -509,7 +518,7 @@ int mbedtls_pk_debug( const mbedtls_pk_context *ctx, mbedtls_pk_debug_item *item /** * \brief Access the type name * - * \param ctx Context to use + * \param ctx The PK context to use. It must have been initialized. * * \return Type name on success, or "invalid PK" */ @@ -518,9 +527,10 @@ const char * mbedtls_pk_get_name( const mbedtls_pk_context *ctx ); /** * \brief Get the key type * - * \param ctx Context to use + * \param ctx The PK context to use. It must have been initialized. * - * \return Type on success, or MBEDTLS_PK_NONE + * \return Type on success. + * \return #MBEDTLS_PK_NONE for a context that has not been set up. */ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx ); @@ -529,7 +539,8 @@ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx ); /** * \brief Parse a private key in PEM or DER format * - * \param ctx key to be initialized + * \param ctx The PK context to fill. It must have been initialized + * but not set up. * \param key Input buffer to parse. * The buffer must contain the input exactly, with no * extra trailing material. For PEM, the buffer must @@ -561,7 +572,8 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *ctx, /** * \brief Parse a public key in PEM or DER format * - * \param ctx key to be initialized + * \param ctx The PK context to fill. It must have been initialized + * but not set up. * \param key Input buffer to parse. * The buffer must contain the input exactly, with no * extra trailing material. For PEM, the buffer must @@ -586,7 +598,8 @@ int mbedtls_pk_parse_public_key( mbedtls_pk_context *ctx, /** * \brief Load and parse a private key * - * \param ctx key to be initialized + * \param ctx The PK context to fill. It must have been initialized + * but not set up. * \param path filename to read the private key from * \param password Optional password to decrypt the file. * Pass \c NULL if expecting a non-encrypted key. @@ -609,7 +622,8 @@ int mbedtls_pk_parse_keyfile( mbedtls_pk_context *ctx, /** * \brief Load and parse a public key * - * \param ctx key to be initialized + * \param ctx The PK context to fill. It must have been initialized + * but not set up. * \param path filename to read the public key from * * \note On entry, ctx must be empty, either freshly initialised @@ -632,7 +646,7 @@ int mbedtls_pk_parse_public_keyfile( mbedtls_pk_context *ctx, const char *path ) * return value to determine where you should start * using the buffer * - * \param ctx private to write away + * \param ctx PK context which must contain a valid private key. * \param buf buffer to write to * \param size size of the buffer * @@ -647,7 +661,7 @@ int mbedtls_pk_write_key_der( mbedtls_pk_context *ctx, unsigned char *buf, size_ * return value to determine where you should start * using the buffer * - * \param ctx public key to write away + * \param ctx PK context which must contain a valid public or private key. * \param buf buffer to write to * \param size size of the buffer * @@ -660,7 +674,7 @@ int mbedtls_pk_write_pubkey_der( mbedtls_pk_context *ctx, unsigned char *buf, si /** * \brief Write a public key to a PEM string * - * \param ctx Context containing the public key to write. + * \param ctx PK context which must contain a valid public or private key. * \param buf Buffer to write to. The output includes a * terminating null byte. * \param size Size of the buffer in bytes. @@ -672,7 +686,7 @@ int mbedtls_pk_write_pubkey_pem( mbedtls_pk_context *ctx, unsigned char *buf, si /** * \brief Write a private key to a PKCS#1 or SEC1 PEM string * - * \param ctx Context containing the private key to write. + * \param ctx PK context which must contain a valid private key. * \param buf Buffer to write to. The output includes a * terminating null byte. * \param size Size of the buffer in bytes. @@ -694,7 +708,8 @@ int mbedtls_pk_write_key_pem( mbedtls_pk_context *ctx, unsigned char *buf, size_ * * \param p the position in the ASN.1 data * \param end end of the buffer - * \param pk the key to fill + * \param pk The PK context to fill. It must have been initialized + * but not set up. * * \return 0 if successful, or a specific PK error code */ @@ -709,7 +724,7 @@ int mbedtls_pk_parse_subpubkey( unsigned char **p, const unsigned char *end, * * \param p reference to current position pointer * \param start start of the buffer (for bounds-checking) - * \param key public key to write away + * \param key PK context which must contain a valid public or private key. * * \return the length written or a negative error code */ diff --git a/library/pk.c b/library/pk.c index 66301ee2d..2658627c4 100644 --- a/library/pk.c +++ b/library/pk.c @@ -183,6 +183,9 @@ int mbedtls_pk_setup_rsa_alt( mbedtls_pk_context *ctx, void * key, */ int mbedtls_pk_can_do( const mbedtls_pk_context *ctx, mbedtls_pk_type_t type ) { + /* A context with null pk_info is not set up yet and can't do anything. + * For backward compatibility, also accept NULL instead of a context + * pointer. */ if( ctx == NULL || ctx->pk_info == NULL ) return( 0 ); @@ -493,6 +496,8 @@ int mbedtls_pk_check_pair( const mbedtls_pk_context *pub, const mbedtls_pk_conte */ size_t mbedtls_pk_get_bitlen( const mbedtls_pk_context *ctx ) { + /* For backward compatibility, accept NULL or a context that + * isn't set up yet, and return a fake value that should be safe. */ if( ctx == NULL || ctx->pk_info == NULL ) return( 0 ); diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index bf3cf5dac..8a09171e6 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -90,10 +90,12 @@ void valid_parameters( ) TEST_ASSERT( mbedtls_pk_setup( &pk, NULL ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + /* In informational functions, we accept NULL where a context pointer + * is expected because that's what the library has done forever. + * We do not document that NULL is accepted, so we may wish to change + * the behavior in a future version. */ TEST_ASSERT( mbedtls_pk_get_bitlen( NULL ) == 0 ); - TEST_ASSERT( mbedtls_pk_get_len( NULL ) == 0 ); - TEST_ASSERT( mbedtls_pk_can_do( NULL, MBEDTLS_PK_NONE ) == 0 ); TEST_ASSERT( mbedtls_pk_sign_restartable( &pk, From d6027119bedf0583203947ae3bf714774af9b045 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Dec 2018 12:15:41 +0100 Subject: [PATCH 13/15] Fix dependencies on MBEDTLS_FS_IO --- tests/suites/test_suite_pk.function | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 8a09171e6..fac7e6143 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -200,7 +200,9 @@ void invalid_parameters( ) unsigned char *null_buf = NULL; unsigned char buf[1]; unsigned char *p = buf; +#if defined(MBEDTLS_FS_IO) char str[1] = {0}; +#endif mbedtls_pk_context pk; mbedtls_md_type_t valid_md = MBEDTLS_MD_SHA256; void *options = buf; @@ -398,6 +400,7 @@ void invalid_parameters( ) mbedtls_pk_debug( NULL, NULL ) ); #if defined(MBEDTLS_PK_PARSE_C) +#if defined(MBEDTLS_FS_IO) TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_load_file( NULL, &p, &len ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, @@ -405,7 +408,6 @@ void invalid_parameters( ) TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_load_file( str, &p, NULL ) ); -#if defined(MBEDTLS_FS_IO) TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, mbedtls_pk_parse_keyfile( NULL, str, NULL ) ); TEST_INVALID_PARAM_RET( MBEDTLS_ERR_PK_BAD_INPUT_DATA, From 88ca3a244e1d9b0cb31f95d0d306b992a79a36af Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Dec 2018 12:26:16 +0100 Subject: [PATCH 14/15] Avoid unused-variable warnings in some configurations --- tests/suites/test_suite_pk.function | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index fac7e6143..231a74d63 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -207,6 +207,9 @@ void invalid_parameters( ) mbedtls_md_type_t valid_md = MBEDTLS_MD_SHA256; void *options = buf; + (void) null_buf; + (void) p; + mbedtls_pk_init( &pk ); TEST_INVALID_PARAM( mbedtls_pk_init( NULL ) ); From 743e3988dcf6c63403cfbdf3b824166542ccbb15 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Dec 2018 12:29:48 +0100 Subject: [PATCH 15/15] Avoid unused-variable warnings for str as well The exact guard is FS_IO && PK_PARSE_C. Just keep it simple. --- tests/suites/test_suite_pk.function | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 231a74d63..4e6ab172c 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -200,15 +200,14 @@ void invalid_parameters( ) unsigned char *null_buf = NULL; unsigned char buf[1]; unsigned char *p = buf; -#if defined(MBEDTLS_FS_IO) char str[1] = {0}; -#endif mbedtls_pk_context pk; mbedtls_md_type_t valid_md = MBEDTLS_MD_SHA256; void *options = buf; (void) null_buf; (void) p; + (void) str; mbedtls_pk_init( &pk );