From 65eefc8707059ca905aaf46e249b5f6ebdf927ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 23 Oct 2015 14:08:48 +0200 Subject: [PATCH] Fix missing check for RSA key length on EE certs - also adapt tests to use lesser requirement for compatibility with old testing material --- ChangeLog | 8 ++++++++ include/mbedtls/ssl.h | 18 ++++++++++-------- include/mbedtls/x509_crt.h | 4 ++-- library/x509_crt.c | 16 ++++++++++++++-- tests/suites/test_suite_x509parse.data | 4 ++-- tests/suites/test_suite_x509parse.function | 19 ++++++++++++++++++- 6 files changed, 54 insertions(+), 15 deletions(-) diff --git a/ChangeLog b/ChangeLog index aa96b1848..d0ec0312a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,13 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.2.0 released 2015-10-06 + +Bugfix + * mbedtls_x509_crt_verify(_with_profile)() now also checks the key type and + size/curve against the profile. Before that, there was no way to set a + minimum key size for end-entity certificates with RSA keys. Found by + Matthew Page. + = mbed TLS 2.1.2 released 2015-10-06 Security diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index a017ec0b1..72035b80a 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1385,6 +1385,10 @@ void mbedtls_ssl_conf_ciphersuites_for_version( mbedtls_ssl_config *conf, /** * \brief Set the X.509 security profile used for verification * + * \note The restrictions are enforced for all certificates in the + * chain. However, signatures in the handshake are not covered + * by this setting but by \b mbedtls_ssl_conf_sig_hashes(). + * * \param conf SSL configuration * \param profile Profile to use */ @@ -1546,16 +1550,14 @@ void mbedtls_ssl_conf_dhm_min_bitlen( mbedtls_ssl_config *conf, * On client: this affects the list of curves offered for any * use. The server can override our preference order. * - * Both sides: limits the set of curves used by peer to the - * listed curves for any use ECDHE and the end-entity - * certificate. + * Both sides: limits the set of curves accepted for use in + * ECDHE and in the peer's end-entity certificate. * - * \note This has no influence on which curve are allowed inside the + * \note This has no influence on which curves are allowed inside the * certificate chains, see \c mbedtls_ssl_conf_cert_profile() - * for that. For example, if the peer's certificate chain is - * EE -> CA_int -> CA_root, then the allowed curves for EE are - * controlled by \c mbedtls_ssl_conf_curves() but for CA_int - * and CA_root it's \c mbedtls_ssl_conf_cert_profile(). + * for that. For the end-entity certificate however, the key + * will be accepted only if it is allowed both by this list + * and by the cert profile. * * \note This list should be ordered by decreasing preference * (preferred curve first). diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 72b02ffd3..294f36a28 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -301,8 +301,8 @@ int mbedtls_x509_crt_verify( mbedtls_x509_crt *crt, * security profile. * * \note The restrictions on keys (RSA minimum size, allowed curves - * for ECDSA) only applys to (intermediate) CAs, not to the - * end-entity certificate. + * for ECDSA) apply to all certificates: trusted root, + * intermediate CAs if any, and end entity certificate. * * \param crt a certificate to be verified * \param trust_ca the trusted CA chain diff --git a/library/x509_crt.c b/library/x509_crt.c index f6879ddcf..14e5d94cd 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -186,8 +186,10 @@ static int x509_profile_check_key( const mbedtls_x509_crt_profile *profile, } #endif -#if defined(MBEDTLS_ECDSA_C) - if( pk_alg == MBEDTLS_PK_ECDSA ) +#if defined(MBEDTLS_ECP_C) + if( pk_alg == MBEDTLS_PK_ECDSA || + pk_alg == MBEDTLS_PK_ECKEY || + pk_alg == MBEDTLS_PK_ECKEY_DH ) { mbedtls_ecp_group_id gid = mbedtls_pk_ec( *pk )->grp.id; @@ -2151,6 +2153,7 @@ int mbedtls_x509_crt_verify_with_profile( mbedtls_x509_crt *crt, mbedtls_x509_crt *parent; mbedtls_x509_name *name; mbedtls_x509_sequence *cur = NULL; + mbedtls_pk_type_t pk_type; if( profile == NULL ) return( MBEDTLS_ERR_X509_BAD_INPUT_DATA ); @@ -2209,6 +2212,15 @@ int mbedtls_x509_crt_verify_with_profile( mbedtls_x509_crt *crt, } } + /* Check the type and size of the key */ + pk_type = mbedtls_pk_get_type( &crt->pk ); + + if( x509_profile_check_pk_alg( profile, pk_type ) != 0 ) + *flags |= MBEDTLS_X509_BADCERT_BAD_PK; + + if( x509_profile_check_key( profile, pk_type, &crt->pk ) != 0 ) + *flags |= MBEDTLS_X509_BADCERT_BAD_KEY; + /* Look for a parent in trusted CAs */ for( parent = trust_ca; parent != NULL; parent = parent->next ) { diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index e25258ff8..eb4df8f9c 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -673,7 +673,7 @@ x509_verify:"data_files/server6-ss-child.crt":"data_files/server5-selfsigned.crt X509 Certificate verification #75 (encoding mismatch) depends_on:MBEDTLS_PEM_PARSE_C -x509_verify:"data_files/enco-cert-utf8str.pem":"data_files/enco-ca-prstr.pem":"data_files/crl.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_BAD_KEY:"NULL" +x509_verify:"data_files/enco-cert-utf8str.pem":"data_files/enco-ca-prstr.pem":"data_files/crl.pem":"NULL":0:0:"NULL" X509 Certificate verification #76 (multiple CRLs, not revoked) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C:MBEDTLS_RSA_C @@ -697,7 +697,7 @@ x509_verify:"data_files/server1.crt":"data_files/test-ca_cat12.crt":"data_files/ X509 Certificate verification #81 (multiple CRLs, none relevant) depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP384R1_ENABLED:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C:MBEDTLS_RSA_C -x509_verify:"data_files/enco-cert-utf8str.pem":"data_files/enco-ca-prstr.pem":"data_files/crl_cat_rsa-ec.pem":"NULL":MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:MBEDTLS_X509_BADCERT_BAD_KEY:"NULL" +x509_verify:"data_files/enco-cert-utf8str.pem":"data_files/enco-ca-prstr.pem":"data_files/crl_cat_rsa-ec.pem":"NULL":0:0:"NULL" X509 Certificate verification callback: trusted EE cert depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_SHA256_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 95fc287be..7cd4eb6fa 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -6,6 +6,19 @@ #include "mbedtls/oid.h" #include "mbedtls/base64.h" +const mbedtls_x509_crt_profile compat_profile = +{ + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) | + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) | + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) | + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) | + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) | + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ), + 0xFFFFFFF, /* Any PK alg */ + 0xFFFFFFF, /* Any curve */ + 1024, +}; + int verify_none( void *data, mbedtls_x509_crt *crt, int certificate_depth, uint32_t *flags ) { ((void) data); @@ -191,7 +204,11 @@ void x509_verify( char *crt_file, char *ca_file, char *crl_file, TEST_ASSERT( mbedtls_x509_crt_parse_file( &ca, ca_file ) == 0 ); TEST_ASSERT( mbedtls_x509_crl_parse_file( &crl, crl_file ) == 0 ); - res = mbedtls_x509_crt_verify( &crt, &ca, &crl, cn_name, &flags, f_vrfy, NULL ); + //puts( "" ); + res = mbedtls_x509_crt_verify_with_profile( &crt, &ca, &crl, &compat_profile, cn_name, &flags, f_vrfy, NULL ); + + //printf( "exp: -%04x, %08x\n", result, flags_result ); + //printf( "got: -%04x, %08x\n", res, flags ); TEST_ASSERT( res == ( result ) ); TEST_ASSERT( flags == (uint32_t)( flags_result ) );