From ea7eab1fde7269de70d328e4383f6bd406824a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 12 Nov 2019 10:31:12 +0100 Subject: [PATCH] Add redundancy (Hamming distance) to cert flags Before this commit, if a certificate only had one issue (for example, if the "untrusted" bit was the only set in flags), an attacker that could flip this single bit between the moment it's set and the moment flags are checked before returning from mbedtls_x509_crt_verify() could make the entire verification routine appear to succeed (return 0 with no bit set in flags). Avoid that by making sure that flags always has either 0 or at least 9 bits set during the execution of the function. However, to preserve the API, clear the 8 extra bits before returning. This doesn't open the door to other attacks, as fortunately the API already had redundancy: either both flags and the return value are 0, or flags has bits set and the return value is non-zero with at least 16 bits set (assuming 32-bit 2-complement ints). --- include/mbedtls/x509.h | 2 ++ library/x509_crt.c | 45 +++++++++++++++++++++++++++++++----------- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h index c31847d92..b2ad182f2 100644 --- a/include/mbedtls/x509.h +++ b/include/mbedtls/x509.h @@ -85,6 +85,8 @@ * \{ */ /* Reminder: update x509_crt_verify_strings[] in library/x509_crt.c */ +/* Reminder: update X509_BADCERT_FI_EXTRA in library/x509_crt.c if using more + * that 24 bits */ #define MBEDTLS_X509_BADCERT_EXPIRED 0x01 /**< The certificate validity has expired. */ #define MBEDTLS_X509_BADCERT_REVOKED 0x02 /**< The certificate has been revoked (is on a CRL). */ #define MBEDTLS_X509_BADCERT_CN_MISMATCH 0x04 /**< The certificate Common Name (CN) does not match with the expected CN. */ diff --git a/library/x509_crt.c b/library/x509_crt.c index c0914fa20..6b4ed1c1f 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -3328,6 +3328,23 @@ static unsigned x509_crt_verify_chain_len( #endif /* MBEDTLS_X509_REMOVE_VERIFY_CALLBACK */ +/* + * This is used in addition to the flag for a specific issue, to ensure that + * it is not possible for an active physical attacker to entirely clear the + * flags just by flipping a single bit. Take advantage of the fact that all + * values defined in include/mbedtls/x509.h so far are 24-bit or less, so the + * top byte is free. + * + * Currently this protection is not compatible with the vrfy callback (as it + * can observ and modify flags freely), so it's only enabled when the callback + * is disabled. + */ +#if defined(MBEDTLS_X509_REMOVE_VERIFY_CALLBACK) +#define X509_BADCERT_FI_EXTRA 0xff000000u +#else +#define X509_BADCERT_FI_EXTRA 0u +#endif + /* * Build and verify a certificate chain * @@ -3434,9 +3451,9 @@ find_parent: #if !defined(MBEDTLS_X509_CRT_REMOVE_TIME) /* Check time-validity (all certificates) */ if( mbedtls_x509_time_is_past( &child->valid_to ) ) - *flags |= MBEDTLS_X509_BADCERT_EXPIRED; + *flags |= MBEDTLS_X509_BADCERT_EXPIRED | X509_BADCERT_FI_EXTRA; if( mbedtls_x509_time_is_future( &child->valid_from ) ) - *flags |= MBEDTLS_X509_BADCERT_FUTURE; + *flags |= MBEDTLS_X509_BADCERT_FUTURE | X509_BADCERT_FI_EXTRA; #endif /* !MBEDTLS_X509_CRT_REMOVE_TIME */ /* Stop here for trusted roots (but not for trusted EE certs) */ @@ -3456,10 +3473,10 @@ find_parent: /* Check signature algorithm: MD & PK algs */ if( x509_profile_check_md_alg( profile, child->sig_md ) != 0 ) - *flags |= MBEDTLS_X509_BADCERT_BAD_MD; + *flags |= MBEDTLS_X509_BADCERT_BAD_MD | X509_BADCERT_FI_EXTRA; if( x509_profile_check_pk_alg( profile, child->sig_pk ) != 0 ) - *flags |= MBEDTLS_X509_BADCERT_BAD_PK; + *flags |= MBEDTLS_X509_BADCERT_BAD_PK | X509_BADCERT_FI_EXTRA; /* Special case: EE certs that are locally trusted */ if( x509_crt_verify_chain_len( ver_chain ) == 1 && self_issued && @@ -3507,7 +3524,7 @@ find_parent: /* No parent? We're done here */ if( parent_crt == NULL ) { - *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED | X509_BADCERT_FI_EXTRA; return( 0 ); } @@ -3531,11 +3548,11 @@ find_parent: signature_is_good_fi = signature_is_good; if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) { - *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED | X509_BADCERT_FI_EXTRA; mbedtls_platform_enforce_volatile_reads(); if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) - *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED | X509_BADCERT_FI_EXTRA; } { @@ -3546,7 +3563,7 @@ find_parent: /* check size of signing key */ if( x509_profile_check_key( profile, parent_pk ) != 0 ) - *flags |= MBEDTLS_X509_BADCERT_BAD_KEY; + *flags |= MBEDTLS_X509_BADCERT_BAD_KEY | X509_BADCERT_FI_EXTRA; mbedtls_x509_crt_pk_release( parent_crt ); } @@ -3677,7 +3694,7 @@ static int x509_crt_verify_name( const mbedtls_x509_crt *crt, if( ret != 0 ) ret = MBEDTLS_ERR_X509_FATAL_ERROR; - *flags |= MBEDTLS_X509_BADCERT_CN_MISMATCH; + *flags |= MBEDTLS_X509_BADCERT_CN_MISMATCH | X509_BADCERT_FI_EXTRA; return( ret ); } #endif /* !MBEDTLS_X509_REMOVE_HOSTNAME_VERIFICATION */ @@ -3799,10 +3816,10 @@ int mbedtls_x509_crt_verify_restartable( mbedtls_x509_crt *crt, pk_type = mbedtls_pk_get_type( pk ); if( x509_profile_check_pk_alg( profile, pk_type ) != 0 ) - ee_flags |= MBEDTLS_X509_BADCERT_BAD_PK; + ee_flags |= MBEDTLS_X509_BADCERT_BAD_PK | X509_BADCERT_FI_EXTRA; if( x509_profile_check_key( profile, pk ) != 0 ) - ee_flags |= MBEDTLS_X509_BADCERT_BAD_KEY; + ee_flags |= MBEDTLS_X509_BADCERT_BAD_KEY | X509_BADCERT_FI_EXTRA; mbedtls_x509_crt_pk_release( crt ); } @@ -3843,7 +3860,13 @@ exit: } if( *flags != 0 ) + { + /* Preserve the API by removing internal extra bits - from now on the + * fact that flags is non-zero is also redundantly encoded by the + * return value from this function. */ + *flags &= ~ X509_BADCERT_FI_EXTRA; return( MBEDTLS_ERR_X509_CERT_VERIFY_FAILED ); + } return( 0 ); }