Skip to content

Fix pk_parse_key() validation of RSA private keys #363

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

Closed
wants to merge 4 commits into from
Closed

Fix pk_parse_key() validation of RSA private keys #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dc3d86
Add mbedtls_rsa_is_private()
mpg Feb 7, 2020
ee70d56
Fix pk_parse_key() to check the key is private
mpg Feb 7, 2020
4422ac8
Clean up test function pk_parse_key
mpg Feb 10, 2020
fdb6221
Test each failure mode of pk_parse_key_pkcs1_der()
mpg Feb 10, 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
2 changes: 1 addition & 1 deletion include/mbedtls/pk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@ mbedtls_pk_type_t mbedtls_pk_get_type( const mbedtls_pk_context *ctx );
* with mbedtls_pk_init() or reset with mbedtls_pk_free(). If you need a
* specific key type, check the result with mbedtls_pk_can_do().
*
* \note The key is also checked for correctness.
* \note The key is also checked for sanity.
*
* \return 0 if successful, or a specific PK or PEM error code
*/
Expand Down
14 changes: 14 additions & 0 deletions include/mbedtls/rsa.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,20 @@ void mbedtls_rsa_set_padding( mbedtls_rsa_context *ctx, int padding,
*/
size_t mbedtls_rsa_get_len( const mbedtls_rsa_context *ctx );

/**
* \brief This function says if the context contains a private key.
*
* \note This function does not check the correctness or even sanity
* of the parameters, it just tells if the private part is
* present. For more checks, see mbedtls_rsa_check_privkey().
*
* \param ctx The initialized RSA context.
*
* \return \c 1 if the context contains a private key.
* \c 0 if the context doesn't contain a private key.
*/
int mbedtls_rsa_is_private( const mbedtls_rsa_context *ctx );

/**
* \brief This function generates an RSA keypair.
*
Expand Down
17 changes: 15 additions & 2 deletions library/pkparse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,9 +801,22 @@ 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 )
/* Complete and sanity-check the RSA private key.
*
* rsa_complete() includes some sanity checks but it guesses at the type of
* key, so we need to explicitly ensure that the key is private.
* We also test the public part for consistency with pk_get_rsapubkey().
*/
if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ||
( ret = mbedtls_rsa_check_pubkey( rsa ) ) != 0 )
{
goto cleanup;
}
if( !mbedtls_rsa_is_private( rsa ) )
{
ret = MBEDTLS_ERR_PK_KEY_INVALID_FORMAT;
goto cleanup;
}

