Skip to content

Fix pk_parse_key()'s use of rsa_complete() #367

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 5 commits into from
Feb 19, 2020
Merged

Fix pk_parse_key()'s use of rsa_complete() #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a04a2c3
Don't pass zero to rsa_complete() as a param
mpg Feb 18, 2020
c422679
Check public part when parsing private RSA key
mpg Feb 14, 2020
b65370f
Clean up test function pk_parse_key
mpg Feb 10, 2020
9bbe328
Test each failure mode of pk_parse_key_pkcs1_der()
mpg Feb 10, 2020
bbb5a0a
Fix pkparse bug wrt MBEDTLS_RSA_ALT
mpg Feb 18, 2020
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
98 changes: 64 additions & 34 deletions library/pkparse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -730,46 +756,36 @@ 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)
#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
Expand All @@ -782,28 +798,42 @@ 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 ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're only verifying “existance” (sic) here, why test for zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with the CRT case. I'd like to avoid changing the set of keys accepted by this function too much depending on whether MBEDTLS_RSA_NO_CRT is enabled or not, at least when we can conveniently avoid differences.

( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 )
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 )
{
Expand Down
73 changes: 62 additions & 11 deletions tests/suites/test_suite_pkparse.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,33 +1072,84 @@ 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)
pk_parse_key:"":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
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
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, correct format+values, minimal modulus size (128 bit))
depends_on:MBEDTLS_RSA_C
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":0

Key ASN1 (RSAPrivateKey, correct format, modulus too small (127 bit))
depends_on:MBEDTLS_RSA_C
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, values present, length mismatch)
Key ASN1 (RSAPrivateKey, correct values, dq wrong tag)
depends_on:MBEDTLS_RSA_C
pk_parse_key:"301c02010002010102010102010102010102010102010102010102010100":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401FF0813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT

Key ASN1 (RSAPrivateKey, values present, check_privkey fails)
Key ASN1 (RSAPrivateKey, correct values, qp wrong tag)
depends_on:MBEDTLS_RSA_C
pk_parse_key:"301b020100020102020101020101020101020101020101020101020101":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b7221FF08052b93d01747a87c":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
15 changes: 3 additions & 12 deletions tests/suites/test_suite_pkparse.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down