Merge remote-tracking branch 'public/pr/2932' into baremetal
diff --git a/include/mbedtls/error.h b/include/mbedtls/error.h
index 31f294f..a52f9f5 100644
--- a/include/mbedtls/error.h
+++ b/include/mbedtls/error.h
@@ -86,7 +86,7 @@
* CHACHA20 3 0x0051-0x0055
* POLY1305 3 0x0057-0x005B
* CHACHAPOLY 2 0x0054-0x0056
- * PLATFORM 1 0x0070-0x0072
+ * PLATFORM 3 0x0070-0x0072 0x0071-0x0071
*
* High-level module nr (3 bits - 0x0...-0x7...)
* Name ID Nr of Errors
diff --git a/include/mbedtls/platform.h b/include/mbedtls/platform.h
index 89fe8a7..ec1df15 100644
--- a/include/mbedtls/platform.h
+++ b/include/mbedtls/platform.h
@@ -39,13 +39,16 @@
#include MBEDTLS_CONFIG_FILE
#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 hardware fault was detected in a critical path. As a security precaution this should be treated as a potential physical attack */
+
+#if defined(MBEDTLS_PLATFORM_C)
+
#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 */
-
#ifdef __cplusplus
extern "C" {
#endif
@@ -364,4 +367,6 @@
}
#endif
+#endif /* MBEDTLS_PLATFORM_C */
+
#endif /* platform.h */
diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h
index 586f0d9..e20f1c3 100644
--- a/include/mbedtls/platform_util.h
+++ b/include/mbedtls/platform_util.h
@@ -238,6 +238,13 @@
*/
uint32_t mbedtls_platform_random_in_range( size_t num );
+/**
+ * \brief This function does nothing, but can be inserted between
+ * successive reads to a volatile local variable to prevent
+ * compilers from optimizing them away.
+ */
+void mbedtls_platform_enforce_volatile_reads( void );
+
#if defined(MBEDTLS_HAVE_TIME_DATE)
/**
* \brief Platform-specific implementation of gmtime_r()
diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h
index c31847d..b2ad182 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/include/tinycrypt/ecc.h b/include/tinycrypt/ecc.h
index e4435fd..df8f1df 100644
--- a/include/tinycrypt/ecc.h
+++ b/include/tinycrypt/ecc.h
@@ -82,6 +82,13 @@
extern "C" {
#endif
+/* Return values for functions, chosen with large Hamming distances between
+ * them (especially to SUCESS) to mitigate the impact of fault injection
+ * attacks flipping a low number of bits. */
+#define UECC_SUCCESS 0
+#define UECC_FAILURE 0x75555555
+#define UECC_ATTACK_DETECTED 0x7aaaaaaa
+
/* Word size (4 bytes considering 32-bits architectures) */
#define uECC_WORD_SIZE 4
@@ -414,7 +421,7 @@
* @param left IN -- left term in comparison
* @param right IN -- right term in comparison
* @param num_words IN -- number of words
- * @return Returns 0 if left == right, 1 otherwise.
+ * @return Returns 0 if left == right, non-zero otherwise.
*/
uECC_word_t uECC_vli_equal(const uECC_word_t *left, const uECC_word_t *right);
diff --git a/include/tinycrypt/ecc_dsa.h b/include/tinycrypt/ecc_dsa.h
index 896e20e..f744319 100644
--- a/include/tinycrypt/ecc_dsa.h
+++ b/include/tinycrypt/ecc_dsa.h
@@ -122,8 +122,8 @@
/**
* @brief Verify an ECDSA signature.
- * @return returns TC_SUCCESS (1) if the signature is valid
- * returns TC_FAIL (0) if the signature is invalid.
+ * @return returns UECC_SUCCESS if the signature is valid
+ * returns UECC_FAILURE if the signature is invalid.
*
* @param p_public_key IN -- The signer's public key.
* @param p_message_hash IN -- The hash of the signed data.
diff --git a/library/error.c b/library/error.c
index c993524..74c9d0b 100644
--- a/library/error.c
+++ b/library/error.c
@@ -841,6 +841,8 @@
mbedtls_snprintf( buf, buflen, "PLATFORM - Hardware accelerator failed" );
if( use_ret == -(MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED) )
mbedtls_snprintf( buf, buflen, "PLATFORM - The requested feature is not supported by the platform" );
+ if( use_ret == -(MBEDTLS_ERR_PLATFORM_FAULT_DETECTED) )
+ mbedtls_snprintf( buf, buflen, "PLATFORM - A hardware fault was detected in a critical path. As a security precaution this should be treated as a potential physical attack" );
#endif /* MBEDTLS_PLATFORM_C */
#if defined(MBEDTLS_POLY1305_C)
diff --git a/library/pk.c b/library/pk.c
index 29123eb..9eddb61 100644
--- a/library/pk.c
+++ b/library/pk.c
@@ -577,6 +577,7 @@
const unsigned char *sig, size_t sig_len )
{
int ret;
+ volatile int ret_fi;
uint8_t signature[2*NUM_ECC_BYTES];
unsigned char *p;
const struct uECC_Curve_t * uecc_curve = uECC_secp256r1();
@@ -589,12 +590,22 @@
if( ret != 0 )
return( ret );
- ret = uECC_verify( keypair->public_key, hash,
- (unsigned) hash_len, signature, uecc_curve );
- if( ret == 0 )
- return( MBEDTLS_ERR_PK_HW_ACCEL_FAILED );
+ ret_fi = uECC_verify( keypair->public_key, hash,
+ (unsigned) hash_len, signature, uecc_curve );
- return( 0 );
+ if( ret_fi == UECC_ATTACK_DETECTED )
+ return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED );
+
+ if( ret_fi == UECC_SUCCESS )
+ {
+ mbedtls_platform_enforce_volatile_reads();
+ if( ret_fi == UECC_SUCCESS )
+ return( 0 );
+ else
+ return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED );
+ }
+
+ return( MBEDTLS_ERR_PK_HW_ACCEL_FAILED );
}
/*
diff --git a/library/platform_util.c b/library/platform_util.c
index 1a0fefa..97dfe73 100644
--- a/library/platform_util.c
+++ b/library/platform_util.c
@@ -168,6 +168,15 @@
#endif
}
+/* Some compilers (armcc 5 for example) optimize away successive reads from a
+ * volatile local variable (which we use as a counter-measure to fault
+ * injection attacks), unless there is a call to an external function between
+ * them. This functions doesn't need to do anything, it just needs to be
+ * in another compilation unit. So here's a function that does nothing. */
+void mbedtls_platform_enforce_volatile_reads( void )
+{
+}
+
#if defined(MBEDTLS_HAVE_TIME_DATE) && !defined(MBEDTLS_PLATFORM_GMTIME_R_ALT)
#include <time.h>
#if !defined(_WIN32) && (defined(unix) || \
diff --git a/library/x509_crt.c b/library/x509_crt.c
index e537983..fd3fa1a 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 <string.h>
@@ -2884,6 +2885,10 @@
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 +2920,8 @@
* - [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
@@ -2938,8 +2944,9 @@
mbedtls_x509_crt_restart_ctx *rs_ctx )
{
int ret;
+ 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;
@@ -3018,10 +3025,10 @@
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 +3037,17 @@
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;
+ if( ret_fi == 0 )
+ {
+ mbedtls_platform_enforce_volatile_reads();
+ if( ret_fi == 0 )
+ signature_is_good = X509_SIGNATURE_IS_GOOD;
+ }
+
if( top && ! signature_is_good )
continue;
@@ -3318,6 +3329,23 @@
#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
*
* Given a peer-provided list of certificates EE, C1, ..., Cn and
@@ -3374,6 +3402,7 @@
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)
@@ -3422,9 +3451,9 @@
#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) */
@@ -3444,10 +3473,10 @@
/* 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 &&
@@ -3495,7 +3524,7 @@
/* 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 );
}
@@ -3516,8 +3545,13 @@
}
/* signature was checked while searching parent */
- if( ! signature_is_good )
- *flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED;
+ 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_pk_context *parent_pk;
@@ -3527,7 +3561,7 @@
/* 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 );
}
@@ -3658,7 +3692,7 @@
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 */
@@ -3747,6 +3781,7 @@
int ret;
mbedtls_x509_crt_verify_chain ver_chain;
uint32_t ee_flags;
+ volatile uint32_t flags_fi = (uint32_t) -1;
*flags = 0;
ee_flags = 0;
@@ -3780,10 +3815,10 @@
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 );
}
@@ -3823,10 +3858,19 @@
return( ret );
}
- if( *flags != 0 )
- return( MBEDTLS_ERR_X509_CERT_VERIFY_FAILED );
+ flags_fi = *flags;
+ if( flags_fi == 0 )
+ {
+ 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 );
}
/*
diff --git a/tests/suites/test_suite_tinycrypt.data b/tests/suites/test_suite_tinycrypt.data
index ac2a8e2..2c4d54b 100644
--- a/tests/suites/test_suite_tinycrypt.data
+++ b/tests/suites/test_suite_tinycrypt.data
@@ -8,4 +8,4 @@
ecdh_primitive_testvec:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8AFE84049C62A9C57862D1433":"DAD0B65394221CF9B051E1FECA5787D098DFE637FC90B9EF945D0C3772581180":"5271A0461CDB8252D61F1C456FA3E59AB1F45B33ACCF5F58389E0577B8990BB3":"C6EF9C5D78AE012A011164ACB397CE2088685D8F06BF9BE0B283AB46476BEE53":"D12DFB5289C8D4F81208B70270398C342296970A0BCCB74C736FC7554494BF63":"56FBF3CA366CC23E8157854C13C58D6AAC23F046ADA30F8353E74F33039872AB":"D6840F6B42F6EDAFD13116E0E12565202FEF8E9ECE7DCE03812464D04B9442DE"
ECDSA primitive rfc 4754 p256
-ecdsa_primitive_testvec:"2442A5CC0ECD015FA3CA31DC8E2BBC70BF42D60CBCA20085E0822CB04235E970":"6FC98BD7E50211A4A27102FA3549DF79EBCB4BF246B80945CDDFE7D509BBFD7D":"BA7816BF8F01CFEA414140DE5DAE2223B00361A396177A9CB410FF61F20015AD":"CB28E0999B9C7715FD0A80D8E47A77079716CBBF917DD72E97566EA1C066957C":"86FA3BB4E26CAD5BF90B7F81899256CE7594BB1EA0C89212748BFF3B3D5B0315":1
+ecdsa_primitive_testvec:"2442A5CC0ECD015FA3CA31DC8E2BBC70BF42D60CBCA20085E0822CB04235E970":"6FC98BD7E50211A4A27102FA3549DF79EBCB4BF246B80945CDDFE7D509BBFD7D":"BA7816BF8F01CFEA414140DE5DAE2223B00361A396177A9CB410FF61F20015AD":"CB28E0999B9C7715FD0A80D8E47A77079716CBBF917DD72E97566EA1C066957C":"86FA3BB4E26CAD5BF90B7F81899256CE7594BB1EA0C89212748BFF3B3D5B0315"
diff --git a/tests/suites/test_suite_tinycrypt.function b/tests/suites/test_suite_tinycrypt.function
index 24b331d..664cd08 100644
--- a/tests/suites/test_suite_tinycrypt.function
+++ b/tests/suites/test_suite_tinycrypt.function
@@ -55,7 +55,7 @@
TEST_ASSERT( uECC_sign( private, hash, sizeof( hash ), sig, curve ) != 0 );
- TEST_ASSERT( uECC_verify( public, hash, sizeof( hash ), sig, curve ) != 0 );
+ TEST_ASSERT( uECC_verify( public, hash, sizeof( hash ), sig, curve ) == UECC_SUCCESS );
}
/* END_CASE */
@@ -88,8 +88,7 @@
/* BEGIN_CASE depends_on:MBEDTLS_USE_TINYCRYPT */
void ecdsa_primitive_testvec( data_t * xQ_str, data_t * yQ_str,
- data_t * hash, data_t * r_str, data_t * s_str,
- int result )
+ data_t * hash, data_t * r_str, data_t * s_str )
{
const struct uECC_Curve_t * curve = uECC_secp256r1();
uint8_t pub_bytes[2*NUM_ECC_BYTES] = {0};
@@ -101,7 +100,7 @@
memcpy( sig_bytes + NUM_ECC_BYTES, s_str->x, r_str->len );
TEST_ASSERT( uECC_verify( pub_bytes, hash->x, hash->len,
- sig_bytes, curve ) == result );
+ sig_bytes, curve ) == UECC_SUCCESS );
// Alter the signature and check the verification fails
for( int i = 0; i < 2*NUM_ECC_BYTES; i++ )
@@ -109,7 +108,7 @@
uint8_t temp = sig_bytes[i];
sig_bytes[i] = ( sig_bytes[i] + 1 ) % 256;
TEST_ASSERT( uECC_verify( pub_bytes, hash->x, hash->len,
- sig_bytes, curve ) == 0 );
+ sig_bytes, curve ) == UECC_FAILURE );
sig_bytes[i] = temp;
}
diff --git a/tinycrypt/ecc.c b/tinycrypt/ecc.c
index 14fa582..38b11d9 100644
--- a/tinycrypt/ecc.c
+++ b/tinycrypt/ecc.c
@@ -184,7 +184,7 @@
for (i = NUM_ECC_WORDS - 1; i >= 0; --i) {
diff |= (left[i] ^ right[i]);
}
- return !(diff == 0);
+ return diff;
}
uECC_word_t cond_set(uECC_word_t p_true, uECC_word_t p_false, unsigned int cond)
diff --git a/tinycrypt/ecc_dsa.c b/tinycrypt/ecc_dsa.c
index b44fa37..67b33a4 100644
--- a/tinycrypt/ecc_dsa.c
+++ b/tinycrypt/ecc_dsa.c
@@ -66,6 +66,7 @@
#include <tinycrypt/ecc.h>
#include <tinycrypt/ecc_dsa.h>
+#include "mbedtls/platform_util.h"
#if default_RNG_defined
static uECC_RNG_Function g_rng_function = &default_CSPRNG;
@@ -213,6 +214,7 @@
const uECC_word_t *point;
bitcount_t num_bits;
bitcount_t i;
+ volatile uECC_word_t diff;
uECC_word_t _public[NUM_ECC_WORDS * 2];
uECC_word_t r[NUM_ECC_WORDS], s[NUM_ECC_WORDS];
@@ -234,13 +236,13 @@
/* r, s must not be 0. */
if (uECC_vli_isZero(r) || uECC_vli_isZero(s)) {
- return 0;
+ return UECC_FAILURE;
}
/* r, s must be < n. */
if (uECC_vli_cmp_unsafe(curve->n, r) != 1 ||
uECC_vli_cmp_unsafe(curve->n, s) != 1) {
- return 0;
+ return UECC_FAILURE;
}
/* Calculate u1 and u2. */
@@ -300,5 +302,16 @@
}
/* Accept only if v == r. */
- return (int)(uECC_vli_equal(rx, r) == 0);
+ diff = uECC_vli_equal(rx, r);
+ if (diff == 0) {
+ mbedtls_platform_enforce_volatile_reads();
+ if (diff == 0) {
+ return UECC_SUCCESS;
+ }
+ else {
+ return UECC_ATTACK_DETECTED;
+ }
+ }
+
+ return UECC_FAILURE;
}