From 684aa260c0f8dfe805e76a1551742573a4cedc19 Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Tue, 12 Nov 2019 16:06:31 -0500 Subject: [PATCH 1/3] Use sizeof(level_t) instead of sizeof(int) in apdu_reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit int can be different sizes on different platforms (although 32-bit is most common). To ensure portability we can use the ‘level_t’ type which is defined as a uint32_t value. This should have no difference in function. Co-authored-by: Jean-Baptiste Bédrune --- src/apdu_baking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apdu_baking.c b/src/apdu_baking.c index dfe5ae7d..942bab09 100644 --- a/src/apdu_baking.c +++ b/src/apdu_baking.c @@ -19,7 +19,7 @@ static bool reset_ok(void); size_t handle_apdu_reset(__attribute__((unused)) uint8_t instruction) { uint8_t *dataBuffer = G_io_apdu_buffer + OFFSET_CDATA; uint32_t dataLength = G_io_apdu_buffer[OFFSET_LC]; - if (dataLength != sizeof(int)) { + if (dataLength != sizeof(level_t)) { THROW(EXC_WRONG_LENGTH_FOR_INS); } level_t const lvl = READ_UNALIGNED_BIG_ENDIAN(level_t, dataBuffer); From 675f4e80395e0a6a809b0bc7fc7a6c4e7d61e6aa Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Tue, 12 Nov 2019 16:24:41 -0500 Subject: [PATCH 2/3] Use explicit_bzero in place of memset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a safer version of memset where memory is completely cleared. This is useful for sensitive data like private keys. https://www.gnu.org/software/libc/manual/html_node/Erasing-Sensitive-Data.html Co-authored-by: Jean-Baptiste Bédrune --- src/keys.c | 28 +++++++++++++++++----------- src/keys.h | 2 +- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/keys.c b/src/keys.c index db4fd997..bf541782 100644 --- a/src/keys.c +++ b/src/keys.c @@ -65,17 +65,23 @@ key_pair_t *generate_key_pair_return_global( priv->private_key_data, NULL); } - cx_ecfp_init_private_key(cx_curve, priv->private_key_data, sizeof(priv->private_key_data), &priv->res.private_key); - cx_ecfp_generate_pair(cx_curve, &priv->res.public_key, &priv->res.private_key, 1); - - if (cx_curve == CX_CURVE_Ed25519) { - cx_edward_compress_point( - CX_CURVE_Ed25519, - priv->res.public_key.W, - priv->res.public_key.W_len); - priv->res.public_key.W_len = 33; + BEGIN_TRY { + TRY { + cx_ecfp_init_private_key(cx_curve, priv->private_key_data, sizeof(priv->private_key_data), &priv->res.private_key); + cx_ecfp_generate_pair(cx_curve, &priv->res.public_key, &priv->res.private_key, 1); + + if (cx_curve == CX_CURVE_Ed25519) { + cx_edward_compress_point(CX_CURVE_Ed25519, + priv->res.public_key.W, + priv->res.public_key.W_len); + priv->res.public_key.W_len = 33; + } + } FINALLY { + explicit_bzero(priv->private_key_data, sizeof(priv->private_key_data)); + } } - memset(priv->private_key_data, 0, sizeof(priv->private_key_data)); + END_TRY; + return &priv->res; } @@ -85,7 +91,7 @@ cx_ecfp_public_key_t const *generate_public_key_return_global( ) { check_null(bip32_path); key_pair_t *const pair = generate_key_pair_return_global(curve, bip32_path); - memset(&pair->private_key, 0, sizeof(pair->private_key)); + explicit_bzero(&pair->private_key, sizeof(pair->private_key)); return &pair->public_key; } diff --git a/src/keys.h b/src/keys.h index ee21471a..f77ef0d7 100644 --- a/src/keys.h +++ b/src/keys.h @@ -35,7 +35,7 @@ static inline void generate_key_pair( check_null(out); key_pair_t *const result = generate_key_pair_return_global(derivation_type, bip32_path); memcpy(out, result, sizeof(*out)); - memset(result, 0, sizeof(*result)); + explicit_bzero(result, sizeof(*result)); } // Non-reentrant From 74e9ed2d43ef0d2ceaa9c265de63756a427a1fd0 Mon Sep 17 00:00:00 2001 From: Matthew Bauer Date: Tue, 12 Nov 2019 16:39:40 -0500 Subject: [PATCH 3/3] Guard against buffer underflow in b58enc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detected by ASAN, buffer underflow occurs when code is capable of unbounded writing to before the buffer begins (compare to buffer overflow). This change ensures that the index is always greater than 0. Co-authored-by: Jean-Baptiste Bédrune --- src/base58.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/base58.c b/src/base58.c index 14389829..2a51edf1 100644 --- a/src/base58.c +++ b/src/base58.c @@ -31,7 +31,7 @@ bool b58enc(/* out */ char *b58, /* in/out */ size_t *b58sz, const void *data, s for (i = zcount, high = size - 1; i < binsz; ++i, high = j) { - for (carry = bin[i], j = size - 1; (j > high) || carry; --j) + for (carry = bin[i], j = size - 1; ((int)j >= 0) && ((j > high) || carry); --j) { carry += 256 * buf[j]; buf[j] = carry % 58;