Skip to content

Commit f841eab

Browse files
gilles-peskine-armgilles-peskine-arm
authored
Merge pull request #367 from mpg/fix-rsa-complete
Fix pk_parse_key()'s use of rsa_complete()
2 parents f8b9329 + bbb5a0a commit f841eab

File tree

3 files changed

+129
-57
lines changed

3 files changed

+129
-57
lines changed

‎library/pkparse.c

Lines changed: 64 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,32 @@ int mbedtls_pk_parse_subpubkey( unsigned char **p, const unsigned char *end,
678678
}
679679

680680
#if defined(MBEDTLS_RSA_C)
681+
/*
682+
* Wrapper around mbedtls_asn1_get_mpi() that rejects zero.
683+
*
684+
* The value zero is:
685+
* - never a valid value for an RSA parameter
686+
* - interpreted as "omitted, please reconstruct" by mbedtls_rsa_complete().
687+
*
688+
* Since values can't be omitted in PKCS#1, passing a zero value to
689+
* rsa_complete() would be incorrect, so reject zero values early.
690+
*/
691+
static int asn1_get_nonzero_mpi( unsigned char **p,
692+
const unsigned char *end,
693+
mbedtls_mpi *X )
694+
{
695+
int ret;
696+
697+
ret = mbedtls_asn1_get_mpi( p, end, X );
698+
if( ret != 0 )
699+
return( ret );
700+
701+
if( mbedtls_mpi_cmp_int( X, 0 ) == 0 )
702+
return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT );
703+
704+
return( 0 );
705+
}
706+
681707
/*
682708
* Parse a PKCS#1 encoded private RSA key
683709
*/
@@ -730,46 +756,36 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa,
730756
}
731757

732758
/* Import N */
733-
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
734-
MBEDTLS_ASN1_INTEGER ) ) != 0 ||
735-
( ret = mbedtls_rsa_import_raw( rsa, p, len, NULL, 0, NULL, 0,
736-
NULL, 0, NULL, 0 ) ) != 0 )
759+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
760+
( ret = mbedtls_rsa_import( rsa, &T, NULL, NULL,
761+
NULL, NULL ) ) != 0 )
737762
goto cleanup;
738-
p += len;
739763

740764
/* Import E */
741-
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
742-
MBEDTLS_ASN1_INTEGER ) ) != 0 ||
743-
( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, NULL, 0,
744-
NULL, 0, p, len ) ) != 0 )
765+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
766+
( ret = mbedtls_rsa_import( rsa, NULL, NULL, NULL,
767+
NULL, &T ) ) != 0 )
745768
goto cleanup;
746-
p += len;
747769

748770
/* Import D */
749-
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
750-
MBEDTLS_ASN1_INTEGER ) ) != 0 ||
751-
( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, NULL, 0,
752-
p, len, NULL, 0 ) ) != 0 )
771+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
772+
( ret = mbedtls_rsa_import( rsa, NULL, NULL, NULL,
773+
&T, NULL ) ) != 0 )
753774
goto cleanup;
754-
p += len;
755775

756776
/* Import P */
757-
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
758-
MBEDTLS_ASN1_INTEGER ) ) != 0 ||
759-
( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, p, len, NULL, 0,
760-
NULL, 0, NULL, 0 ) ) != 0 )
777+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
778+
( ret = mbedtls_rsa_import( rsa, NULL, &T, NULL,
779+
NULL, NULL ) ) != 0 )
761780
goto cleanup;
762-
p += len;
763781

764782
/* Import Q */
765-
if( ( ret = mbedtls_asn1_get_tag( &p, end, &len,
766-
MBEDTLS_ASN1_INTEGER ) ) != 0 ||
767-
( ret = mbedtls_rsa_import_raw( rsa, NULL, 0, NULL, 0, p, len,
768-
NULL, 0, NULL, 0 ) ) != 0 )
783+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
784+
( ret = mbedtls_rsa_import( rsa, NULL, NULL, &T,
785+
NULL, NULL ) ) != 0 )
769786
goto cleanup;
770-
p += len;
771787

