From a04a2c3ef13643ad7ee1c90f11a3a111dd39ca03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Feb 2020 10:12:14 +0100 Subject: [PATCH 1/5] Don't pass zero to rsa_complete() as a param When parsing a PKCS#1 RSAPrivateKey structure, all parameters are always present. After importing them, we need to call rsa_complete() for the sake of alternative implementations. That function interprets zero as a signal for "this parameter was not provided". As that's never the case, we mustn't pass any zero value to that function, so we need to explicitly check for it. --- library/pkparse.c | 81 +++++++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 31 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index 7df30fea9..7fb24e843 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -678,6 +678,32 @@ int mbedtls_pk_parse_subpubkey( unsigned char **p, const unsigned char *end, } #if defined(MBEDTLS_RSA_C) +/* + * Wrapper around mbedtls_asn1_get_mpi() that rejects zero. + * + * The value zero is: + * - never a valid value for an RSA parameter + * - interpreted as "omitted, please reconstruct" by mbedtls_rsa_complete(). + * + * Since values can't be omitted in PKCS#1, passing a zero value to + * rsa_complete() would be incorrect, so reject zero values early. + */ +static int asn1_get_nonzero_mpi( unsigned char **p, + const unsigned char *end, + mbedtls_mpi *X ) +{ + int ret; + + ret = mbedtls_asn1_get_mpi( p, end, X ); + if( ret != 0 ) + return( ret ); + + if( mbedtls_mpi_cmp_int( X, 0 ) == 0 ) + return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT ); + + return( 0 ); +} + /* * Parse a PKCS#1 encoded private RSA key */ @@ -730,44 +756,34 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, } /* Import N */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_rsa_import_raw( rsa, p, len, NULL, 0, NULL, 0, - NULL, 0, NULL, 0 ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_rsa_import( rsa, &T, NULL, NULL, + NULL, NULL ) ) != 0 ) goto cleanup; - p += len; /* Import E */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, NULL, 0, - NULL, 0, p, len ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_rsa_import( rsa, NULL, NULL, NULL, + NULL, &T ) ) != 0 ) goto cleanup; - p += len; /* Import D */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, NULL, 0, - p, len, NULL, 0 ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_rsa_import( rsa, NULL, NULL, NULL, + &T, NULL ) ) != 0 ) goto cleanup; - p += len; /* Import P */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, p, len, NULL, 0, - NULL, 0, NULL, 0 ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_rsa_import( rsa, NULL, &T, NULL, + NULL, NULL ) ) != 0 ) goto cleanup; - p += len; /* Import Q */ - if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, - MBEDTLS_ASN1_INTEGER ) ) != 0 || - ( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, p, len, - NULL, 0, NULL, 0 ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_rsa_import( rsa, NULL, NULL, &T, + NULL, NULL ) ) != 0 ) goto cleanup; - p += len; #if !defined(MBEDTLS_RSA_NO_CRT) /* @@ -782,22 +798,25 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, */ /* Import DP */ - if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_mpi_copy( &rsa->DP, &T ) ) != 0 ) goto cleanup; /* Import DQ */ - if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DQ ) ) != 0) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_mpi_copy( &rsa->DQ, &T ) ) != 0 ) goto cleanup; /* Import QP */ - if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->QP ) ) != 0) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = mbedtls_mpi_copy( &rsa->QP, &T ) ) != 0 ) goto cleanup; #else /* Verify existance of the CRT params */ - if( ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || - ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || - ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ) + if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 || + ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ) goto cleanup; #endif From c42267920c6601f6174a3bdd0802ff5b1d08e8f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 14 Feb 2020 11:28:47 +0100 Subject: [PATCH 2/5] Check public part when parsing private RSA key --- library/pkparse.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index 7fb24e843..da9edc973 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -820,9 +820,20 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; #endif - /* Complete the RSA private key */ - if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) + /* rsa_complete() doesn't complete anything with the default + * implementation but is still called: + * - for the benefit of alternative implementation that may want to + * pre-compute stuff beyond what's provided (eg Montgomery factors) + * - as is also sanity-checks the key + * + * Furthermore, we also check the public part for consistency with + * mbedtls_pk_parse_pubkey(), as it includes size minima for example. + */ + if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 || + ( ret = mbedtls_rsa_check_pubkey( rsa ) ) != 0 ) + { goto cleanup; + } if( p != end ) { From b65370f97d88120c39fef88a224736dcb66faf94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 10 Feb 2020 10:50:16 +0100 Subject: [PATCH 3/5] Clean up test function pk_parse_key - remove incorrect compile-time dependency (the individual cases already have correct run-time dependency information) - remove unused argument - remove unused stack buffer - remove useless code block --- tests/suites/test_suite_pkparse.data | 16 ++++++++-------- tests/suites/test_suite_pkparse.function | 15 +++------------ 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index fc643a88a..328f3e048 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -1073,32 +1073,32 @@ depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MB pk_parse_keyfile_ec:"data_files/ec_prv.specdom.der":"NULL":0 Key ASN1 (Incorrect first tag) -pk_parse_key:"":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, incorrect version tag) depends_on:MBEDTLS_RSA_C -pk_parse_key:"300100":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"300100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, version tag missing) depends_on:MBEDTLS_RSA_C -pk_parse_key:"3000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"3000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, invalid version) depends_on:MBEDTLS_RSA_C -pk_parse_key:"3003020101":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"3003020101":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, correct version, incorrect tag) depends_on:MBEDTLS_RSA_C -pk_parse_key:"300402010000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"300402010000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, values present, length mismatch) depends_on:MBEDTLS_RSA_C -pk_parse_key:"301c02010002010102010102010102010102010102010102010102010100":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"301c02010002010102010102010102010102010102010102010102010100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (RSAPrivateKey, values present, check_privkey fails) depends_on:MBEDTLS_RSA_C -pk_parse_key:"301b020100020102020101020101020101020101020101020101020101":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"301b020100020102020101020101020101020101020101020101020101":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (ECPrivateKey, empty parameters) depends_on:MBEDTLS_ECP_C -pk_parse_key:"30070201010400a000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"30070201010400a000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT diff --git a/tests/suites/test_suite_pkparse.function b/tests/suites/test_suite_pkparse.function index 3eb0397e6..4650d3311 100644 --- a/tests/suites/test_suite_pkparse.function +++ b/tests/suites/test_suite_pkparse.function @@ -113,23 +113,14 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_RSA_C */ -void pk_parse_key( data_t * buf, char * result_str, int result ) +/* BEGIN_CASE */ +void pk_parse_key( data_t * buf, int result ) { mbedtls_pk_context pk; - unsigned char output[2000]; - ((void) result_str); mbedtls_pk_init( &pk ); - memset( output, 0, 2000 ); - - - TEST_ASSERT( mbedtls_pk_parse_key( &pk, buf->x, buf->len, NULL, 0 ) == ( result ) ); - if( ( result ) == 0 ) - { - TEST_ASSERT( 1 ); - } + TEST_ASSERT( mbedtls_pk_parse_key( &pk, buf->x, buf->len, NULL, 0 ) == result ); exit: mbedtls_pk_free( &pk ); From 9bbe328752256d9b38d2f6e98c099526f1687051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 10 Feb 2020 12:49:50 +0100 Subject: [PATCH 4/5] Test each failure mode of pk_parse_key_pkcs1_der() (Only the top-level ones, ie, for each call to eg asn1_get_mpi(), ensure there's at least one test case that makes this call fail in one way, but don't test the various ways to make asn1_get_mpi fail - that should be covered elsewhere.) - the new checks added by the previous commits needed exercising - existing tests sometimes had wrong descriptions or where passing for the wrong reason (eg with the "length mismatch" test, the function actually failed before reaching the length check) - while at it, add tests for the rest as well The valid minimal-size key was generated with: openssl genrsa 128 2>/dev/null | openssl rsa -outform der 2>/dev/null | xxd -p --- tests/suites/test_suite_pkparse.data | 61 +++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index 328f3e048..91d51977a 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -1072,9 +1072,12 @@ Parse EC Key #15 (SEC1 DER, secp256k1, SpecifiedECDomain) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED pk_parse_keyfile_ec:"data_files/ec_prv.specdom.der":"NULL":0 -Key ASN1 (Incorrect first tag) +Key ASN1 (No data) pk_parse_key:"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +Key ASN1 (First tag not Sequence) +pk_parse_key:"020100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + Key ASN1 (RSAPrivateKey, incorrect version tag) depends_on:MBEDTLS_RSA_C pk_parse_key:"300100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT @@ -1091,13 +1094,61 @@ Key ASN1 (RSAPrivateKey, correct version, incorrect tag) depends_on:MBEDTLS_RSA_C pk_parse_key:"300402010000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT -Key ASN1 (RSAPrivateKey, values present, length mismatch) +Key ASN1 (RSAPrivateKey, correct format+values, minimal modulus size (128 bit)) depends_on:MBEDTLS_RSA_C -pk_parse_key:"301c02010002010102010102010102010102010102010102010102010100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":0 -Key ASN1 (RSAPrivateKey, values present, check_privkey fails) +Key ASN1 (RSAPrivateKey, correct format, modulus too small (127 bit)) depends_on:MBEDTLS_RSA_C -pk_parse_key:"301b020100020102020101020101020101020101020101020101020101":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT +pk_parse_key:"30630201000211007c8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct format, modulus even) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857002030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct format, d == 0) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"30630201000211007c8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct format, d == p == q == 0) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900000000000000000002090000000000000000000209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, trailing garbage) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3064020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c00":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, n wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100FF1100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, e wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571FF030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, d wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c85710203010001FF11009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, p wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201FF0900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, q wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61FF0900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, dp wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a211FF09009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, dq wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401FF0813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT + +Key ASN1 (RSAPrivateKey, correct values, qp wrong tag) +depends_on:MBEDTLS_RSA_C +pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b7221FF08052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT Key ASN1 (ECPrivateKey, empty parameters) depends_on:MBEDTLS_ECP_C From bbb5a0a94a526df1d17bfe02195fb3091031f8b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Feb 2020 10:22:54 +0100 Subject: [PATCH 5/5] Fix pkparse bug wrt MBEDTLS_RSA_ALT Some code paths want to access members of the mbedtls_rsa_context structure. We can only do that when using our own implementation, as otherwise we don't know anything about that structure. --- library/pkparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/pkparse.c b/library/pkparse.c index da9edc973..1cbb8cc33 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -785,7 +785,7 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, NULL, NULL ) ) != 0 ) goto cleanup; -#if !defined(MBEDTLS_RSA_NO_CRT) +#if !defined(MBEDTLS_RSA_NO_CRT) && !defined(MBEDTLS_RSA_ALT) /* * The RSA CRT parameters DP, DQ and QP are nominally redundant, in * that they can be easily recomputed from D, P and Q. However by