From d1e55dfce66638659a4cec1ba17fc63ba314a519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 8 Nov 2019 11:02:56 +0100 Subject: [PATCH 1/9] Add double check on cert signature verification x509_crt_check_signature() directly returns the return value of pk_verify_xxx() without looking at it, so nothing to do here. But its caller compares the value to 0, which ought to be double-checked. --- library/x509_crt.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index e53798353..e1e98df52 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2938,6 +2938,7 @@ static int x509_crt_find_parent_in( mbedtls_x509_crt_restart_ctx *rs_ctx ) { int ret; + volatile int ret_fi; mbedtls_x509_crt *parent_crt; int signature_is_good; @@ -3018,10 +3019,10 @@ check_signature: continue; /* Signature */ - ret = x509_crt_check_signature( child_sig, parent_crt, rs_ctx ); + ret_fi = x509_crt_check_signature( child_sig, parent_crt, rs_ctx ); #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) - if( rs_ctx != NULL && ret == MBEDTLS_ERR_ECP_IN_PROGRESS ) + if( rs_ctx != NULL && ret_fi == MBEDTLS_ERR_ECP_IN_PROGRESS ) { /* save state */ rs_ctx->parent = parent_crt; @@ -3030,13 +3031,18 @@ check_signature: rs_ctx->fallback_signature_is_good = fallback_signature_is_good; #endif /* MBEDTLS_HAVE_TIME_DATE */ - return( ret ); + return( ret_fi ); } -#else - (void) ret; #endif - signature_is_good = ret == 0; + signature_is_good = 0; + if( ret_fi == 0 ) + { + mbedtls_platform_enforce_volatile_reads(); + if( ret_fi == 0 ) + signature_is_good = 1; + } + if( top && ! signature_is_good ) continue; From f66657ac44ccf9ab983042831db342631adfe1d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 8 Nov 2019 11:14:09 +0100 Subject: [PATCH 2/9] Use large Hamming distance for signature validity If signature_is_good is 0 (invalid) of 1 (valid), then it's all too easy for an active physical attacker to turn invalid into valid by flipping a single bit in RAM, on the bus or in a CPU register. Use a special value to represent "valid" that can't easily be reached by flipping a few bits. --- library/x509_crt.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index e1e98df52..d75e304ae 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2884,6 +2884,10 @@ static int x509_crt_check_parent( const mbedtls_x509_crt_sig_info *sig_info, return( 0 ); } +/* This value is different enough from 0 that it's hard for an active physical + * attacker to reach it just by flipping a few bits. */ +#define X509_SIGNATURE_IS_GOOD 0x7f5a5a5a + /* * Find a suitable parent for child in candidates, or return NULL. * @@ -2915,7 +2919,8 @@ static int x509_crt_check_parent( const mbedtls_x509_crt_sig_info *sig_info, * - [in] child: certificate for which we're looking for a parent * - [in] candidates: chained list of potential parents * - [out] r_parent: parent found (or NULL) - * - [out] r_signature_is_good: 1 if child signature by parent is valid, or 0 + * - [out] r_signature_is_good: set to X509_SIGNATURE_IS_GOOD if + * child signature by parent is valid, or to 0 * - [in] top: 1 if candidates consists of trusted roots, ie we're at the top * of the chain, 0 otherwise * - [in] path_cnt: number of intermediates seen so far @@ -3040,7 +3045,7 @@ check_signature: { mbedtls_platform_enforce_volatile_reads(); if( ret_fi == 0 ) - signature_is_good = 1; + signature_is_good = X509_SIGNATURE_IS_GOOD; } if( top && ! signature_is_good ) @@ -3522,7 +3527,7 @@ find_parent: } /* signature was checked while searching parent */ - if( ! signature_is_good ) + if( signature_is_good != X509_SIGNATURE_IS_GOOD ) *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; { From 81c1fc41327bae26fe494a6d9c25df090c41e593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 8 Nov 2019 11:25:16 +0100 Subject: [PATCH 3/9] Add double check on bad signature flagging --- library/x509_crt.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index d75e304ae..c0914fa20 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -3385,6 +3385,7 @@ static int x509_crt_verify_chain( int parent_is_trusted; int child_is_trusted; int signature_is_good; + volatile int signature_is_good_fi; unsigned self_cnt; #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) @@ -3527,9 +3528,16 @@ find_parent: } /* signature was checked while searching parent */ - if( signature_is_good != X509_SIGNATURE_IS_GOOD ) + signature_is_good_fi = signature_is_good; + if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) + { *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + mbedtls_platform_enforce_volatile_reads(); + if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED; + } + { mbedtls_pk_context *parent_pk; ret = mbedtls_x509_crt_pk_acquire( parent_crt, &parent_pk ); 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 4/9] 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 ); } From 4c9b556e385d8723a1dc21fd3e9973eab97441fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 12 Nov 2019 10:45:32 +0100 Subject: [PATCH 5/9] Add double-check for flags == 0 in crt_verify() Also move to "default flow assumes failure" while at it. --- library/x509_crt.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index 6b4ed1c1f..22a6a23a1 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -3783,6 +3783,7 @@ int mbedtls_x509_crt_verify_restartable( mbedtls_x509_crt *crt, int ret; mbedtls_x509_crt_verify_chain ver_chain; uint32_t ee_flags; + volatile uint32_t flags_fi; *flags = 0; ee_flags = 0; @@ -3859,16 +3860,19 @@ exit: return( ret ); } - if( *flags != 0 ) + flags_fi = *flags; + if( flags_fi == 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 ); + mbedtls_platform_enforce_volatile_reads(); + if( flags_fi == 0 ) + return( 0 ); } - return( 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 + * non-zero return value from this function. */ + *flags &= ~ X509_BADCERT_FI_EXTRA; + return( MBEDTLS_ERR_X509_CERT_VERIFY_FAILED ); } /* From 18761926a82632301b14f0ff13c5805eec766c21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 14 Nov 2019 09:19:08 +0100 Subject: [PATCH 6/9] Fix double-check for bad signature In the previous version, it was enough for the attacker to glitch the top-level 'if' to skip the entire block. We want two independent blocks here, so that an attacker can only succeed with two successive glitches. --- library/x509_crt.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index 22a6a23a1..fde6843b5 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -3547,13 +3547,11 @@ find_parent: /* signature was checked while searching parent */ signature_is_good_fi = signature_is_good; if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) - { *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 | X509_BADCERT_FI_EXTRA; - } + mbedtls_platform_enforce_volatile_reads(); + if( signature_is_good_fi != X509_SIGNATURE_IS_GOOD ) + *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED | X509_BADCERT_FI_EXTRA; { mbedtls_pk_context *parent_pk; From 6bdc6809dabf6f18e55bce972a1c0f289a60427c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 28 Nov 2019 10:29:41 +0100 Subject: [PATCH 7/9] Initialise variables to failing values --- library/x509_crt.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index fde6843b5..b90ec72e8 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2943,9 +2943,9 @@ static int x509_crt_find_parent_in( mbedtls_x509_crt_restart_ctx *rs_ctx ) { int ret; - volatile int ret_fi; + volatile int ret_fi = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; mbedtls_x509_crt *parent_crt; - int signature_is_good; + int signature_is_good = 0; #if defined(MBEDTLS_HAVE_TIME_DATE) mbedtls_x509_crt *fallback_parent; @@ -3040,7 +3040,6 @@ check_signature: } #endif - signature_is_good = 0; if( ret_fi == 0 ) { mbedtls_platform_enforce_volatile_reads(); @@ -3781,7 +3780,7 @@ int mbedtls_x509_crt_verify_restartable( mbedtls_x509_crt *crt, int ret; mbedtls_x509_crt_verify_chain ver_chain; uint32_t ee_flags; - volatile uint32_t flags_fi; + volatile uint32_t flags_fi = -1u; *flags = 0; ee_flags = 0; From 9ca11fc8929790f5a600ec062c82c57d9eac66ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 28 Nov 2019 12:07:01 +0100 Subject: [PATCH 8/9] Fix issues found by the CI - MSVC doesn't like -1u - We need to include platform.h for MBEDTLS_ERR_PLATFORM_FAULT_DETECTED - in some configurations it was already included indirectly, but not in all configurations, so better include it directly. --- library/x509_crt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index b90ec72e8..fd3fa1a04 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -43,6 +43,7 @@ #include "mbedtls/x509_internal.h" #include "mbedtls/oid.h" #include "mbedtls/platform_util.h" +#include "mbedtls/platform.h" #include @@ -3780,7 +3781,7 @@ int mbedtls_x509_crt_verify_restartable( mbedtls_x509_crt *crt, int ret; mbedtls_x509_crt_verify_chain ver_chain; uint32_t ee_flags; - volatile uint32_t flags_fi = -1u; + volatile uint32_t flags_fi = (uint32_t) -1; *flags = 0; ee_flags = 0; From 65be6b48dee73760fdee5213b74f3eab0ddfb7c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 28 Nov 2019 12:51:45 +0100 Subject: [PATCH 9/9] Add compile guard in platform.h We may want to include it just for the errors it defines without having all of the other defines that only make sense when PLATFORM_C is enabled. --- include/mbedtls/platform.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/platform.h b/include/mbedtls/platform.h index 82d5e3355..212c26720 100644 --- a/include/mbedtls/platform.h +++ b/include/mbedtls/platform.h @@ -39,14 +39,16 @@ #include MBEDTLS_CONFIG_FILE #endif -#if defined(MBEDTLS_HAVE_TIME) -#include "platform_time.h" -#endif - #define MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED -0x0070 /**< Hardware accelerator failed */ #define MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED -0x0072 /**< The requested feature is not supported by the platform */ #define MBEDTLS_ERR_PLATFORM_FAULT_DETECTED -0x0071 /**< A fault was detected in a critical path, likely indicative of an active physical attack */ +#if defined(MBEDTLS_PLATFORM_C) + +#if defined(MBEDTLS_HAVE_TIME) +#include "platform_time.h" +#endif + #ifdef __cplusplus extern "C" { #endif @@ -365,4 +367,6 @@ void mbedtls_platform_teardown( mbedtls_platform_context *ctx ); } #endif +#endif /* MBEDTLS_PLATFORM_C */ + #endif /* platform.h */