772-
#if !defined(MBEDTLS_RSA_NO_CRT)
788+
#if !defined(MBEDTLS_RSA_NO_CRT) && !defined(MBEDTLS_RSA_ALT)
773789
/*
774790
* The RSA CRT parameters DP, DQ and QP are nominally redundant, in
775791
* that they can be easily recomputed from D, P and Q. However by
@@ -782,28 +798,42 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa,
782798
*/
783799

784800
/* Import DP */
785-
if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0)
801+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
802+
( ret = mbedtls_mpi_copy( &rsa->DP, &T ) ) != 0 )
786803
goto cleanup;
787804

788805
/* Import DQ */
789-
if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DQ ) ) != 0)
806+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
807+
( ret = mbedtls_mpi_copy( &rsa->DQ, &T ) ) != 0 )
790808
goto cleanup;
791809

792810
/* Import QP */
793-
if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->QP ) ) != 0)
811+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
812+
( ret = mbedtls_mpi_copy( &rsa->QP, &T ) ) != 0 )
794813
goto cleanup;
795814

796815
#else
797816
/* Verify existance of the CRT params */
798-
if( ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ||
799-
( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ||
800-
( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 )
817+
if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
818+
( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||
819+
( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 )
801820
goto cleanup;
802821
#endif
803822

804-
/* Complete the RSA private key */
805-
if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 )
823+
/* rsa_complete() doesn't complete anything with the default
824+
* implementation but is still called:
825+
* - for the benefit of alternative implementation that may want to
826+
* pre-compute stuff beyond what's provided (eg Montgomery factors)
827+
* - as is also sanity-checks the key
828+
*
829+
* Furthermore, we also check the public part for consistency with
830+
* mbedtls_pk_parse_pubkey(), as it includes size minima for example.
831+
*/
832+
if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ||
833+
( ret = mbedtls_rsa_check_pubkey( rsa ) ) != 0 )
834+
{
806835
goto cleanup;
836+
}
807837

808838
if( p != end )
809839
{

‎tests/suites/test_suite_pkparse.data

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,33 +1072,84 @@ Parse EC Key #15 (SEC1 DER, secp256k1, SpecifiedECDomain)
10721072
depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED
10731073
pk_parse_keyfile_ec:"data_files/ec_prv.specdom.der":"NULL":0
10741074

1075-
Key ASN1 (Incorrect first tag)
1076-
pk_parse_key:"":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1075+
Key ASN1 (No data)
1076+
pk_parse_key:"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1077+
1078+
Key ASN1 (First tag not Sequence)
1079+
pk_parse_key:"020100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10771080

10781081
Key ASN1 (RSAPrivateKey, incorrect version tag)
10791082
depends_on:MBEDTLS_RSA_C
1080-
pk_parse_key:"300100":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1083+
pk_parse_key:"300100":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10811084

10821085
Key ASN1 (RSAPrivateKey, version tag missing)
10831086
depends_on:MBEDTLS_RSA_C
1084-
pk_parse_key:"3000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1087+
pk_parse_key:"3000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10851088

10861089
Key ASN1 (RSAPrivateKey, invalid version)
10871090
depends_on:MBEDTLS_RSA_C
1088-
pk_parse_key:"3003020101":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1091+
pk_parse_key:"3003020101":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10891092

10901093
Key ASN1 (RSAPrivateKey, correct version, incorrect tag)
10911094
depends_on:MBEDTLS_RSA_C
1092-
pk_parse_key:"300402010000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1095+
pk_parse_key:"300402010000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1096+
1097+
Key ASN1 (RSAPrivateKey, correct format+values, minimal modulus size (128 bit))
1098+
depends_on:MBEDTLS_RSA_C
1099+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":0
1100+
1101+
Key ASN1 (RSAPrivateKey, correct format, modulus too small (127 bit))
1102+
depends_on:MBEDTLS_RSA_C
1103+
pk_parse_key:"30630201000211007c8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1104+
1105+
Key ASN1 (RSAPrivateKey, correct format, modulus even)
1106+
depends_on:MBEDTLS_RSA_C
1107+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857002030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1108+
1109+
Key ASN1 (RSAPrivateKey, correct format, d == 0)
1110+
depends_on:MBEDTLS_RSA_C
1111+
pk_parse_key:"30630201000211007c8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1112+
1113+
Key ASN1 (RSAPrivateKey, correct format, d == p == q == 0)
1114+
depends_on:MBEDTLS_RSA_C
1115+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900000000000000000002090000000000000000000209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1116+
1117+
Key ASN1 (RSAPrivateKey, correct values, trailing garbage)
1118+
depends_on:MBEDTLS_RSA_C
1119+
pk_parse_key:"3064020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c00":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1120+
1121+
Key ASN1 (RSAPrivateKey, correct values, n wrong tag)
1122+
depends_on:MBEDTLS_RSA_C
1123+
pk_parse_key:"3063020100FF1100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1124+
1125+
Key ASN1 (RSAPrivateKey, correct values, e wrong tag)
1126+
depends_on:MBEDTLS_RSA_C
1127+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571FF030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1128+
1129+
Key ASN1 (RSAPrivateKey, correct values, d wrong tag)
1130+
depends_on:MBEDTLS_RSA_C
1131+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c85710203010001FF11009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1132+
1133+
Key ASN1 (RSAPrivateKey, correct values, p wrong tag)
1134+
depends_on:MBEDTLS_RSA_C
1135+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201FF0900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1136+
1137+
Key ASN1 (RSAPrivateKey, correct values, q wrong tag)
1138+
depends_on:MBEDTLS_RSA_C
1139+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61FF0900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1140+
1141+
Key ASN1 (RSAPrivateKey, correct values, dp wrong tag)
1142+
depends_on:MBEDTLS_RSA_C
1143+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a211FF09009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10931144

1094-
Key ASN1 (RSAPrivateKey, values present, length mismatch)
1145+
Key ASN1 (RSAPrivateKey, correct values, dq wrong tag)
10951146
depends_on:MBEDTLS_RSA_C
1096-
pk_parse_key:"301c02010002010102010102010102010102010102010102010102010100":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1147+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401FF0813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
10971148

1098-
Key ASN1 (RSAPrivateKey, values present, check_privkey fails)
1149+
Key ASN1 (RSAPrivateKey, correct values, qp wrong tag)
10991150
depends_on:MBEDTLS_RSA_C
1100-
pk_parse_key:"301b020100020102020101020101020101020101020101020101020101":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1151+
pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c857102030100010211009a6318982a7231de1894c54aa4909201020900f3058fd8dc484d61020900d7770dbd8b78a2110209009471f14c26428401020813425f060c4b7221FF08052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
11011152

11021153
Key ASN1 (ECPrivateKey, empty parameters)
11031154
depends_on:MBEDTLS_ECP_C
1104-
pk_parse_key:"30070201010400a000":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
1155+
pk_parse_key:"30070201010400a000":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT

‎tests/suites/test_suite_pkparse.function

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -113,23 +113,14 @@ exit:
113113
}
114114
/* END_CASE */
115115

116-
/* BEGIN_CASE depends_on:MBEDTLS_RSA_C */
117-
void pk_parse_key( data_t * buf, char * result_str, int result )
116+
/* BEGIN_CASE */
117+
void pk_parse_key( data_t * buf, int result )
118118
{
119119
mbedtls_pk_context pk;
120-
unsigned char output[2000];
121-
((void) result_str);
122120

123121
mbedtls_pk_init( &pk );
124122

125-
memset( output, 0, 2000 );
126-
127-
128-
TEST_ASSERT( mbedtls_pk_parse_key( &pk, buf->x, buf->len, NULL, 0 ) == ( result ) );
129-
if( ( result ) == 0 )
130-
{
131-
TEST_ASSERT( 1 );
132-
}
123+
TEST_ASSERT( mbedtls_pk_parse_key( &pk, buf->x, buf->len, NULL, 0 ) == result );
133124

134125
exit:
135126
mbedtls_pk_free( &pk );

0 commit comments

Comments
 (0)