if( p != end )
{
Expand Down
32 changes: 12 additions & 20 deletions library/rsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,23 +398,24 @@ int mbedtls_rsa_export_raw( const mbedtls_rsa_context *ctx,
return( ret );
}

int mbedtls_rsa_is_private( const mbedtls_rsa_context *ctx )
{
return(
mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function returns 0 on an incomplete key. I find this behavior surprising. I'd prefer mbedtls_rsa_is_private(rsa) to return the same thing before and after mbedtls_rsa_complete(rsa). If it doesn't, this should be documented clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I agree this should be documented. I'm likely to rework the PR in a way that no longer relies on this function, but I'll make sure to properly document that aspect of the new function.

mbedtls_mpi_cmp_int( &ctx->P, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Two comments - first this seems to accept values which are negative, values which could be deserialized from the DER structure. Also it seems to ignore the CRT params, which should probably also be checked, if CRT is enabled at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! Actually I think the checks in rsa_check_context() (called from mbedtls_rsa_complete() called from mbedtls_pk_parse_key()) already prevent that, but I agree that it should be more obvious. I was intending to rework the PR anyway, as I'm no longer convinced mbedtls_rsa_is_private() is the right function here, so I'll keep that in mind when reworking.

}

int mbedtls_rsa_export( const mbedtls_rsa_context *ctx,
mbedtls_mpi *N, mbedtls_mpi *P, mbedtls_mpi *Q,
mbedtls_mpi *D, mbedtls_mpi *E )
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
int is_priv;
RSA_VALIDATE_RET( ctx != NULL );

/* Check if key is private or public */
is_priv =
mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->P, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0;

if( !is_priv )
if( !mbedtls_rsa_is_private( ctx ) )
{
/* If we're trying to export private parameters for a public key,
* something must be wrong. */
Expand Down Expand Up @@ -447,18 +448,9 @@ int mbedtls_rsa_export_crt( const mbedtls_rsa_context *ctx,
mbedtls_mpi *DP, mbedtls_mpi *DQ, mbedtls_mpi *QP )
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
int is_priv;
RSA_VALIDATE_RET( ctx != NULL );

/* Check if key is private or public */
is_priv =
mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->P, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&
mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0;

if( !is_priv )
if( !mbedtls_rsa_is_private( ctx ) )
return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA );

#if !defined(MBEDTLS_RSA_NO_CRT)
Expand Down
69 changes: 58 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,80 @@ 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 == p == q == 0)
depends_on:MBEDTLS_RSA_C
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900000000000000000002090000000000000000000209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT

Key ASN1 (RSAPrivateKey, correct values, length mismatch)
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
33 changes: 33 additions & 0 deletions tests/suites/test_suite_rsa.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,39 @@ RSA PKCS1 Encrypt Bad RNG
depends_on:MBEDTLS_PKCS1_V15
rsa_pkcs1_encrypt_bad_rng:"4E636AF98E40F3ADCFCCB698F4E80B9F":MBEDTLS_RSA_PKCS_V15:2048:16:"b38ac65c8141f7f5c96e14470e851936a67bf94cc6821a39ac12c05f7c0b06d9e6ddba2224703b02e25f31452f9c4a8417b62675fdc6df46b94813bc7b9769a892c482b830bfe0ad42e46668ace68903617faf6681f4babf1cc8e4b0420d3c7f61dc45434c6b54e2c3ee0fc07908509d79c9826e673bf8363255adb0add2401039a7bcd1b4ecf0fbe6ec8369d2da486eec59559dd1d54c9b24190965eafbdab203b35255765261cd0909acf93c3b8b8428cbb448de4715d1b813d0c94829c229543d391ce0adab5351f97a3810c1f73d7b1458b97daed4209c50e16d064d2d5bfda8c23893d755222793146d0a78c3d64f35549141486c3b0961a7b4c1a2034f":16:"3":"a42eda41e56235e666e7faaa77100197f657288a1bf183e4820f0c37ce2c456b960278d6003e0bbcd4be4a969f8e8fd9231e1f492414f00ed09844994c86ec32db7cde3bec7f0c3dbf6ae55baeb2712fa609f5fc3207a824eb3dace31849cd6a6084318523912bccb84cf42e3c6d6d1685131d69bb545acec827d2b0dfdd5568b7dcc4f5a11d6916583fefa689d367f8c9e1d95dcd2240895a9470b0c1730f97cd6e8546860bd254801769f54be96e16362ddcbf34d56035028890199e0f48db38642cb66a4181e028a6443a404fea284ce02b4614b683367d40874e505611d23142d49f06feea831d52d347b13610b413c4efc43a6de9f0b08d2a951dc503b6":MBEDTLS_ERR_RSA_RNG_FAILED

RSA is_private: private
rsa_is_private:1:"01":"01":"01":"01":"01"

RSA is_private: N == 0
rsa_is_private:0:"00":"01":"01":"01":"01"

RSA is_private: E == 0
rsa_is_private:0:"01":"00":"01":"01":"01"

RSA is_private: P == 0
rsa_is_private:0:"01":"01":"00":"01":"01"

RSA is_private: Q == 0
rsa_is_private:0:"01":"01":"01":"00":"01"

RSA is_private: D == 0
rsa_is_private:0:"01":"01":"01":"01":"00"

RSA is_private: N NULL
rsa_is_private:0:"":"01":"01":"01":"01"

RSA is_private: E NULL
rsa_is_private:0:"01":"":"01":"01":"01"

RSA is_private: P NULL
rsa_is_private:0:"01":"01":"":"01":"01"

RSA is_private: Q NULL
rsa_is_private:0:"01":"01":"01":"":"01"

RSA is_private: D NULL
rsa_is_private:0:"01":"01":"01":"01":""

RSA Selftest
depends_on:MBEDTLS_SELF_TEST
rsa_selftest:
22 changes: 22 additions & 0 deletions tests/suites/test_suite_rsa.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1784,6 +1784,28 @@ exit:
}
/* END_CASE */

/* BEGIN_CASE */
void rsa_is_private( int is_private, data_t *N, data_t *E,
data_t *P, data_t *Q, data_t *D )
{
mbedtls_rsa_context ctx;

mbedtls_rsa_init( &ctx, 0, 0 );

TEST_ASSERT( mbedtls_rsa_import_raw( &ctx,
N->len ? N->x : NULL, N->len,
P->len ? P->x : NULL, P->len,
Q->len ? Q->x : NULL, Q->len,
D->len ? D->x : NULL, D->len,
E->len ? E->x : NULL, E->len ) == 0 );

TEST_EQUAL( mbedtls_rsa_is_private( &ctx ), is_private );

exit:
mbedtls_rsa_free( &ctx );
}
/* END_CASE */

/* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */
void rsa_selftest( )
{
Expand Down