From eb0dccdb9434db7576f62b2c3e835dc809ddc7f2 Mon Sep 17 00:00:00 2001 From: Leandro Pereira Date: Fri, 18 Aug 2017 10:41:31 -0700 Subject: [PATCH] tinycrypt: ecc_dh: Properly clear out temporary secret buffers Zeroing out 2*NUM_ECC_WORDS bytes starting from the `p2` pointer would not only write 16 bytes to an 8-byte array allocated on the stack, but also not clear out important arrays such as `_private` and `tmp`. Moreover, no memory was cleared out before returning from the function, and there are two exit points. Properly memset() all private data and use an empty assembly block referencing the memory region to avoid the memset() calls to be elided by the compiler. Ideally, in the future, all stack-allocated variables that contains sensitive information should be marked with __attribute__((cleanup)), a GCC extension that calls a function when the variable exits the scope. This will not only reduce code size, but for other functions with multiple exit points, also ensure that sensitive data is always cleared. Signed-off-by: Leandro Pereira --- ext/lib/crypto/tinycrypt/source/ecc_dh.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/ext/lib/crypto/tinycrypt/source/ecc_dh.c b/ext/lib/crypto/tinycrypt/source/ecc_dh.c index c9961c19a98..e5257d2d454 100644 --- a/ext/lib/crypto/tinycrypt/source/ecc_dh.c +++ b/ext/lib/crypto/tinycrypt/source/ecc_dh.c @@ -154,6 +154,7 @@ int uECC_shared_secret(const uint8_t *public_key, const uint8_t *private_key, uECC_word_t carry; wordcount_t num_words = curve->num_words; wordcount_t num_bytes = curve->num_bytes; + int r; /* Converting buffers to correct bit order: */ uECC_vli_bytesToNative(_private, @@ -174,7 +175,8 @@ int uECC_shared_secret(const uint8_t *public_key, const uint8_t *private_key, * improve protection against side-channel attacks. */ if (g_rng_function) { if (!uECC_generate_random_int(p2[carry], curve->p, num_words)) { - return 0; + r = 0; + goto clear_and_out; } initial_Z = p2[carry]; } @@ -182,9 +184,17 @@ int uECC_shared_secret(const uint8_t *public_key, const uint8_t *private_key, EccPoint_mult(_public, _public, p2[!carry], initial_Z, curve->num_n_bits + 1, curve); - /* erasing temporary buffer used to store secret: */ - memset (p2, 0, 2*NUM_ECC_WORDS); - uECC_vli_nativeToBytes(secret, num_bytes, _public); - return !EccPoint_isZero(_public, curve); + r = !EccPoint_isZero(_public, curve); + +clear_and_out: + /* erasing temporary buffer used to store secret: */ + memset(p2, 0, sizeof(p2)); + __asm__ __volatile__("" :: "g"(p2) : "memory"); + memset(tmp, 0, sizeof(tmp)); + __asm__ __volatile__("" :: "g"(tmp) : "memory"); + memset(_private, 0, sizeof(_private)); + __asm__ __volatile__("" :: "g"(_private) : "memory"); + + return r; }