From 5d55ca6bdead9de3455a47110d9df23937e7e7b8 Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Sat, 30 Sep 2023 14:29:16 +0200 Subject: [PATCH 1/6] Remove exceprions from public and private key code --- src/diffieHellman.c | 19 +++- src/eos_utils.c | 64 ++++++----- src/eos_utils.h | 18 +-- src/getPublicKey.c | 7 +- src/hash.h | 162 +++++++++++++++------------ src/keyDerivation.c | 198 +++++++++++++++++++++++++-------- src/keyDerivation.h | 55 ++++++++- src/keyDerivation_test.c | 10 +- src/signTransaction.c | 108 ++++-------------- src/signTransactionIntegrity.c | 16 +-- src/uiScreens.c | 1 + 11 files changed, 404 insertions(+), 254 deletions(-) diff --git a/src/diffieHellman.c b/src/diffieHellman.c index 667fd134..f0a315d4 100644 --- a/src/diffieHellman.c +++ b/src/diffieHellman.c @@ -92,23 +92,30 @@ __noinline_due_to_stack__ void dh_init_aes_key(dh_aes_key_t* dhKey, BEGIN_TRY { TRY { TRACE_STACK_USAGE(); - derivePrivateKey(pathSpec, &privateKey); + { + uint16_t err = derivePrivateKey(pathSpec, &privateKey); + if (err != SUCCESS) { + THROW(err); + } + } - // this is how it is done... cx_err_t err = cx_ecdh_no_throw(&privateKey, CX_ECDH_X, publicKey->W, publicKey->W_len, basicSecret, SIZEOF(basicSecret)); - ASSERT(err == CX_OK); - sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret)); - sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K)); + CX_THROW(err); + err = sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret)); + CX_THROW(err); + err = sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K)); + CX_THROW(err); // First DH_AES_SECRET_SIZE bytes are used to compute shared secret, then DH_KM_SIZE are // used as Km for HMAC calculation STATIC_ASSERT(SIZEOF(K) == DH_AES_SECRET_SIZE + DH_KM_SIZE, "Incompatible types"); - cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey); + err = cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey); + CX_THROW(err); STATIC_ASSERT(SIZEOF(dhKey->km) == DH_KM_SIZE, "Incompatible types"); memmove(dhKey->km, K + DH_AES_SECRET_SIZE, DH_KM_SIZE); dhKey->initialized_magic = DH_AES_KEY_INITIALIZED_MAGIC; diff --git a/src/eos_utils.c b/src/eos_utils.c index c1af1efd..62c9e769 100644 --- a/src/eos_utils.c +++ b/src/eos_utils.c @@ -20,6 +20,12 @@ #include "cx.h" #include "utils.h" +#define FORWARD_CX_ERROR(call) \ + { \ + cx_err_t callResult = (call); \ + if (callResult != CX_OK) return callResult; \ + } + /** * EOS way to check if a signature is canonical :/ */ @@ -80,14 +86,14 @@ int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { * - V, out * - K, out */ -void rng_rfc6979(unsigned char *rnd, - unsigned char *h1, - unsigned char *x, - unsigned int x_len, - const unsigned char *q, - unsigned int q_len, - unsigned char *V, - unsigned char *K) { +cx_err_t rng_rfc6979(unsigned char *rnd, + unsigned char *h1, + unsigned char *x, + unsigned int x_len, + const unsigned char *q, + unsigned int q_len, + unsigned char *V, + unsigned char *K) { unsigned int h_len, found, i; cx_hmac_sha256_t hmac; @@ -104,32 +110,32 @@ void rng_rfc6979(unsigned char *rnd, memset(K, 0x00, h_len); // d. Set: K = HMAC_K(V || 0x00 || int2octets(x) || bits2octets(h1)) V[h_len] = 0; - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); // e. Set: V = HMAC_K(V) - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); // f. Set: K = HMAC_K(V || 0x01 || int2octets(x) || bits2octets(h1)) V[h_len] = 1; - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); // g. Set: V = HMAC_K(V) -- - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); // initial setup only once x = NULL; } else { // h.3 K = HMAC_K(V || 0x00) V[h_len] = 0; - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); // h.3 V = HMAC_K(V) - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); } // generate candidate @@ -145,8 +151,8 @@ void rng_rfc6979(unsigned char *rnd, if (x_len < h_len) { h_len = x_len; } - CX_THROW(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - CX_THROW(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); memcpy(rnd, V, h_len); x_len -= h_len; } @@ -159,6 +165,8 @@ void rng_rfc6979(unsigned char *rnd, } } } + + return CX_OK; } unsigned char const BASE58ALPHABET[58] = { @@ -229,7 +237,9 @@ uint32_t compressed_public_key_to_wif(const uint8_t *publicKey, uint8_t check[20]; cx_ripemd160_t riprip; cx_ripemd160_init(&riprip); - CX_THROW(cx_hash_no_throw(&riprip.header, CX_LAST, temp, 33, check, sizeof(check))); + if (cx_hash_no_throw(&riprip.header, CX_LAST, temp, 33, check, sizeof(check)) != CX_OK) { + return 0; + } memcpy(temp + 33, check, 4); explicit_bzero(out, outLength); diff --git a/src/eos_utils.h b/src/eos_utils.h index f2de8ac1..e303af84 100644 --- a/src/eos_utils.h +++ b/src/eos_utils.h @@ -20,20 +20,22 @@ #include #include +#include "cx.h" unsigned char check_canonical(uint8_t *rs); int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig); -void rng_rfc6979(unsigned char *rnd, - unsigned char *h1, - unsigned char *x, - unsigned int x_len, - const unsigned char *q, - unsigned int q_len, - unsigned char *V, - unsigned char *K); +cx_err_t rng_rfc6979(unsigned char *rnd, + unsigned char *h1, + unsigned char *x, + unsigned int x_len, + const unsigned char *q, + unsigned int q_len, + unsigned char *V, + unsigned char *K); +// returns 0 on error uint32_t public_key_to_wif(const uint8_t *publicKey, uint32_t keyLength, char *out, diff --git a/src/getPublicKey.c b/src/getPublicKey.c index aa5357c1..25fa0f41 100644 --- a/src/getPublicKey.c +++ b/src/getPublicKey.c @@ -51,6 +51,8 @@ static void getPublicKey_ui_runStep() { SIZEOF(ctx->pubKey.W), (char*) G_io_apdu_buffer + SIZEOF(ctx->pubKey.W), MAX_WIF_PUBKEY_LENGTH); + ASSERT(wifkeylen != 0); + ASSERT(wifkeylen <= BUFFER_SIZE_PARANOIA); // we do not copy trailing 0 io_send_buf(SUCCESS, G_io_apdu_buffer, SIZEOF(ctx->pubKey.W) + wifkeylen - 1); @@ -77,7 +79,10 @@ static void runGetPublicKeyUIFlow() { { // Calculation - derivePublicKey(&ctx->pathSpec, &ctx->pubKey); + uint16_t err = derivePublicKey(&ctx->pathSpec, &ctx->pubKey); + if (err != CX_OK) { + THROW(err); + } ctx->responseReadyMagic = RESPONSE_READY_MAGIC; } diff --git a/src/hash.h b/src/hash.h index 43fe1767..cfbe08eb 100644 --- a/src/hash.h +++ b/src/hash.h @@ -22,50 +22,61 @@ typedef struct { cx_sha256_t cx_ctx; } sha_256_context_t; -static __attribute__((always_inline, unused)) void sha_256_init(sha_256_context_t* ctx) { - cx_sha256_init(&ctx->cx_ctx); - ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; +static __attribute__((always_inline, unused)) cx_err_t sha_256_init(sha_256_context_t* ctx) { + cx_err_t err = cx_sha256_init_no_throw(&ctx->cx_ctx); + if (err == CX_OK) { + ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; + } + return err; } -static __attribute__((always_inline, unused)) void sha_256_append(sha_256_context_t* ctx, - const uint8_t* inBuffer, - size_t inSize) { - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); +static __attribute__((always_inline, unused)) cx_err_t sha_256_append(sha_256_context_t* ctx, + const uint8_t* inBuffer, + size_t inSize) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + return CX_INVALID_PARAMETER; + } TRACE_BUFFER(inBuffer, inSize); - CX_THROW(cx_hash_no_throw(&ctx->cx_ctx.header, - 0, /* Do not output the hash, yet */ - inBuffer, - inSize, - NULL, - 0)); + return cx_hash_no_throw(&ctx->cx_ctx.header, + 0, /* Do not output the hash, yet */ + inBuffer, + inSize, + NULL, + 0); } -static __attribute__((always_inline, unused)) void sha_256_finalize(sha_256_context_t* ctx, - uint8_t* outBuffer, - size_t outSize) { - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); - ASSERT(outSize == SHA_256_SIZE); - CX_THROW(cx_hash_no_throw(&ctx->cx_ctx.header, - CX_LAST, /* Output the hash */ - NULL, - 0, - outBuffer, - SHA_256_SIZE)); +static __attribute__((always_inline, unused)) cx_err_t sha_256_finalize(sha_256_context_t* ctx, + uint8_t* outBuffer, + size_t outSize) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC || outSize != SHA_256_SIZE) { + return CX_INVALID_PARAMETER; + } + return cx_hash_no_throw(&ctx->cx_ctx.header, + CX_LAST, /* Output the hash */ + NULL, + 0, + outBuffer, + SHA_256_SIZE); } /* Convenience function to make all in one step */ -static __attribute__((always_inline, unused)) void sha_256_hash(const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize) { - ASSERT(inSize < BUFFER_SIZE_PARANOIA); - ASSERT(outSize == SHA_256_SIZE); +static __attribute__((always_inline, unused)) cx_err_t sha_256_hash(const uint8_t* inBuffer, + size_t inSize, + uint8_t* outBuffer, + size_t outSize) { + if (inSize >= BUFFER_SIZE_PARANOIA) { + return CX_INVALID_PARAMETER; + } sha_256_context_t ctx; - sha_256_init(&ctx); - /* Note: This could be done by single cx_hash call */ - /* But we don't really care */ - sha_256_append(&ctx, inBuffer, inSize); - sha_256_finalize(&ctx, outBuffer, outSize); + cx_err_t err = sha_256_init(&ctx); + if (err != CX_OK) { + return err; + } + err = sha_256_append(&ctx, inBuffer, inSize); + if (err != CX_OK) { + return err; + } + return sha_256_finalize(&ctx, outBuffer, outSize); } typedef struct { @@ -73,50 +84,61 @@ typedef struct { cx_sha512_t cx_ctx; } sha_512_context_t; -static __attribute__((always_inline, unused)) void sha_512_init(sha_512_context_t* ctx) { - cx_sha512_init(&ctx->cx_ctx); - ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; +static __attribute__((always_inline, unused)) cx_err_t sha_512_init(sha_512_context_t* ctx) { + cx_err_t err = cx_sha512_init_no_throw(&ctx->cx_ctx); + if (err == CX_OK) { + ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; + } + return err; } -static __attribute__((always_inline, unused)) void sha_512_append(sha_512_context_t* ctx, - const uint8_t* inBuffer, - size_t inSize) { - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); +static __attribute__((always_inline, unused)) cx_err_t sha_512_append(sha_512_context_t* ctx, + const uint8_t* inBuffer, + size_t inSize) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + return CX_INVALID_PARAMETER; + } TRACE_BUFFER(inBuffer, inSize); - CX_THROW(cx_hash_no_throw(&ctx->cx_ctx.header, - 0, /* Do not output the hash, yet */ - inBuffer, - inSize, - NULL, - 0)); + return cx_hash_no_throw(&ctx->cx_ctx.header, + 0, /* Do not output the hash, yet */ + inBuffer, + inSize, + NULL, + 0); } -static __attribute__((always_inline, unused)) void sha_512_finalize(sha_512_context_t* ctx, - uint8_t* outBuffer, - size_t outSize) { - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); - ASSERT(outSize == SHA_512_SIZE); - CX_THROW(cx_hash_no_throw(&ctx->cx_ctx.header, - CX_LAST, /* Output the hash */ - NULL, - 0, - outBuffer, - SHA_512_SIZE)); +static __attribute__((always_inline, unused)) cx_err_t sha_512_finalize(sha_512_context_t* ctx, + uint8_t* outBuffer, + size_t outSize) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC || outSize != SHA_512_SIZE) { + return CX_INVALID_PARAMETER; + } + return cx_hash_no_throw(&ctx->cx_ctx.header, + CX_LAST, /* Output the hash */ + NULL, + 0, + outBuffer, + SHA_512_SIZE); } /* Convenience function to make all in one step */ -static __attribute__((always_inline, unused)) void sha_512_hash(const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize) { - ASSERT(inSize < BUFFER_SIZE_PARANOIA); - ASSERT(outSize == SHA_512_SIZE); +static __attribute__((always_inline, unused)) cx_err_t sha_512_hash(const uint8_t* inBuffer, + size_t inSize, + uint8_t* outBuffer, + size_t outSize) { + if (inSize >= BUFFER_SIZE_PARANOIA) { + return CX_INVALID_PARAMETER; + } sha_512_context_t ctx; - sha_512_init(&ctx); - /* Note: This could be done by single cx_hash call */ - /* But we don't really care */ - sha_512_append(&ctx, inBuffer, inSize); - sha_512_finalize(&ctx, outBuffer, outSize); + cx_err_t err = sha_512_init(&ctx); + if (err != CX_OK) { + return err; + } + err = sha_512_append(&ctx, inBuffer, inSize); + if (err != CX_OK) { + return err; + } + return sha_512_finalize(&ctx, outBuffer, outSize); } #endif // H_FIO_APP_HASH diff --git a/src/keyDerivation.c b/src/keyDerivation.c index e768ecc4..5b2530af 100644 --- a/src/keyDerivation.c +++ b/src/keyDerivation.c @@ -1,72 +1,180 @@ #include #include - -#include "assert.h" #include "errors.h" #include "keyDerivation.h" #include "hash.h" #include "utils.h" #include "fio.h" #include "securityPolicy.h" +#include "eos_utils.h" #define PRIVATE_KEY_SEED_LEN 64 -__noinline_due_to_stack__ void derivePrivateKey(const bip44_path_t* pathSpec, - private_key_t* privateKey) { - ENSURE_NOT_DENIED(policyDerivePrivateKey(pathSpec)); +// Taken from EOS app. Needed to produce signatures. +static uint8_t const SECP256K1_N[] = { + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, + 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41}; + +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t* pathSpec, + private_key_t* privateKey) { + if (policyDerivePrivateKey(pathSpec) == POLICY_DENY) { + return ERR_REJECTED_BY_POLICY; + } // Sanity check - ASSERT(pathSpec->length < ARRAY_LEN(pathSpec->path)); + if (pathSpec->length >= ARRAY_LEN(pathSpec->path)) { + return ERR_ASSERT; + } TRACE(); uint8_t privateKeySeed[PRIVATE_KEY_SEED_LEN]; + explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); - BEGIN_TRY { - TRY { - STATIC_ASSERT(CX_APILEVEL >= 5, "unsupported api level"); - - io_seproxyhal_io_heartbeat(); - CX_THROW(os_derive_bip32_with_seed_no_throw(HDW_NORMAL, - CX_CURVE_SECP256K1, - pathSpec->path, - pathSpec->length, - privateKeySeed, - NULL, - NULL, - 0)); - io_seproxyhal_io_heartbeat(); - - CX_THROW(cx_ecfp_init_private_key_no_throw(CX_CURVE_SECP256K1, - privateKeySeed, - 32, - privateKey)); - } - FINALLY { - explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); + STATIC_ASSERT(CX_APILEVEL >= 5, "unsupported api level"); + + cx_err_t err = os_derive_bip32_with_seed_no_throw(HDW_NORMAL, + CX_CURVE_SECP256K1, + pathSpec->path, + pathSpec->length, + privateKeySeed, + NULL, + NULL, + 0); + if (err != CX_OK) { + explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); + return ERR_ASSERT; + } + + err = cx_ecfp_init_private_key_no_throw(CX_CURVE_SECP256K1, privateKeySeed, 32, privateKey); + if (err != CX_OK) { + explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); + explicit_bzero(privateKey, SIZEOF(*privateKey)); + return ERR_ASSERT; + } + + return SUCCESS; +} + +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t* pathSpec, + public_key_t* publicKey) { + private_key_t privateKey; + explicit_bzero(&privateKey, SIZEOF(privateKey)); + + { + uint16_t err = derivePrivateKey(pathSpec, &privateKey); + if (err != SUCCESS) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + return err; } } - END_TRY; + + cx_err_t err = cx_ecfp_init_public_key_no_throw(CX_CURVE_SECP256K1, NULL, 0, publicKey); + if (err != CX_OK) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + return ERR_ASSERT; + } + + err = cx_ecfp_generate_pair_no_throw(CX_CURVE_SECP256K1, + publicKey, + &privateKey, + 1); // 1 - private key preserved + if (err != CX_OK) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + return ERR_ASSERT; + } + + return SUCCESS; } -__noinline_due_to_stack__ void derivePublicKey(const bip44_path_t* pathSpec, - public_key_t* publicKey) { +// This function contains code producing signatures that is taken from EOS app +// The produced signature contains both +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t* wittnessPath, + uint8_t hashBuf[SHA_256_SIZE], + uint8_t* signature, + size_t signatureLen) { + if (signatureLen < 200) { + return ERR_ASSERT; + } + + // Derive keys and sign the transaction, setup private_key_t privateKey; - BEGIN_TRY { - TRY { - derivePrivateKey(pathSpec, &privateKey); - // We should do cx_ecfp_generate_pair here, but it does not work in SDK < 1.5.4, - // should work with the new SDK - io_seproxyhal_io_heartbeat(); - CX_THROW(cx_ecfp_init_public_key_no_throw(CX_CURVE_SECP256K1, NULL, 0, publicKey)); - CX_THROW(cx_ecfp_generate_pair_no_throw(CX_CURVE_SECP256K1, - publicKey, - &privateKey, - 1)); // 1 - private key preserved - io_seproxyhal_io_heartbeat(); + explicit_bzero(&privateKey, SIZEOF(privateKey)); + + // We derive the private key + { + uint16_t err = derivePrivateKey(wittnessPath, &privateKey); + if (err != SUCCESS) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + return err; } - FINALLY { + TRACE("privateKey.d:"); + TRACE_BUFFER(privateKey.d, privateKey.d_len); + } + + // We sign the hash + explicit_bzero(signature, signatureLen); + uint8_t V[33]; + uint8_t K[32]; + int tries = 0; + + // Loop until a candidate matching the canonical signature is found + for (;;) { + if (tries == 0) { + cx_err_t err = rng_rfc6979(signature + 100, + hashBuf, + privateKey.d, + privateKey.d_len, + SECP256K1_N, + 32, + V, + K); + if (err != CX_OK) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(signature, signatureLen); + return ERR_ASSERT; + } + } else { + cx_err_t err = rng_rfc6979(signature + 100, hashBuf, NULL, 0, SECP256K1_N, 32, V, K); + if (err != CX_OK) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(signature, signatureLen); + return ERR_ASSERT; + } + } + uint32_t infos; + + size_t sig_len_ = 100; + cx_err_t err = cx_ecdsa_sign_no_throw(&privateKey, + CX_NO_CANONICAL | CX_RND_PROVIDED | CX_LAST, + CX_SHA256, + hashBuf, + 32, + signature + 100, + &sig_len_, + &infos); + if (err != CX_OK) { explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(signature, signatureLen); + return ERR_ASSERT; + } + TRACE_BUFFER(signature + 100, 100); + + if ((infos & CX_ECCINFO_PARITY_ODD) != 0) { + signature[100] |= 0x01; + } + signature[0] = 27 + 4 + (signature[100] & 0x01); + ecdsa_der_to_sig(signature + 100, signature + 1); + TRACE_BUFFER(signature, PUBKEY_LENGTH); + + if (check_canonical(signature + 1)) { + TRACE("Try %d succesfull!", tries); + break; + } else { + TRACE("Try %d unsuccesfull!", tries); + tries++; } } - END_TRY; + + explicit_bzero(&privateKey, sizeof(privateKey)); + return SUCCESS; } diff --git a/src/keyDerivation.h b/src/keyDerivation.h index 70106464..7c083682 100644 --- a/src/keyDerivation.h +++ b/src/keyDerivation.h @@ -5,6 +5,7 @@ #include "handlers.h" #include "bip44.h" #include "utils.h" +#include "hash.h" #define CHAIN_CODE_SIZE (32) @@ -15,11 +16,57 @@ typedef struct { uint8_t code[CHAIN_CODE_SIZE]; } chain_code_t; -__noinline_due_to_stack__ void derivePrivateKey(const bip44_path_t* pathSpec, - private_key_t* privateKey // output -); +/** + * @brief Gets the private key from the device. Call this only from other crypto code. + * + * @param[in] pathSpec Derivation path. + * + * @param[out] privateKey Private key. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t* pathSpec, + private_key_t* privateKey); -__noinline_due_to_stack__ void derivePublicKey(const bip44_path_t* pathSpec, public_key_t* out); +/** + * @brief Gets the public key from the device. + * + * @param[in] pathSpec Derivation path. + * + * @param[out] publicKey Public key. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t* pathSpec, + public_key_t* publicKey); + +/** + * @brief Signs transaction hash. + * + * @param[in] wittnessPath Derivation path. + * + * @param[in] hashBuf Hash to sign. + * + * @param[out] signature Signature. The buffer should be sufficient for der_to_sig conversion. + * This is meant to save space expecting that G_io_apdu_buffer is used. + * + * @param[in] signatureLen Length of the signature buffer. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t* wittnessPath, + uint8_t hashBuf[SHA_256_SIZE], + uint8_t* signature, + size_t signatureLen); #ifdef DEVEL __noinline_due_to_stack__ void run_key_derivation_test(); diff --git a/src/keyDerivation_test.c b/src/keyDerivation_test.c index 9ea3357d..f27bc378 100644 --- a/src/keyDerivation_test.c +++ b/src/keyDerivation_test.c @@ -19,7 +19,10 @@ void testcase_derivePrivateKey(uint32_t* path, uint32_t pathLen, const char* exp PRINTF("\n"); private_key_t privateKey; - derivePrivateKey(&pathSpec, &privateKey); + uint16_t err = derivePrivateKey(&pathSpec, &privateKey); + if (err != SUCCESS) { + THROW(err); + } TRACE("%d", SIZEOF(privateKey.d)); TRACE_BUFFER(privateKey.d, SIZEOF(privateKey.d)); @@ -73,7 +76,10 @@ void testcase_derivePublicKey(uint32_t* path, uint32_t pathLen, const char* expe PRINTF("\n"); public_key_t publicKey; - derivePublicKey(&pathSpec, &publicKey); + uint16_t err = derivePublicKey(&pathSpec, &publicKey); + if (err != SUCCESS) { + THROW(err); + } TRACE("%d", SIZEOF(publicKey.W)); TRACE_BUFFER(publicKey.W, SIZEOF(publicKey.W)); diff --git a/src/signTransaction.c b/src/signTransaction.c index 876616f2..b44deedb 100644 --- a/src/signTransaction.c +++ b/src/signTransaction.c @@ -34,11 +34,6 @@ enum { TX_INIT_WAS_CALLED_INITIALIZED_MAGIC = 12346, }; -// Taken from EOS app. Needed to produce signatures. -static uint8_t const SECP256K1_N[] = { - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, - 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41}; - // Uses ctx->dataToAppendToTx, ctx->dataToAppendToTxLen to extend hash // If ctx->dhIsActive then, we extend hash with encrypted data and prepare resulting encrypted // blocks to G_io_apdu_buffer, ctx->responseLength Variables (&ctx->wittnessPath, &ctx->otherPubkey, @@ -58,7 +53,7 @@ static void processShaAndPosibleDHAndPrepareResponse() { ctx->dataToAppendToTxLen, G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); - sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength); + CX_THROW(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength)); VALIDATE( ctx->countedSectionDifference + ctx->responseLength >= ctx->dataToAppendToTxLen, ERR_INVALID_STATE); @@ -75,7 +70,9 @@ static void processShaAndPosibleDHAndPrepareResponse() { } END_TRY; } else { - sha_256_append(&ctx->hashContext, ctx->dataToAppendToTx, ctx->dataToAppendToTxLen); + cx_err_t err = + sha_256_append(&ctx->hashContext, ctx->dataToAppendToTx, ctx->dataToAppendToTxLen); + ASSERT(err == CX_OK); ctx->responseLength = 0; } } @@ -85,13 +82,15 @@ static void processShaAndPosibleDHAndPrepareResponse() { static void prepareOurPubkeyForDisplay() { public_key_t wittnessPathPubkey; explicit_bzero(&wittnessPathPubkey, SIZEOF(wittnessPathPubkey)); - derivePublicKey(&ctx->wittnessPath, &wittnessPathPubkey); + uint16_t err = derivePublicKey(&ctx->wittnessPath, &wittnessPathPubkey); + ASSERT(err == SUCCESS); TRACE_BUFFER(wittnessPathPubkey.W, SIZEOF(wittnessPathPubkey.W)); uint32_t outlen = public_key_to_wif(wittnessPathPubkey.W, SIZEOF(wittnessPathPubkey.W), ctx->value, SIZEOF(ctx->value)); + ASSERT(outlen != 0); ASSERT(outlen < SIZEOF(ctx->value)); ctx->value[outlen] = 0; } @@ -193,7 +192,8 @@ __noinline_due_to_stack__ void signTx_handleInitAPDU(uint8_t p2, VALIDATE(!ctx->dhIsActive, ERR_INVALID_STATE); VALIDATE(countedSectionProcess(&ctx->countedSections, ctx->dataToAppendToTxLen), ERR_INVALID_DATA); - sha_256_append(&ctx->hashContext, ctx->dataToAppendToTx, ctx->dataToAppendToTxLen); + ASSERT(sha_256_append(&ctx->hashContext, ctx->dataToAppendToTx, ctx->dataToAppendToTxLen) == + CX_OK); ctx->responseLength = 0; } @@ -690,6 +690,7 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( SIZEOF(ctx->otherPubkey.W), ctx->value, SIZEOF(ctx->value)); + ASSERT(outlen != 0); ASSERT(outlen < SIZEOF(ctx->value)); ctx->value[outlen] = 0; } @@ -722,7 +723,9 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( SIZEOF(IV), G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); - ASSERT(ctx->responseLength == 20); // first 5 blocks + if (ctx->responseLength != 20) { // first 5 blocks + THROW(ERR_ASSERT); + } ctx->countedSectionDifference = ctx->responseLength; TRACE("CS diff %d", (int) ctx->responseLength); } @@ -732,7 +735,7 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( } END_TRY; - sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength); + ASSERT(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength) == CX_OK); ctx->dhIsActive = true; } @@ -830,7 +833,8 @@ __noinline_due_to_stack__ void signTx_handleEndDHEncodingAPDU( VALIDATE(countedSectionProcess(&ctx->countedSections, ctx->countedSectionDifference), ERR_INVALID_STATE); - sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength); + ASSERT(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength) == + CX_OK); ctx->dhIsActive = false; } @@ -917,7 +921,7 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( uint8_t hashBuf[SHA_256_SIZE]; explicit_bzero(hashBuf, SIZEOF(hashBuf)); { - sha_256_finalize(&ctx->hashContext, hashBuf, SIZEOF(hashBuf)); + ASSERT(sha_256_finalize(&ctx->hashContext, hashBuf, SIZEOF(hashBuf)) == CX_OK); TRACE("SHA_256_finalize, resulting hash:"); TRACE_BUFFER(hashBuf, SHA_256_SIZE); } @@ -944,81 +948,16 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( } } - // Derive keys and sign the transaction, setup - private_key_t privateKey; - explicit_bzero(&privateKey, SIZEOF(privateKey)); - BEGIN_TRY { - TRY { - // We derive the private key - { - derivePrivateKey(&ctx->wittnessPath, &privateKey); - TRACE("privateKey.d:"); - TRACE_BUFFER(privateKey.d, privateKey.d_len); - } - - // We sign the hash - // Code producing signatures is taken from EOS app - uint8_t V[33]; - uint8_t K[32]; - int tries = 0; - - // Loop until a candidate matching the canonical signature is found - // Taken from EOS app - // We use G_io_apdu_buffer to save memory (and also to minimize changes to EOS code) - // The code produces the signature right where we need it for the respons - explicit_bzero(G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); - for (;;) { - if (tries == 0) { - rng_rfc6979(G_io_apdu_buffer + 100, - hashBuf, - privateKey.d, - privateKey.d_len, - SECP256K1_N, - 32, - V, - K); - } else { - rng_rfc6979(G_io_apdu_buffer + 100, hashBuf, NULL, 0, SECP256K1_N, 32, V, K); - } - uint32_t infos; - - size_t sig_len_ = 100; - CX_THROW(cx_ecdsa_sign_no_throw(&privateKey, - CX_NO_CANONICAL | CX_RND_PROVIDED | CX_LAST, - CX_SHA256, - hashBuf, - 32, - G_io_apdu_buffer + 100, - &sig_len_, - &infos)); - TRACE_BUFFER(G_io_apdu_buffer + 100, 100); - - if ((infos & CX_ECCINFO_PARITY_ODD) != 0) { - G_io_apdu_buffer[100] |= 0x01; - } - G_io_apdu_buffer[0] = 27 + 4 + (G_io_apdu_buffer[100] & 0x01); - ecdsa_der_to_sig(G_io_apdu_buffer + 100, G_io_apdu_buffer + 1); - TRACE_BUFFER(G_io_apdu_buffer, PUBKEY_LENGTH); - - if (check_canonical(G_io_apdu_buffer + 1)) { - TRACE("Try %d succesfull!", tries); - break; - } else { - TRACE("Try %d unsuccesfull!", tries); - tries++; - } - } - } - FINALLY { - explicit_bzero(&privateKey, sizeof(privateKey)); - } + uint16_t err = + signTransaction(&ctx->wittnessPath, hashBuf, G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); + if (err != SUCCESS) { + THROW(err); } - END_TRY; // We add hash to the response TRACE("ecdsa_der_to_sig_result:"); TRACE_BUFFER(G_io_apdu_buffer, PUBKEY_LENGTH); - memcpy(G_io_apdu_buffer + PUBKEY_LENGTH, hashBuf, SHA_256_SIZE); + memcpy(G_io_apdu_buffer + PUBKEY_LENGTH, hashBuf, SIZEOF(hashBuf)); signTx_handleFinish_ui_runStep(); } @@ -1066,7 +1005,8 @@ void signTransaction_handleAPDU(uint8_t p1, if (isNewCall) { explicit_bzero(ctx, SIZEOF(*ctx)); TRACE("SHA_256_init"); - sha_256_init(&ctx->hashContext); + cx_err_t err = sha_256_init(&ctx->hashContext); + ASSERT(err == CX_OK); TRACE("Integrity check init"); integrityCheckInit(&ctx->integrity); TRACE("Counted sections init"); diff --git a/src/signTransactionIntegrity.c b/src/signTransactionIntegrity.c index 7438ad95..92c95681 100644 --- a/src/signTransactionIntegrity.c +++ b/src/signTransactionIntegrity.c @@ -311,13 +311,15 @@ __noinline_due_to_stack__ void integrityCheckProcessInstruction(tx_integrity_t * uint8_t constDataLength) { ASSERT(integrity->initialized_magic == TX_INTEGRITY_HASH_INITIALIZED_MAGIC); sha_256_context_t ctx; - sha_256_init(&ctx); - sha_256_append(&ctx, integrity->integrityHash, SIZEOF(integrity->integrityHash)); - sha_256_append(&ctx, &p1, SIZEOF(p1)); - sha_256_append(&ctx, &p2, SIZEOF(p2)); - sha_256_append(&ctx, &constDataLength, SIZEOF(constDataLength)); - sha_256_append(&ctx, constData, constDataLength); - sha_256_finalize(&ctx, integrity->integrityHash, SIZEOF(integrity->integrityHash)); + ASSERT(sha_256_init(&ctx) == CX_OK); + ASSERT(sha_256_append(&ctx, integrity->integrityHash, SIZEOF(integrity->integrityHash)) == + CX_OK); + ASSERT(sha_256_append(&ctx, &p1, SIZEOF(p1)) == CX_OK); + ASSERT(sha_256_append(&ctx, &p2, SIZEOF(p2)) == CX_OK); + ASSERT(sha_256_append(&ctx, &constDataLength, SIZEOF(constDataLength)) == CX_OK); + ASSERT(sha_256_append(&ctx, constData, constDataLength) == CX_OK); + ASSERT(sha_256_finalize(&ctx, integrity->integrityHash, SIZEOF(integrity->integrityHash)) == + CX_OK); TRACE("p1: %02x. p2: %02x, constdata: %.*h", p1, p2, constDataLength, constData); TRACE_BUFFER(&integrity->integrityHash, SIZEOF(integrity->integrityHash)); diff --git a/src/uiScreens.c b/src/uiScreens.c index da61af6f..a3896f41 100644 --- a/src/uiScreens.c +++ b/src/uiScreens.c @@ -60,6 +60,7 @@ __noinline_due_to_stack__ void ui_displayPubkeyScreen(const char* screenHeader, char buffer[MAX_WIF_PUBKEY_LENGTH + 1]; uint32_t outlen = public_key_to_wif(pubkey->W, SIZEOF(pubkey->W), buffer, SIZEOF(buffer)); + ASSERT(outlen != 0); ASSERT(outlen < MAX_WIF_PUBKEY_LENGTH + 1); buffer[outlen] = 0; From a3661c61227a809a75cf3845602f8acda8fef823 Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Fri, 6 Oct 2023 16:56:44 +0200 Subject: [PATCH 2/6] Refactor DH crypto functions --- src/decodeDH.c | 5 +- src/diffieHellman.c | 591 +++++++++++++++++++++++++------------- src/diffieHellman.h | 185 ++++++++---- src/diffieHellmann_test.c | 129 ++++++--- src/getPublicKey.c | 2 +- src/keyDerivation.c | 42 ++- src/signTransaction.c | 127 ++++---- 7 files changed, 697 insertions(+), 384 deletions(-) diff --git a/src/decodeDH.c b/src/decodeDH.c index 7e1a28cf..9ceb3c0b 100644 --- a/src/decodeDH.c +++ b/src/decodeDH.c @@ -503,7 +503,10 @@ void decode_handleAPDU(uint8_t p1, TRACE("Decoding DH"); ASSERT(ctx->bufferLen <= SIZEOF(ctx->buffer)); TRACE_BUFFER(ctx->buffer, ctx->bufferLen); - ctx->bufferLen = dh_decode(&ctx->pathSpec, &ctx->otherPubKey, ctx->buffer, ctx->bufferLen); + uint16_t err = dh_decode(&ctx->pathSpec, &ctx->otherPubKey, ctx->buffer, &ctx->bufferLen); + if (err != SUCCESS) { + THROW(err); + } ctx->messageDecodedMagic = DECODING_FINISHED_MAGIC; ctx->bufferSentLen = 0; TRACE_BUFFER(ctx->buffer, ctx->bufferLen); diff --git a/src/diffieHellman.c b/src/diffieHellman.c index f0a315d4..5e4ea8ef 100644 --- a/src/diffieHellman.c +++ b/src/diffieHellman.c @@ -16,12 +16,12 @@ static void base64EncBlock(uint8_t in[3], uint8_t out[4]) { out[3] = BASE64[in[2] & 0x3F]; } -// Returns number of bytes written +// Base64 encodes as many block as possible. // Move unencoded part of inBuffer to the beginning and sets inSize accordingly -static size_t base64EncWholeBlocks(uint8_t* inBuffer, - uint8_t* inSize, - uint8_t* outBuffer, - size_t outSize) { +static WARN_UNUSED_RESULT uint16_t base64EncWholeBlocks(uint8_t *inBuffer, + uint8_t *inSize, + uint8_t *outBuffer, + uint16_t *outSize) { uint8_t processedBlocks = 0; while (1) { // We cannot process whole block @@ -32,45 +32,61 @@ static size_t base64EncWholeBlocks(uint8_t* inBuffer, break; } - // process one block, inBuffer + BASE64_IN_BLOCK_SIZE*processedBlocks and next two values - // exist - ASSERT(outSize >= BASE64_OUT_BLOCK_SIZE * (processedBlocks + 1)); + // process one block + if (*outSize < BASE64_OUT_BLOCK_SIZE * (processedBlocks + 1)) { + return ERR_ASSERT; + }; base64EncBlock(inBuffer + BASE64_IN_BLOCK_SIZE * processedBlocks, outBuffer + BASE64_OUT_BLOCK_SIZE * processedBlocks); processedBlocks++; } - return BASE64_OUT_BLOCK_SIZE * processedBlocks; + *outSize = BASE64_OUT_BLOCK_SIZE * processedBlocks; + return SUCCESS; } -static size_t processDHOneBlockFromCache(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - uint8_t* outBuffer, - size_t outSize) { - ASSERT(ctx->cacheLength == CX_AES_BLOCK_SIZE); +// Encodes one block from cache using AES key +// Move unencoded part of inBuffer to the beginning and sets inSize accordingly +static WARN_UNUSED_RESULT uint16_t processDHOneBlockFromCache(dh_context_t *ctx, + const dh_aes_key_t *aesKey, + uint8_t *outBuffer, + uint16_t *outSize) { + // precondition sanity check + if (ctx->cacheLength != CX_AES_BLOCK_SIZE) { + return ERR_ASSERT; + } STATIC_ASSERT(SIZEOF(ctx->IV) == CX_AES_BLOCK_SIZE, "Incompatible IV size"); // We work in CBC mode // 1. IV xor plaintext - for (size_t i = 0; i < CX_AES_BLOCK_SIZE; i++) { + for (uint16_t i = 0; i < CX_AES_BLOCK_SIZE; i++) { ctx->cache[i] ^= ctx->IV[i]; } // 2. encrypt the result to obtain cyphertext 3. use cyphertext as new IV - cx_err_t err = cx_aes_enc_block(&aes_key->aesKey, ctx->cache, ctx->IV); - ASSERT(err == CX_OK); + cx_err_t err = cx_aes_enc_block(&aesKey->aesKey, ctx->cache, ctx->IV); + if (err != CX_OK) { + return ERR_ASSERT; + } // append cyphertext (not base64 encrypted) to hmac and clear cache - err = cx_hmac_update((cx_hmac_t*) &ctx->hmacCtx, ctx->IV, CX_AES_BLOCK_SIZE); - ASSERT(err == CX_OK); + err = cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, ctx->IV, CX_AES_BLOCK_SIZE); + if (err != CX_OK) { + return ERR_ASSERT; + } explicit_bzero(ctx->cache, SIZEOF(ctx->cache)); ctx->cacheLength = 0; // base64 encode - ASSERT(ctx->base64EncodingCacheLen < BASE64_IN_BLOCK_SIZE); + + // sanity check, we expect that cache does not contain whole block first + if (ctx->base64EncodingCacheLen >= BASE64_IN_BLOCK_SIZE) { + return ERR_ASSERT; + } STATIC_ASSERT(SIZEOF(ctx->base64EncodingCache) >= BASE64_IN_BLOCK_SIZE + CX_AES_BLOCK_SIZE, "Cache too small"); memmove(ctx->base64EncodingCache + ctx->base64EncodingCacheLen, ctx->IV, CX_AES_BLOCK_SIZE); ctx->base64EncodingCacheLen += CX_AES_BLOCK_SIZE; + // This also sets outSize as desired return base64EncWholeBlocks(ctx->base64EncodingCache, &ctx->base64EncodingCacheLen, outBuffer, @@ -79,9 +95,8 @@ static size_t processDHOneBlockFromCache(dh_context_t* ctx, //---------------------------- DH ENCODING --------------------------------------- -__noinline_due_to_stack__ void dh_init_aes_key(dh_aes_key_t* dhKey, - const bip44_path_t* pathSpec, - const public_key_t* publicKey) { +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_key_t *publicKey) { TRACE_STACK_USAGE(); private_key_t privateKey; unsigned char basicSecret[32]; @@ -89,70 +104,107 @@ __noinline_due_to_stack__ void dh_init_aes_key(dh_aes_key_t* dhKey, uint8_t K[SHA_512_SIZE]; TRACE("dh_init_aesKey"); - BEGIN_TRY { - TRY { - TRACE_STACK_USAGE(); - { - uint16_t err = derivePrivateKey(pathSpec, &privateKey); - if (err != SUCCESS) { - THROW(err); - } - } - cx_err_t err = cx_ecdh_no_throw(&privateKey, - CX_ECDH_X, - publicKey->W, - publicKey->W_len, - basicSecret, - SIZEOF(basicSecret)); - CX_THROW(err); - err = sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret)); - CX_THROW(err); - err = sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K)); - CX_THROW(err); - - // First DH_AES_SECRET_SIZE bytes are used to compute shared secret, then DH_KM_SIZE are - // used as Km for HMAC calculation - STATIC_ASSERT(SIZEOF(K) == DH_AES_SECRET_SIZE + DH_KM_SIZE, "Incompatible types"); - err = cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey); - CX_THROW(err); - STATIC_ASSERT(SIZEOF(dhKey->km) == DH_KM_SIZE, "Incompatible types"); - memmove(dhKey->km, K + DH_AES_SECRET_SIZE, DH_KM_SIZE); - dhKey->initialized_magic = DH_AES_KEY_INITIALIZED_MAGIC; - } - FINALLY { - explicit_bzero(&privateKey, sizeof(privateKey)); - explicit_bzero(basicSecret, SIZEOF(basicSecret)); - explicit_bzero(secret, SIZEOF(secret)); - explicit_bzero(K, SIZEOF(K)); + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(basicSecret, SIZEOF(basicSecret)); + explicit_bzero(secret, SIZEOF(secret)); + explicit_bzero(K, SIZEOF(K)); + explicit_bzero(dhKey, SIZEOF(*dhKey)); + + { + uint16_t err = derivePrivateKey(pathSpec, &privateKey); + if (err != SUCCESS) { + explicit_bzero(&privateKey, SIZEOF(privateKey)); + return err; } } - END_TRY; + + cx_err_t err = cx_ecdh_no_throw(&privateKey, + CX_ECDH_X, + publicKey->W, + publicKey->W_len, + basicSecret, + SIZEOF(basicSecret)); + explicit_bzero(&privateKey, SIZEOF(privateKey)); + if (err != CX_OK) { + explicit_bzero(basicSecret, SIZEOF(basicSecret)); + return ERR_ASSERT; + } + + err = sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret)); + explicit_bzero(basicSecret, SIZEOF(basicSecret)); + if (err != CX_OK) { + explicit_bzero(secret, SIZEOF(secret)); + return ERR_ASSERT; + } + + err = sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K)); + explicit_bzero(secret, SIZEOF(secret)); + if (err != CX_OK) { + explicit_bzero(K, SIZEOF(K)); + return ERR_ASSERT; + } + + // First DH_AES_SECRET_SIZE bytes are used to compute shared secret, then DH_KM_SIZE are + // used as Km for HMAC calculation + STATIC_ASSERT(SIZEOF(K) == DH_AES_SECRET_SIZE + DH_KM_SIZE, "Incompatible types"); + err = cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey); + if (err != CX_OK) { + explicit_bzero(K, SIZEOF(K)); + explicit_bzero(dhKey, SIZEOF(*dhKey)); + return ERR_ASSERT; + } + + STATIC_ASSERT(SIZEOF(dhKey->km) == DH_KM_SIZE, "Incompatible types"); + memmove(dhKey->km, K + DH_AES_SECRET_SIZE, DH_KM_SIZE); + return SUCCESS; } -__noinline_due_to_stack__ size_t dh_encode_init(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - const uint8_t* iv, - size_t ivSize, - uint8_t* outBuffer, - size_t outSize) { +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode_init(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + const uint8_t *iv, + uint16_t ivSize, + uint8_t *outBuffer, + uint16_t *outSize) { + // Crypto assets to be cleared + dh_aes_key_t aesKey; + explicit_bzero(&aesKey, SIZEOF(aesKey)); + TRACE_STACK_USAGE(); TRACE("dh_encode_init"); - ASSERT(ivSize == SIZEOF(ctx->IV)); - ASSERT(aes_key->initialized_magic == DH_AES_KEY_INITIALIZED_MAGIC); + STATIC_ASSERT(SIZEOF(ctx->IV) == CX_AES_BLOCK_SIZE, "Unexpected IV length"); + if (ivSize != SIZEOF(ctx->IV)) { + return ERR_ASSERT; + } + // initialize DH context ctx->cacheLength = 0; explicit_bzero(ctx->cache, SIZEOF(ctx->cache)); ctx->base64EncodingCacheLen = 0; explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); memcpy(ctx->IV, iv, SIZEOF(ctx->IV)); + // Compute AES key + { + uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; + } + } + explicit_bzero(&ctx->hmacCtx, SIZEOF(ctx->hmacCtx)); - cx_err_t err = cx_hmac_sha256_init_no_throw(&ctx->hmacCtx, aes_key->km, SIZEOF(aes_key->km)); - ASSERT(err == CX_OK); - err = cx_hmac_update((cx_hmac_t*) &ctx->hmacCtx, iv, ivSize); - ASSERT(err == CX_OK); + cx_err_t err = cx_hmac_sha256_init_no_throw(&ctx->hmacCtx, aesKey.km, SIZEOF(aesKey.km)); + explicit_bzero(&aesKey, SIZEOF(aesKey)); + if (err != CX_OK) { + return ERR_ASSERT; + } + err = cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, iv, ivSize); + if (err != CX_OK) { + return ERR_ASSERT; + } ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; @@ -160,32 +212,54 @@ __noinline_due_to_stack__ size_t dh_encode_init(dh_context_t* ctx, STATIC_ASSERT(SIZEOF(ctx->base64EncodingCache) >= SIZEOF(ctx->IV), "Cache too small"); memcpy(ctx->base64EncodingCache, ctx->IV, SIZEOF(ctx->IV)); ctx->base64EncodingCacheLen = SIZEOF(ctx->IV); + // This also sets outSize as desired return base64EncWholeBlocks(ctx->base64EncodingCache, &ctx->base64EncodingCacheLen, outBuffer, outSize); } -__noinline_due_to_stack__ size_t dh_encode_append(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize) { +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_encode_append(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + const uint8_t *inBuffer, + uint16_t inSize, + uint8_t *outBuffer, + uint16_t *outSize) { + // Crypto assets to be cleared + dh_aes_key_t aesKey; + explicit_bzero(&aesKey, SIZEOF(aesKey)); + TRACE("dh_encode_append"); TRACE_STACK_USAGE(); - ASSERT(inSize < BUFFER_SIZE_PARANOIA); - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); - ASSERT(aes_key->initialized_magic == DH_AES_KEY_INITIALIZED_MAGIC); - ASSERT(ctx->cacheLength < CX_AES_BLOCK_SIZE); + // sanity checks + if (inSize >= BUFFER_SIZE_PARANOIA) { + return ERR_ASSERT; + } + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + return ERR_ASSERT; + } + if (ctx->cacheLength >= CX_AES_BLOCK_SIZE) { + return ERR_ASSERT; + } STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); - size_t processedInput = 0; - size_t written = 0; + // Compute AES key + { + uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; + } + } + + uint16_t processedInput = 0; + uint16_t written = 0; while (1) { // fill ctx->buffer - size_t to_read = MIN(CX_AES_BLOCK_SIZE - ctx->cacheLength, inSize - processedInput); + uint16_t to_read = MIN(CX_AES_BLOCK_SIZE - ctx->cacheLength, inSize - processedInput); memcpy(ctx->cache + ctx->cacheLength, inBuffer + processedInput, to_read); ctx->cacheLength += to_read; processedInput += to_read; @@ -197,49 +271,104 @@ __noinline_due_to_stack__ size_t dh_encode_append(dh_context_t* ctx, break; } - written += processDHOneBlockFromCache(ctx, aes_key, outBuffer + written, outSize - written); + uint16_t restLength = *outSize - written; // remaining buffer + uint16_t err = processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; + } + written += restLength; // processDHOneBlockFromCache returns number of bytes written } TRACE("Leaving dh_encode_append, written: %d", (int) written); - return written; + *outSize = written; + return SUCCESS; } -__noinline_due_to_stack__ size_t dh_encode_finalize(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - uint8_t* outBuffer, - size_t outSize) { +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_encode_finalize(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + uint8_t *outBuffer, + uint16_t *outSize) { + // Crypto assets to be cleared + dh_aes_key_t aesKey; + explicit_bzero(&aesKey, SIZEOF(aesKey)); + // Crypto assets to be cleared in case of failure + explicit_bzero(outBuffer, *outSize); // It suffices to clear this after final hmac + TRACE("dh_encode_finalize"); + TRACE_STACK_USAGE(); - ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); - ASSERT(aes_key->initialized_magic == DH_AES_KEY_INITIALIZED_MAGIC); - ASSERT(ctx->cacheLength < CX_AES_BLOCK_SIZE); - TRACE("Cache length %d", (int) ctx->cacheLength); + // sanity checks + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + return ERR_ASSERT; + } + if (ctx->cacheLength >= CX_AES_BLOCK_SIZE) { + return ERR_ASSERT; + } + STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); TRACE_BUFFER(ctx->cache, ctx->cacheLength); + // Compute AES key + { + uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; + } + } + // fill the last cache block with integers equal to number of elements missing // if the next block is empty we create a block full of 0x10 (CX_AES_BLOCK_SIZE) STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); uint8_t fillValue = CX_AES_BLOCK_SIZE - ctx->cacheLength; - for (size_t i = ctx->cacheLength; i < CX_AES_BLOCK_SIZE; i++) { + for (uint16_t i = ctx->cacheLength; i < CX_AES_BLOCK_SIZE; i++) { ctx->cache[i] = fillValue; } ctx->cacheLength = CX_AES_BLOCK_SIZE; uint8_t written = 0; - written += processDHOneBlockFromCache(ctx, aes_key, outBuffer + written, outSize - written); + + // Encode last block + { + uint16_t restLength = *outSize - written; // remaining buffer + uint16_t err = processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength); + explicit_bzero(&aesKey, SIZEOF(aesKey)); + if (err != SUCCESS) { + return err; + } + written += restLength; + } // finalize hmac and append base64 encode it and append to cyphertext - size_t hmacOutSize = SIZEOF(ctx->base64EncodingCache) - ctx->base64EncodingCacheLen; - cx_err_t err = cx_hmac_final((cx_hmac_t*) &ctx->hmacCtx, - ctx->base64EncodingCache + ctx->base64EncodingCacheLen, - &hmacOutSize); - ASSERT(err == CX_OK); - ASSERT(hmacOutSize == DH_HMAC_SIZE); - ctx->base64EncodingCacheLen += DH_HMAC_SIZE; - written += base64EncWholeBlocks(ctx->base64EncodingCache, - &ctx->base64EncodingCacheLen, - outBuffer + written, - outSize - written); + { + size_t hmacOutSize = SIZEOF(ctx->base64EncodingCache) - ctx->base64EncodingCacheLen; + cx_err_t err = cx_hmac_final((cx_hmac_t *) &ctx->hmacCtx, + ctx->base64EncodingCache + ctx->base64EncodingCacheLen, + &hmacOutSize); + // cache now contains possibly sensitive data + if (err != CX_OK || hmacOutSize != DH_HMAC_SIZE) { + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + return ERR_ASSERT; + } + ctx->base64EncodingCacheLen += DH_HMAC_SIZE; + } + + // Encode cache content + { + uint16_t restLength = *outSize - written; // remaining buffer + uint16_t err = base64EncWholeBlocks(ctx->base64EncodingCache, + &ctx->base64EncodingCacheLen, + outBuffer + written, + &restLength); + if (err != SUCCESS) { + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + explicit_bzero(outBuffer, *outSize); + return err; + } + written += restLength; // processDHOneBlockFromCache returns number of bytes written + } // the last base64 encoding block uint8_t lastBlock[3] = {0, 0, 0}; @@ -247,7 +376,11 @@ __noinline_due_to_stack__ size_t dh_encode_finalize(dh_context_t* ctx, case 0: break; case 1: - ASSERT(outSize >= written + BASE64_OUT_BLOCK_SIZE); + if (*outSize < written + BASE64_OUT_BLOCK_SIZE) { + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + explicit_bzero(outBuffer, *outSize); + return ERR_ASSERT; + } lastBlock[0] = ctx->base64EncodingCache[0]; base64EncBlock(lastBlock, outBuffer + written); *(outBuffer + written + 2) = '='; @@ -255,7 +388,11 @@ __noinline_due_to_stack__ size_t dh_encode_finalize(dh_context_t* ctx, written += BASE64_OUT_BLOCK_SIZE; break; case 2: - ASSERT(outSize >= written + BASE64_OUT_BLOCK_SIZE); + if (*outSize < written + BASE64_OUT_BLOCK_SIZE) { + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + explicit_bzero(outBuffer, *outSize); + return ERR_ASSERT; + } lastBlock[0] = ctx->base64EncodingCache[0]; lastBlock[1] = ctx->base64EncodingCache[1]; base64EncBlock(lastBlock, outBuffer + written); @@ -263,122 +400,166 @@ __noinline_due_to_stack__ size_t dh_encode_finalize(dh_context_t* ctx, written += BASE64_OUT_BLOCK_SIZE; break; default: - ASSERT(false); + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + explicit_bzero(outBuffer, *outSize); + return ERR_ASSERT; } - return written; + *outSize = written; + return SUCCESS; + ; } -__noinline_due_to_stack__ size_t dh_encode(bip44_path_t* pathSpec, - public_key_t* publicKey, - const uint8_t* iv, - size_t ivSize, - const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize) { - dh_aes_key_t key; +#ifdef DEVEL +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode(bip44_path_t *pathSpec, + public_key_t *publicKey, + const uint8_t *iv, + uint16_t ivSize, + const uint8_t *inBuffer, + uint16_t inSize, + uint8_t *outBuffer, + uint16_t *outSize) { TRACE("dh_encode"); - ASSERT(inSize < BUFFER_SIZE_PARANOIA); - size_t written = 0; - - BEGIN_TRY { - TRY { - TRACE_STACK_USAGE(); - dh_init_aes_key(&key, pathSpec, publicKey); - - dh_context_t ctx; - written += - dh_encode_init(&ctx, &key, iv, ivSize, outBuffer + written, outSize - written); - TRACE("Init: written %d", written); - TRACE_BUFFER(outBuffer, written); - written += dh_encode_append(&ctx, - &key, - inBuffer, - inSize, - outBuffer + written, - outSize - written); - TRACE("Append: written %d", written); - TRACE_BUFFER(outBuffer, written); - written += dh_encode_finalize(&ctx, &key, outBuffer + written, outSize - written); - TRACE("Finalize: written %d", written); - TRACE_BUFFER(outBuffer, written); - } - FINALLY { - explicit_bzero(&key, sizeof(key)); - } + dh_context_t ctx; + + uint16_t written = 0; + uint16_t remaining = *outSize - written; + uint16_t err = + dh_encode_init(&ctx, pathSpec, publicKey, iv, ivSize, outBuffer + written, &remaining); + if (err != SUCCESS) { + return err; } - END_TRY; + written += remaining; + + remaining = *outSize - written; + err = dh_encode_append(&ctx, + pathSpec, + publicKey, + inBuffer, + inSize, + outBuffer + written, + &remaining); + if (err != SUCCESS) { + return err; + } + written += remaining; + + remaining = *outSize - written; + err = dh_encode_finalize(&ctx, pathSpec, publicKey, outBuffer + written, &remaining); + if (err != SUCCESS) { + return err; + } + written += remaining; - return written; + *outSize = written; + return SUCCESS; } +#endif -__noinline_due_to_stack__ static void validateHmac(dh_aes_key_t* aes_key, - const uint8_t* buffer, - size_t inSize) { - VALIDATE(inSize >= DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE, ERR_INVALID_DATA); +__noinline_due_to_stack__ static WARN_UNUSED_RESULT uint16_t validateHmac(dh_aes_key_t *aesKey, + const uint8_t *buffer, + uint16_t inSize) { + if (inSize < DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE) { + return ERR_INVALID_DATA; + } cx_hmac_sha256_t hmac; - cx_err_t err = cx_hmac_sha256_init_no_throw(&hmac, aes_key->km, SIZEOF(aes_key->km)); - ASSERT(err == CX_OK); - err = cx_hmac_update((cx_hmac_t*) &hmac, buffer, inSize - DH_HMAC_SIZE); - ASSERT(err == CX_OK); + cx_err_t err = cx_hmac_sha256_init_no_throw(&hmac, aesKey->km, SIZEOF(aesKey->km)); + if (err != CX_OK) { + return ERR_ASSERT; + } + err = cx_hmac_update((cx_hmac_t *) &hmac, buffer, inSize - DH_HMAC_SIZE); + if (err != CX_OK) { + return ERR_ASSERT; + } uint8_t hmacBuf[DH_HMAC_SIZE]; size_t outLen = SIZEOF(hmacBuf); - err = cx_hmac_final((cx_hmac_t*) &hmac, hmacBuf, &outLen); - ASSERT(err == CX_OK); - ASSERT(outLen == DH_HMAC_SIZE); - VALIDATE(!memcmp(hmacBuf, buffer + inSize - DH_HMAC_SIZE, DH_HMAC_SIZE), ERR_INVALID_HMAC); + err = cx_hmac_final((cx_hmac_t *) &hmac, hmacBuf, &outLen); + if (err != CX_OK || outLen != DH_HMAC_SIZE) { + return ERR_ASSERT; + } + if (memcmp(hmacBuf, buffer + inSize - DH_HMAC_SIZE, DH_HMAC_SIZE)) { + return ERR_INVALID_HMAC; + } + return SUCCESS; } -__noinline_due_to_stack__ size_t dh_decode(bip44_path_t* pathSpec, - public_key_t* publicKey, - uint8_t* buffer, - size_t inSize) { - VALIDATE(inSize >= DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE, ERR_INVALID_DATA); - VALIDATE(inSize % CX_AES_BLOCK_SIZE == 0, ERR_INVALID_DATA); - - size_t read = - DH_AES_IV_SIZE; // we do not decode IV, this also creates a buffer so we can decode inplace - size_t written = 0; - dh_aes_key_t aes_key; - BEGIN_TRY { - TRY { - dh_init_aes_key(&aes_key, pathSpec, publicKey); - - // validate HMAC - validateHmac(&aes_key, buffer, inSize); - TRACE("HMAC validation succesfull."); - - // initiate DH decryptions - uint8_t IV[CX_AES_BLOCK_SIZE]; - memcpy(IV, buffer, SIZEOF(IV)); - - for (; read < inSize - DH_HMAC_SIZE; read += CX_AES_BLOCK_SIZE) { - // 1. Decode next block - ASSERT(read - written == CX_AES_BLOCK_SIZE); - cx_err_t err = cx_aes_dec_block(&aes_key.aesKey, buffer + read, buffer + written); - ASSERT(err == CX_OK); - // 2. XOR with IV - for (size_t i = 0; i < CX_AES_BLOCK_SIZE; i++) { - buffer[written + i] ^= IV[i]; - } - // 3. Cyphertext is the new IV ... we do not care that we copy part of HMAC in last - // iteration here - memcpy(IV, buffer + read, CX_AES_BLOCK_SIZE); - written += CX_AES_BLOCK_SIZE; - } +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pathSpec, + public_key_t *publicKey, + uint8_t *buffer, + uint16_t *size) { + // Crypto assets to be cleared + dh_aes_key_t aesKey; + explicit_bzero(&aesKey, SIZEOF(aesKey)); + // Crypto assets to be cleared in case of failure + TRACE_BUFFER(buffer, *size); // buffer after decoding + + if (*size < DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE) { + return ERR_INVALID_DATA; + } + if (*size % CX_AES_BLOCK_SIZE != 0) { + return ERR_INVALID_DATA; + } + + // we do not decode IV, this also creates a buffer so we can decode inplace + uint16_t read = DH_AES_IV_SIZE; + uint16_t written = 0; + { + uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; } - FINALLY { - explicit_bzero(&aes_key, sizeof(aes_key)); + } + + // validate HMAC + { + uint16_t err = validateHmac(&aesKey, buffer, *size); + if (err != SUCCESS) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return err; } + + TRACE("HMAC validation succesfull."); } - END_TRY; + + // initiate DH decryptions + uint8_t IV[CX_AES_BLOCK_SIZE]; + memcpy(IV, buffer, SIZEOF(IV)); + + for (; read < *size - DH_HMAC_SIZE; read += CX_AES_BLOCK_SIZE) { + // 1. Decode next block + if (read - written != CX_AES_BLOCK_SIZE) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + explicit_bzero(buffer, *size); + return ERR_ASSERT; + } + cx_err_t err = cx_aes_dec_block(&aesKey.aesKey, buffer + read, buffer + written); + if (err != CX_OK) { + explicit_bzero(&aesKey, SIZEOF(aesKey)); + explicit_bzero(buffer, *size); + return ERR_ASSERT; + } + // 2. XOR with IV + for (uint16_t i = 0; i < CX_AES_BLOCK_SIZE; i++) { + buffer[written + i] ^= IV[i]; + } + // 3. Cyphertext is the new IV ... we do not care that we copy part of HMAC in last + // iteration here + memcpy(IV, buffer + read, CX_AES_BLOCK_SIZE); + written += CX_AES_BLOCK_SIZE; + } + + explicit_bzero(&aesKey, SIZEOF(aesKey)); TRACE("Finishing decription, written:%d, lastCharacter:%d", written, buffer[written - 1]); // Calculate resulting length based on the last decoded value - ASSERT(written != 0); - ASSERT(written >= buffer[written - 1]); - return written - buffer[written - 1]; + if (written == 0 || written < buffer[written - 1]) { + explicit_bzero(buffer, *size); + return ERR_INVALID_DATA; + } + + *size = written - buffer[written - 1]; + return SUCCESS; } diff --git a/src/diffieHellman.h b/src/diffieHellman.h index 4006ed2c..931e6fe1 100644 --- a/src/diffieHellman.h +++ b/src/diffieHellman.h @@ -39,63 +39,146 @@ typedef struct { // Aes key is secret shared between us and the second party typedef struct { - uint16_t initialized_magic; cx_aes_key_t aesKey; uint8_t km[DH_KM_SIZE]; } dh_aes_key_t; -// You may need to call multiple times if encryption spans multiple APDU's -// You may want to do something like -// APDU1 - dh_init_aes_key, dh_encode_init, dh_encode_append -// APDU2 - dh_init_aes_key, dh_encode_append -// APDU3 - dh_init_aes_key, dh_encode_append, dh_encode_finalize -// Do not forget to guarantee that dh_aes_key_t is zeroes at all times - -__noinline_due_to_stack__ void dh_init_aes_key(dh_aes_key_t* dhKey, - const bip44_path_t* pathSpec, - const public_key_t* publicKey); - -// Output data are base64 encrypted -__noinline_due_to_stack__ size_t dh_encode_init(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - const uint8_t* iv, - size_t ivSize, - uint8_t* outBuffer, - size_t outSize); - -// Output data are base64 encrypted -__noinline_due_to_stack__ size_t dh_encode_append(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize); - -// Output data are base64 encrypted -__noinline_due_to_stack__ size_t dh_encode_finalize(dh_context_t* ctx, - const dh_aes_key_t* aes_key, - uint8_t* outBuffer, - size_t outSize); - -// Convenience function to make all in one step -// Output data are base64 encrypted -__noinline_due_to_stack__ size_t dh_encode(bip44_path_t* pathSpec, - public_key_t* publicKey, - const uint8_t* iv, - size_t ivSize, - const uint8_t* inBuffer, - size_t inSize, - uint8_t* outBuffer, - size_t outSize); - -// Inplace decoding function. -// Input data is NOT base64 encrypted -__noinline_due_to_stack__ size_t dh_decode(bip44_path_t* pathSpec, - public_key_t* publicKey, - uint8_t* buffer, - size_t inSize); +/** + * @brief Generates shared secret AES key. Use only in crypto functions to guarantee cleanup. + * + * @param[out] dhKey Resulting AES key. + * + * @param[in] pathSpec Our derivation path. + * + * @param[in] publicKey Their public key. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_key_t *publicKey); + +/** + * @brief Initiates DH+Base64 ecoding. + * + * @param[out] ctx DH encoding context. + * + * @param[in] pathSpec Our derivation path. + * + * @param[in] publicKey Their public key. + * + * @param[in] iv Initiatlization vector. + * + * @param[in] ivSize Size of initialization vector. + * + * @param[out] outBuffer Output buffer, initialization produces cyphertext. + * + * @param[in, out] outSize Size of output buffer. returns the number of new bytes. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode_init(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + const uint8_t *iv, + uint16_t ivSize, + uint8_t *outBuffer, + uint16_t *outSize); + +/** + * @brief Append to DH+Base64 ecoded data. + * + * @param[in, out] ctx DH encoding context. + * + * @param[in] pathSpec Our derivation path. + * + * @param[in] publicKey Their public key. + * + * @param[in] inBuffer Input buffer. + * + * @param[in] inSize Input buffer size. + * + * @param[out] outBuffer Output buffer, initialization produces cyphertext. + * + * @param[in, out] outSize Size of output buffer. returns the number of new bytes. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_encode_append(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + const uint8_t *inBuffer, + uint16_t inSize, + uint8_t *outBuffer, + uint16_t *outSize); + +/** + * @brief Finish DH+Base64 encoding. + * + * @param[in] ctx DH encoding context. + * + * @param[in] pathSpec Our derivation path. + * + * @param[in] publicKey Their public key. + * + * @param[out] outBuffer Output buffer, initialization produces cyphertext. + * + * @param[in, out] outSize Size of output buffer. returns the number of new bytes. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t +dh_encode_finalize(dh_context_t *ctx, + const bip44_path_t *pathSpec, + const public_key_t *publicKey, + uint8_t *outBuffer, + uint16_t *outSize); + +/** + * @brief Inplace DH decoding function. Input data is NOT base64 encrypted + * + * @param[in] pathSpec Our derivation path. + * + * @param[in] publicKey Their public key. + * + * @param[out] buffer Output buffer, initialization produces cyphertext. + * + * @param[in, out] size Size of output buffer. returns the number of new bytes. + * + * @return Error code: + * - SUCCESS on success + * - ERR_REJECTED_BY_POLICY + * - ERR_INVALID_DATA + * - ERR_INVALID_HMAC + * - ERR_ASSERT for other unexpected errors + */ +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pathSpec, + public_key_t *publicKey, + uint8_t *buffer, + uint16_t *size); #ifdef DEVEL +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode(bip44_path_t *pathSpec, + public_key_t *publicKey, + const uint8_t *iv, + uint16_t ivSize, + const uint8_t *inBuffer, + uint16_t inSize, + uint8_t *outBuffer, + uint16_t *outSize); + __noinline_due_to_stack__ void run_diffieHellman_test(); #endif // DEVEL diff --git a/src/diffieHellmann_test.c b/src/diffieHellmann_test.c index 76d7fd31..e26f9e72 100644 --- a/src/diffieHellmann_test.c +++ b/src/diffieHellmann_test.c @@ -57,14 +57,18 @@ __noinline_due_to_stack__ static void run_dh_encode_tests() { uint8_t msg[50]; \ size_t msgLen = decode_hex(msgHex, msg, SIZEOF(msg)); \ uint8_t encMsg[200]; \ - size_t encMsgLength = dh_encode(&pathSpec, \ - &publicKey, \ - IV, \ - DH_AES_IV_SIZE, \ - msg, \ - msgLen, \ - encMsg, \ - SIZEOF(encMsg)); \ + uint16_t encMsgLength = SIZEOF(encMsg); \ + uint16_t err = dh_encode(&pathSpec, \ + &publicKey, \ + IV, \ + DH_AES_IV_SIZE, \ + msg, \ + msgLen, \ + encMsg, \ + &encMsgLength); \ + if (err != SUCCESS) { \ + THROW(err); \ + } \ TRACE_BUFFER(encMsg, encMsgLength); \ ASSERT(encMsgLength == strlen(expectedEncMsg)); \ EXPECT_EQ_BYTES(encMsg, expectedEncMsg, strlen(expectedEncMsg)); \ @@ -132,9 +136,6 @@ __noinline_due_to_stack__ static void run_dh_encode_init_append_finalize_tests() const uint8_t IV[DH_AES_IV_SIZE] = {100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100}; - dh_aes_key_t key; - dh_init_aes_key(&key, &pathSpec, &publicKey); - uint8_t inBuffer[50]; uint8_t outBuffer[160]; const char* inBufferHex = @@ -146,15 +147,16 @@ __noinline_due_to_stack__ static void run_dh_encode_init_append_finalize_tests() "llUGwVFl+lJ3Qpxm5u0fLG9kxuEGaN18XnBMEu6iJXmhERjaQhHhN4L/" "gZPs9AD9BNvPMXpd9H9EnR27den0hCjQqECg6QE5dZrTnSiZDLN/Yg=="; - size_t outBufferLength = 0; - outBufferLength = dh_encode(&pathSpec, - &publicKey, - IV, - DH_AES_IV_SIZE, - inBuffer, - inBufferLength, - outBuffer, - SIZEOF(outBuffer)); + uint16_t outBufferLength = SIZEOF(outBuffer); + uint16_t err = dh_encode(&pathSpec, + &publicKey, + IV, + DH_AES_IV_SIZE, + inBuffer, + inBufferLength, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); TRACE_BUFFER(outBuffer, outBufferLength); ASSERT(outBufferLength == strlen(expected)); EXPECT_EQ_BYTES(expected, outBuffer, outBufferLength); @@ -165,43 +167,87 @@ __noinline_due_to_stack__ static void run_dh_encode_init_append_finalize_tests() TRACE_STACK_USAGE(); // due to memory limitations we reuse static tx data dh_context_t* ctx = &instructionState.signTransactionContext.dhContext; - outBufferLength = - dh_encode_init(ctx, &key, IV, SIZEOF(IV), outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_init(ctx, + &pathSpec, + &publicKey, + IV, + SIZEOF(IV), + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 20); EXPECT_EQ_BYTES(expected + written, outBuffer, 20); written += 20; // Cache 0b, 1b { - outBufferLength = - dh_encode_append(ctx, &key, inBuffer + read, 1, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_append(ctx, + &pathSpec, + &publicKey, + inBuffer + read, + 1, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 0); read += 1; // Cache 1b, 1b } { - outBufferLength = - dh_encode_append(ctx, &key, inBuffer + read, 14, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_append(ctx, + &pathSpec, + &publicKey, + inBuffer + read, + 14, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 0); read += 14; // Cache 15b, 1b } - outBufferLength = - dh_encode_append(ctx, &key, inBuffer + read, 18, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_append(ctx, + &pathSpec, + &publicKey, + inBuffer + read, + 18, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 44); EXPECT_EQ_BYTES(expected + written, outBuffer, 44); read += 18, written += 44; // Cache 1b, 0b - outBufferLength = - dh_encode_append(ctx, &key, inBuffer + read, 1, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_append(ctx, + &pathSpec, + &publicKey, + inBuffer + read, + 1, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 0); read += 1; // Cache 2b, 0b - outBufferLength = - dh_encode_append(ctx, &key, inBuffer + read, 14, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_append(ctx, + &pathSpec, + &publicKey, + inBuffer + read, + 14, + outBuffer, + &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 20); EXPECT_EQ_BYTES(expected + written, outBuffer, 20); read += 14, written += 20; // Cache 0b, 1b - outBufferLength = dh_encode_finalize(ctx, &key, outBuffer, SIZEOF(outBuffer)); + outBufferLength = SIZEOF(outBuffer); + err = dh_encode_finalize(ctx, &pathSpec, &publicKey, outBuffer, &outBufferLength); + ASSERT(err == SUCCESS); ASSERT(outBufferLength == 68); EXPECT_EQ_BYTES(expected + written, outBuffer, 68); written += 68; @@ -240,15 +286,18 @@ __noinline_due_to_stack__ static void run_dh_decode_tests() { { \ const char* msgHex = msgHex_; \ uint8_t msg[120]; \ - size_t msgLen = decode_hex(msgHex, msg, SIZEOF(msg)); \ + uint16_t msgLen = decode_hex(msgHex, msg, SIZEOF(msg)); \ const char* expectedDecMsgHex = expectedDecMsgHex_; \ uint8_t expectedDecMsg[100]; \ size_t expectedMsgLen = \ decode_hex(expectedDecMsgHex, expectedDecMsg, SIZEOF(expectedDecMsg)); \ - size_t decMsgLen = dh_decode(&pathSpec, &publicKey, msg, msgLen); \ - TRACE("Decoded mesage %d, %d", decMsgLen, expectedMsgLen); \ - ASSERT(decMsgLen == expectedMsgLen); \ - EXPECT_EQ_BYTES(msg, expectedDecMsg, decMsgLen); \ + uint16_t err = dh_decode(&pathSpec, &publicKey, msg, &msgLen); \ + if (err != SUCCESS) { \ + THROW(err); \ + } \ + TRACE("Decoded mesage %d, %d", msgLen, expectedMsgLen); \ + ASSERT(msgLen == expectedMsgLen); \ + EXPECT_EQ_BYTES(msg, expectedDecMsg, msgLen); \ } TESTCASE( "000102030405060708090a0b0c0d0e0f9508b492f96f067cc72ef8c7c24ac2072310c4e1d36bd6737958f0" @@ -321,9 +370,9 @@ __noinline_due_to_stack__ static void run_dh_decode_failed_hmac_tests() { "05576d60a63b30e52db993fdb53f67ba03cd0abed894f54929ac6addfd7076970597a43a36c525ad1fc4349c69" "be21718ab07bc639172663927cb075fa777797e0c1c4"; uint8_t msg[120]; - size_t msgLen = decode_hex(msgHex, msg, SIZEOF(msg)); + uint16_t msgLen = decode_hex(msgHex, msg, SIZEOF(msg)); - EXPECT_THROWS(dh_decode(&pathSpec, &publicKey, msg, msgLen), ERR_INVALID_HMAC); + ASSERT(dh_decode(&pathSpec, &publicKey, msg, &msgLen) == ERR_INVALID_HMAC); } __noinline_due_to_stack__ void run_diffieHellman_test() { diff --git a/src/getPublicKey.c b/src/getPublicKey.c index 25fa0f41..f97df1a5 100644 --- a/src/getPublicKey.c +++ b/src/getPublicKey.c @@ -80,7 +80,7 @@ static void runGetPublicKeyUIFlow() { { // Calculation uint16_t err = derivePublicKey(&ctx->pathSpec, &ctx->pubKey); - if (err != CX_OK) { + if (err != SUCCESS) { THROW(err); } ctx->responseReadyMagic = RESPONSE_READY_MAGIC; diff --git a/src/keyDerivation.c b/src/keyDerivation.c index 5b2530af..7e946f2c 100644 --- a/src/keyDerivation.c +++ b/src/keyDerivation.c @@ -17,6 +17,12 @@ static uint8_t const SECP256K1_N[] = { __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t* pathSpec, private_key_t* privateKey) { + // Crypto assets to be cleared + uint8_t privateKeySeed[PRIVATE_KEY_SEED_LEN]; + explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); + // Crypto assets to be cleared in case of failure + explicit_bzero(privateKey, SIZEOF(*privateKey)); + if (policyDerivePrivateKey(pathSpec) == POLICY_DENY) { return ERR_REJECTED_BY_POLICY; } @@ -26,11 +32,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip return ERR_ASSERT; } - TRACE(); - uint8_t privateKeySeed[PRIVATE_KEY_SEED_LEN]; - explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); - STATIC_ASSERT(CX_APILEVEL >= 5, "unsupported api level"); + TRACE(); cx_err_t err = os_derive_bip32_with_seed_no_throw(HDW_NORMAL, CX_CURVE_SECP256K1, @@ -46,8 +49,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip } err = cx_ecfp_init_private_key_no_throw(CX_CURVE_SECP256K1, privateKeySeed, 32, privateKey); + explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); if (err != CX_OK) { - explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); explicit_bzero(privateKey, SIZEOF(*privateKey)); return ERR_ASSERT; } @@ -57,6 +60,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t* pathSpec, public_key_t* publicKey) { + // Crypto assets to be cleared private_key_t privateKey; explicit_bzero(&privateKey, SIZEOF(privateKey)); @@ -78,11 +82,12 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 publicKey, &privateKey, 1); // 1 - private key preserved + explicit_bzero(&privateKey, SIZEOF(privateKey)); if (err != CX_OK) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); return ERR_ASSERT; } + TRACE(); return SUCCESS; } @@ -92,14 +97,20 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path uint8_t hashBuf[SHA_256_SIZE], uint8_t* signature, size_t signatureLen) { + // Crypto assets to be cleared + private_key_t privateKey; + explicit_bzero(&privateKey, SIZEOF(privateKey)); + uint8_t V[33]; + explicit_bzero(V, SIZEOF(V)); + uint8_t K[32]; + explicit_bzero(K, SIZEOF(K)); + // Crypto assets to be cleared in case of failure + explicit_bzero(signature, signatureLen); + if (signatureLen < 200) { return ERR_ASSERT; } - // Derive keys and sign the transaction, setup - private_key_t privateKey; - explicit_bzero(&privateKey, SIZEOF(privateKey)); - // We derive the private key { uint16_t err = derivePrivateKey(wittnessPath, &privateKey); @@ -112,9 +123,6 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path } // We sign the hash - explicit_bzero(signature, signatureLen); - uint8_t V[33]; - uint8_t K[32]; int tries = 0; // Loop until a candidate matching the canonical signature is found @@ -131,6 +139,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path if (err != CX_OK) { explicit_bzero(&privateKey, SIZEOF(privateKey)); explicit_bzero(signature, signatureLen); + explicit_bzero(V, SIZEOF(V)); + explicit_bzero(K, SIZEOF(K)); return ERR_ASSERT; } } else { @@ -138,6 +148,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path if (err != CX_OK) { explicit_bzero(&privateKey, SIZEOF(privateKey)); explicit_bzero(signature, signatureLen); + explicit_bzero(V, SIZEOF(V)); + explicit_bzero(K, SIZEOF(K)); return ERR_ASSERT; } } @@ -155,6 +167,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path if (err != CX_OK) { explicit_bzero(&privateKey, SIZEOF(privateKey)); explicit_bzero(signature, signatureLen); + explicit_bzero(V, SIZEOF(V)); + explicit_bzero(K, SIZEOF(K)); return ERR_ASSERT; } TRACE_BUFFER(signature + 100, 100); @@ -176,5 +190,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path } explicit_bzero(&privateKey, sizeof(privateKey)); + explicit_bzero(V, SIZEOF(V)); + explicit_bzero(K, SIZEOF(K)); return SUCCESS; } diff --git a/src/signTransaction.c b/src/signTransaction.c index b44deedb..97f479a0 100644 --- a/src/signTransaction.c +++ b/src/signTransaction.c @@ -40,35 +40,27 @@ enum { // &ctx->dhContext) are needed for encryption static void processShaAndPosibleDHAndPrepareResponse() { if (ctx->dhIsActive) { - dh_aes_key_t aesKey; - BEGIN_TRY { - TRY { - // Compute AES key - dh_init_aes_key(&aesKey, &ctx->wittnessPath, &ctx->otherPubkey); - - // Encode message chunk - ctx->responseLength = dh_encode_append(&ctx->dhContext, - &aesKey, - ctx->dataToAppendToTx, - ctx->dataToAppendToTxLen, - G_io_apdu_buffer, - SIZEOF(G_io_apdu_buffer)); - CX_THROW(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength)); - VALIDATE( - ctx->countedSectionDifference + ctx->responseLength >= ctx->dataToAppendToTxLen, - ERR_INVALID_STATE); - ctx->countedSectionDifference = - ctx->countedSectionDifference + ctx->responseLength - ctx->dataToAppendToTxLen; - TRACE("CS diff %d from:%d, %d", - (int) ctx->countedSectionDifference, - (int) ctx->responseLength, - (int) ctx->dataToAppendToTxLen); - } - FINALLY { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - } + uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); + uint16_t err = dh_encode_append(&ctx->dhContext, + &ctx->wittnessPath, + &ctx->otherPubkey, + ctx->dataToAppendToTx, + ctx->dataToAppendToTxLen, + G_io_apdu_buffer, + &bufferLen); + if (err != SUCCESS) { + THROW(err); } - END_TRY; + ctx->responseLength = bufferLen; + CX_THROW(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength)); + VALIDATE(ctx->countedSectionDifference + ctx->responseLength >= ctx->dataToAppendToTxLen, + ERR_INVALID_STATE); + ctx->countedSectionDifference = + ctx->countedSectionDifference + ctx->responseLength - ctx->dataToAppendToTxLen; + TRACE("CS diff %d from:%d, %d", + (int) ctx->countedSectionDifference, + (int) ctx->responseLength, + (int) ctx->dataToAppendToTxLen); } else { cx_err_t err = sha_256_append(&ctx->hashContext, ctx->dataToAppendToTx, ctx->dataToAppendToTxLen); @@ -703,37 +695,32 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( TRACE_STACK_USAGE(); VALIDATE(!ctx->dhIsActive, ERR_INVALID_STATE); - dh_aes_key_t aesKey; - BEGIN_TRY { - TRY { - TRACE_STACK_USAGE(); - // Compute AES key - dh_init_aes_key(&aesKey, &ctx->wittnessPath, &ctx->otherPubkey); - - // Generate IV - uint8_t IV[DH_AES_IV_SIZE]; - cx_rng_no_throw(IV, SIZEOF(IV)); - - // INIT dh context - STATIC_ASSERT(DH_AES_IV_SIZE == CX_AES_BLOCK_SIZE, "Unexpected IV length"); - ctx->dhCountedSectionEntryLevel = ctx->countedSections.currentLevel; - ctx->responseLength = dh_encode_init(&ctx->dhContext, - &aesKey, - IV, - SIZEOF(IV), - G_io_apdu_buffer, - SIZEOF(G_io_apdu_buffer)); - if (ctx->responseLength != 20) { // first 5 blocks - THROW(ERR_ASSERT); - } - ctx->countedSectionDifference = ctx->responseLength; - TRACE("CS diff %d", (int) ctx->responseLength); - } - FINALLY { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - } + TRACE_STACK_USAGE(); + // Generate IV + uint8_t IV[DH_AES_IV_SIZE]; + cx_rng_no_throw(IV, SIZEOF(IV)); + + // INIT dh context + STATIC_ASSERT(DH_AES_IV_SIZE == CX_AES_BLOCK_SIZE, "Unexpected IV length"); + ctx->dhCountedSectionEntryLevel = ctx->countedSections.currentLevel; + uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); + + uint16_t err = dh_encode_init(&ctx->dhContext, + &ctx->wittnessPath, + &ctx->otherPubkey, + IV, + SIZEOF(IV), + G_io_apdu_buffer, + &bufferLen); + if (err != SUCCESS) { + THROW(err); + } + ctx->responseLength = bufferLen; + if (ctx->responseLength != 20) { // first 5 blocks + THROW(ERR_ASSERT); } - END_TRY; + ctx->countedSectionDifference = ctx->responseLength; + TRACE("CS diff %d", (int) ctx->responseLength); ASSERT(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength) == CX_OK); ctx->dhIsActive = true; @@ -809,22 +796,16 @@ __noinline_due_to_stack__ void signTx_handleEndDHEncodingAPDU( VALIDATE(ctx->dhCountedSectionEntryLevel == ctx->countedSections.currentLevel, ERR_INVALID_STATE); - dh_aes_key_t aesKey; - BEGIN_TRY { - TRY { - // Compute AES key - dh_init_aes_key(&aesKey, &ctx->wittnessPath, &ctx->otherPubkey); - - ctx->responseLength = dh_encode_finalize(&ctx->dhContext, - &aesKey, - G_io_apdu_buffer, - SIZEOF(G_io_apdu_buffer)); - } - FINALLY { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - } + uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); + uint16_t err = dh_encode_finalize(&ctx->dhContext, + &ctx->wittnessPath, + &ctx->otherPubkey, + G_io_apdu_buffer, + &bufferLen); + if (err != SUCCESS) { + THROW(err); } - END_TRY; + ctx->responseLength = bufferLen; ctx->countedSectionDifference += ctx->responseLength; TRACE("CS diff %d from:%d", From 83703bfb9435b739b6be6a4f32485510a8c89755 Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Sat, 7 Oct 2023 10:46:32 +0200 Subject: [PATCH 3/6] Move dependencies to non-throwing crypto modules --- src/eos_utils.c | 153 +-------------------------------------- src/eos_utils.h | 14 ---- src/keyDerivation.c | 161 ++++++++++++++++++++++++++++++++++++++++-- src/signTransaction.c | 1 - 4 files changed, 155 insertions(+), 174 deletions(-) diff --git a/src/eos_utils.c b/src/eos_utils.c index 62c9e769..0547b36d 100644 --- a/src/eos_utils.c +++ b/src/eos_utils.c @@ -20,155 +20,6 @@ #include "cx.h" #include "utils.h" -#define FORWARD_CX_ERROR(call) \ - { \ - cx_err_t callResult = (call); \ - if (callResult != CX_OK) return callResult; \ - } - -/** - * EOS way to check if a signature is canonical :/ - */ -unsigned char check_canonical(uint8_t *rs) { - return !(rs[0] & 0x80) && !(rs[0] == 0 && !(rs[1] & 0x80)) && !(rs[32] & 0x80) && - !(rs[32] == 0 && !(rs[33] & 0x80)); -} - -int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { - int length; - int offset = 2; - int delta = 0; - if (der[offset + 2] == 0) { - length = der[offset + 1] - 1; - offset += 3; - } else { - length = der[offset + 1]; - offset += 2; - } - if ((length < 0) || (length > 32)) { - return 0; - } - while ((length + delta) < 32) { - sig[delta++] = 0; - } - memcpy(sig + delta, der + offset, length); - - delta = 0; - offset += length; - if (der[offset + 2] == 0) { - length = der[offset + 1] - 1; - offset += 3; - } else { - length = der[offset + 1]; - offset += 2; - } - if ((length < 0) || (length > 32)) { - return 0; - } - while ((length + delta) < 32) { - sig[32 + delta++] = 0; - } - memcpy(sig + 32 + delta, der + offset, length); - - return 1; -} - -/** - * The nonce generated by internal library CX_RND_RFC6979 is not compatible - * with EOS. So this is the way to generate nonve for EOS. - * Arguments (by relatko): - * - rnd - out - * - h1 - hash, in - * - x - private key, in - * - x_len - private key length - * - q - SECP256K1_N, in - * - q_len - 32, in - * - V, out - * - K, out - */ -cx_err_t rng_rfc6979(unsigned char *rnd, - unsigned char *h1, - unsigned char *x, - unsigned int x_len, - const unsigned char *q, - unsigned int q_len, - unsigned char *V, - unsigned char *K) { - unsigned int h_len, found, i; - cx_hmac_sha256_t hmac; - - h_len = 32; - // a. h1 as input - - // loop for a candidate - found = 0; - while (!found) { - if (x) { - // b. Set: V = 0x01 0x01 0x01 ... 0x01 - memset(V, 0x01, h_len); - // c. Set: K = 0x00 0x00 0x00 ... 0x00 - memset(K, 0x00, h_len); - // d. Set: K = HMAC_K(V || 0x00 || int2octets(x) || bits2octets(h1)) - V[h_len] = 0; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); - // e. Set: V = HMAC_K(V) - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); - // f. Set: K = HMAC_K(V || 0x01 || int2octets(x) || bits2octets(h1)) - V[h_len] = 1; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); - // g. Set: V = HMAC_K(V) -- - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); - // initial setup only once - x = NULL; - } else { - // h.3 K = HMAC_K(V || 0x00) - V[h_len] = 0; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); - // h.3 V = HMAC_K(V) - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); - } - - // generate candidate - /* Shortcut: As only secp256k1/sha256 is supported, the step h.2 : - * While tlen < qlen, do the following: - * V = HMAC_K(V) - * T = T || V - * is replace by - * V = HMAC_K(V) - */ - x_len = q_len; - while (x_len) { - if (x_len < h_len) { - h_len = x_len; - } - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); - memcpy(rnd, V, h_len); - x_len -= h_len; - } - - // h.3 Check T is < n - for (i = 0; i < q_len; i++) { - if (V[i] < q[i]) { - found = 1; - break; - } - } - } - - return CX_OK; -} - unsigned char const BASE58ALPHABET[58] = { '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', @@ -237,9 +88,7 @@ uint32_t compressed_public_key_to_wif(const uint8_t *publicKey, uint8_t check[20]; cx_ripemd160_t riprip; cx_ripemd160_init(&riprip); - if (cx_hash_no_throw(&riprip.header, CX_LAST, temp, 33, check, sizeof(check)) != CX_OK) { - return 0; - } + ASSERT(cx_hash_no_throw(&riprip.header, CX_LAST, temp, 33, check, sizeof(check)) == CX_OK); memcpy(temp + 33, check, 4); explicit_bzero(out, outLength); diff --git a/src/eos_utils.h b/src/eos_utils.h index e303af84..b05bca2c 100644 --- a/src/eos_utils.h +++ b/src/eos_utils.h @@ -22,20 +22,6 @@ #include #include "cx.h" -unsigned char check_canonical(uint8_t *rs); - -int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig); - -cx_err_t rng_rfc6979(unsigned char *rnd, - unsigned char *h1, - unsigned char *x, - unsigned int x_len, - const unsigned char *q, - unsigned int q_len, - unsigned char *V, - unsigned char *K); - -// returns 0 on error uint32_t public_key_to_wif(const uint8_t *publicKey, uint32_t keyLength, char *out, diff --git a/src/keyDerivation.c b/src/keyDerivation.c index 7e946f2c..977f845b 100644 --- a/src/keyDerivation.c +++ b/src/keyDerivation.c @@ -15,8 +15,14 @@ static uint8_t const SECP256K1_N[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41}; -__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t* pathSpec, - private_key_t* privateKey) { +#define FORWARD_CX_ERROR(call) \ + { \ + cx_err_t callResult = (call); \ + if (callResult != CX_OK) return callResult; \ + } + +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t *pathSpec, + private_key_t *privateKey) { // Crypto assets to be cleared uint8_t privateKeySeed[PRIVATE_KEY_SEED_LEN]; explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); @@ -58,8 +64,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip return SUCCESS; } -__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t* pathSpec, - public_key_t* publicKey) { +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t *pathSpec, + public_key_t *publicKey) { // Crypto assets to be cleared private_key_t privateKey; explicit_bzero(&privateKey, SIZEOF(privateKey)); @@ -91,11 +97,152 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 return SUCCESS; } +// EOS way to check if a signature is canonical :/ +static unsigned char check_canonical(uint8_t *rs) { + return !(rs[0] & 0x80) && !(rs[0] == 0 && !(rs[1] & 0x80)) && !(rs[32] & 0x80) && + !(rs[32] == 0 && !(rs[33] & 0x80)); +} + +static int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { + int length; + int offset = 2; + int delta = 0; + if (der[offset + 2] == 0) { + length = der[offset + 1] - 1; + offset += 3; + } else { + length = der[offset + 1]; + offset += 2; + } + if ((length < 0) || (length > 32)) { + return 0; + } + while ((length + delta) < 32) { + sig[delta++] = 0; + } + memcpy(sig + delta, der + offset, length); + + delta = 0; + offset += length; + if (der[offset + 2] == 0) { + length = der[offset + 1] - 1; + offset += 3; + } else { + length = der[offset + 1]; + offset += 2; + } + if ((length < 0) || (length > 32)) { + return 0; + } + while ((length + delta) < 32) { + sig[32 + delta++] = 0; + } + memcpy(sig + 32 + delta, der + offset, length); + + return 1; +} + +/** + * The nonce generated by internal library CX_RND_RFC6979 is not compatible + * with EOS. So this is the way to generate nonve for EOS. + * Arguments (deduced by relatko): + * - rnd - out + * - h1 - hash, in + * - x - private key, in + * - x_len - private key length + * - q - SECP256K1_N, in + * - q_len - 32, in + * - V, out + * - K, out + */ +static cx_err_t rng_rfc6979(unsigned char *rnd, + unsigned char *h1, + unsigned char *x, + unsigned int x_len, + const unsigned char *q, + unsigned int q_len, + unsigned char *V, + unsigned char *K) { + unsigned int h_len, found, i; + cx_hmac_sha256_t hmac; + + h_len = 32; + // a. h1 as input + + // loop for a candidate + found = 0; + while (!found) { + if (x) { + // b. Set: V = 0x01 0x01 0x01 ... 0x01 + memset(V, 0x01, h_len); + // c. Set: K = 0x00 0x00 0x00 ... 0x00 + memset(K, 0x00, h_len); + // d. Set: K = HMAC_K(V || 0x00 || int2octets(x) || bits2octets(h1)) + V[h_len] = 0; + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + // e. Set: V = HMAC_K(V) + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + // f. Set: K = HMAC_K(V || 0x01 || int2octets(x) || bits2octets(h1)) + V[h_len] = 1; + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + // g. Set: V = HMAC_K(V) -- + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + // initial setup only once + x = NULL; + } else { + // h.3 K = HMAC_K(V || 0x00) + V[h_len] = 0; + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); + // h.3 V = HMAC_K(V) + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + } + + // generate candidate + /* Shortcut: As only secp256k1/sha256 is supported, the step h.2 : + * While tlen < qlen, do the following: + * V = HMAC_K(V) + * T = T || V + * is replace by + * V = HMAC_K(V) + */ + x_len = q_len; + while (x_len) { + if (x_len < h_len) { + h_len = x_len; + } + FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + memcpy(rnd, V, h_len); + x_len -= h_len; + } + + // h.3 Check T is < n + for (i = 0; i < q_len; i++) { + if (V[i] < q[i]) { + found = 1; + break; + } + } + } + + return CX_OK; +} + // This function contains code producing signatures that is taken from EOS app -// The produced signature contains both -__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t* wittnessPath, +// To be sure that the functionality is unchanged, constants are left as they were +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t *wittnessPath, uint8_t hashBuf[SHA_256_SIZE], - uint8_t* signature, + uint8_t *signature, size_t signatureLen) { // Crypto assets to be cleared private_key_t privateKey; diff --git a/src/signTransaction.c b/src/signTransaction.c index 97f479a0..2f500d54 100644 --- a/src/signTransaction.c +++ b/src/signTransaction.c @@ -936,7 +936,6 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( } // We add hash to the response - TRACE("ecdsa_der_to_sig_result:"); TRACE_BUFFER(G_io_apdu_buffer, PUBKEY_LENGTH); memcpy(G_io_apdu_buffer + PUBKEY_LENGTH, hashBuf, SIZEOF(hashBuf)); From 0f87fffc02b72e2487e0fd24be472ae08fb10231 Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Sat, 7 Oct 2023 12:29:06 +0200 Subject: [PATCH 4/6] Review changes --- src/diffieHellman.c | 21 ++++++++++++--------- src/hash.h | 13 +++++-------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/diffieHellman.c b/src/diffieHellman.c index 5e4ea8ef..44c28537 100644 --- a/src/diffieHellman.c +++ b/src/diffieHellman.c @@ -97,20 +97,20 @@ static WARN_UNUSED_RESULT uint16_t processDHOneBlockFromCache(dh_context_t *ctx, __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_key_t *publicKey) { - TRACE_STACK_USAGE(); + // Crypto assets to be cleared private_key_t privateKey; - unsigned char basicSecret[32]; - unsigned char secret[SHA_512_SIZE]; - uint8_t K[SHA_512_SIZE]; - - TRACE("dh_init_aesKey"); - explicit_bzero(&privateKey, SIZEOF(privateKey)); + unsigned char basicSecret[32]; explicit_bzero(basicSecret, SIZEOF(basicSecret)); + unsigned char secret[SHA_512_SIZE]; explicit_bzero(secret, SIZEOF(secret)); + uint8_t K[SHA_512_SIZE]; explicit_bzero(K, SIZEOF(K)); explicit_bzero(dhKey, SIZEOF(*dhKey)); + TRACE_STACK_USAGE(); + TRACE("dh_init_aesKey"); + { uint16_t err = derivePrivateKey(pathSpec, &privateKey); if (err != SUCCESS) { @@ -157,6 +157,7 @@ dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_ STATIC_ASSERT(SIZEOF(dhKey->km) == DH_KM_SIZE, "Incompatible types"); memmove(dhKey->km, K + DH_AES_SECRET_SIZE, DH_KM_SIZE); + explicit_bzero(K, SIZEOF(K)); return SUCCESS; } @@ -280,6 +281,7 @@ dh_encode_append(dh_context_t *ctx, written += restLength; // processDHOneBlockFromCache returns number of bytes written } + explicit_bzero(&aesKey, SIZEOF(aesKey)); TRACE("Leaving dh_encode_append, written: %d", (int) written); *outSize = written; return SUCCESS; @@ -294,6 +296,7 @@ dh_encode_finalize(dh_context_t *ctx, // Crypto assets to be cleared dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); + // ctx->base64EncodingCache will contain final HMAC at one point // Crypto assets to be cleared in case of failure explicit_bzero(outBuffer, *outSize); // It suffices to clear this after final hmac @@ -404,9 +407,9 @@ dh_encode_finalize(dh_context_t *ctx, explicit_bzero(outBuffer, *outSize); return ERR_ASSERT; } + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); *outSize = written; return SUCCESS; - ; } #ifdef DEVEL @@ -493,7 +496,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pa dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); // Crypto assets to be cleared in case of failure - TRACE_BUFFER(buffer, *size); // buffer after decoding + TRACE_BUFFER(buffer, *size); // only after decoding if (*size < DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE) { return ERR_INVALID_DATA; diff --git a/src/hash.h b/src/hash.h index cfbe08eb..ffff32f0 100644 --- a/src/hash.h +++ b/src/hash.h @@ -33,7 +33,8 @@ static __attribute__((always_inline, unused)) cx_err_t sha_256_init(sha_256_cont static __attribute__((always_inline, unused)) cx_err_t sha_256_append(sha_256_context_t* ctx, const uint8_t* inBuffer, size_t inSize) { - if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC || + inSize >= BUFFER_SIZE_PARANOIA) { return CX_INVALID_PARAMETER; } TRACE_BUFFER(inBuffer, inSize); @@ -64,9 +65,6 @@ static __attribute__((always_inline, unused)) cx_err_t sha_256_hash(const uint8_ size_t inSize, uint8_t* outBuffer, size_t outSize) { - if (inSize >= BUFFER_SIZE_PARANOIA) { - return CX_INVALID_PARAMETER; - } sha_256_context_t ctx; cx_err_t err = sha_256_init(&ctx); if (err != CX_OK) { @@ -95,9 +93,11 @@ static __attribute__((always_inline, unused)) cx_err_t sha_512_init(sha_512_cont static __attribute__((always_inline, unused)) cx_err_t sha_512_append(sha_512_context_t* ctx, const uint8_t* inBuffer, size_t inSize) { - if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { + if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC || + inSize >= BUFFER_SIZE_PARANOIA) { return CX_INVALID_PARAMETER; } + TRACE_BUFFER(inBuffer, inSize); return cx_hash_no_throw(&ctx->cx_ctx.header, 0, /* Do not output the hash, yet */ @@ -126,9 +126,6 @@ static __attribute__((always_inline, unused)) cx_err_t sha_512_hash(const uint8_ size_t inSize, uint8_t* outBuffer, size_t outSize) { - if (inSize >= BUFFER_SIZE_PARANOIA) { - return CX_INVALID_PARAMETER; - } sha_512_context_t ctx; cx_err_t err = sha_512_init(&ctx); if (err != CX_OK) { From 7eb02b9a9d2527d9171be4c26e45c2b5c132d72c Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Tue, 10 Oct 2023 10:16:39 +0200 Subject: [PATCH 5/6] Consolidate crypto cleanup --- src/decodeDH.c | 3 + src/diffieHellman.c | 419 +++++++++++++++++------------------------- src/keyDerivation.c | 223 +++++++++------------- src/signTransaction.c | 2 + src/utils.h | 39 ++++ 5 files changed, 299 insertions(+), 387 deletions(-) diff --git a/src/decodeDH.c b/src/decodeDH.c index 9ceb3c0b..d78f2eee 100644 --- a/src/decodeDH.c +++ b/src/decodeDH.c @@ -505,6 +505,9 @@ void decode_handleAPDU(uint8_t p1, TRACE_BUFFER(ctx->buffer, ctx->bufferLen); uint16_t err = dh_decode(&ctx->pathSpec, &ctx->otherPubKey, ctx->buffer, &ctx->bufferLen); if (err != SUCCESS) { + explicit_bzero(ctx->buffer, ctx->bufferLen); + TRACE(); + PRINTF("Error: %d\n", err); THROW(err); } ctx->messageDecodedMagic = DECODING_FINISHED_MAGIC; diff --git a/src/diffieHellman.c b/src/diffieHellman.c index 44c28537..a7d9c6e7 100644 --- a/src/diffieHellman.c +++ b/src/diffieHellman.c @@ -1,6 +1,12 @@ #include "diffieHellman.h" #include "os_math.h" +// This is crypto module, we do not want to jump from it +// To avoid mistakes, we undef most common macros we do not want to use +#undef ASSERT +#undef THROW +#undef VALIDATE + //---------------------------- UTILS --------------------------------------- static const uint8_t BASE64[64] = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', @@ -22,6 +28,9 @@ static WARN_UNUSED_RESULT uint16_t base64EncWholeBlocks(uint8_t *inBuffer, uint8_t *inSize, uint8_t *outBuffer, uint16_t *outSize) { + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + uint8_t processedBlocks = 0; while (1) { // We cannot process whole block @@ -33,15 +42,15 @@ static WARN_UNUSED_RESULT uint16_t base64EncWholeBlocks(uint8_t *inBuffer, } // process one block - if (*outSize < BASE64_OUT_BLOCK_SIZE * (processedBlocks + 1)) { - return ERR_ASSERT; - }; + CRYPTO_ASSERT(*outSize >= BASE64_OUT_BLOCK_SIZE * (processedBlocks + 1)); base64EncBlock(inBuffer + BASE64_IN_BLOCK_SIZE * processedBlocks, outBuffer + BASE64_OUT_BLOCK_SIZE * processedBlocks); processedBlocks++; } *outSize = BASE64_OUT_BLOCK_SIZE * processedBlocks; - return SUCCESS; + error_to_return = SUCCESS; +end: + return error_to_return; } // Encodes one block from cache using AES key @@ -50,10 +59,11 @@ static WARN_UNUSED_RESULT uint16_t processDHOneBlockFromCache(dh_context_t *ctx, const dh_aes_key_t *aesKey, uint8_t *outBuffer, uint16_t *outSize) { + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + // precondition sanity check - if (ctx->cacheLength != CX_AES_BLOCK_SIZE) { - return ERR_ASSERT; - } + CRYPTO_ASSERT(ctx->cacheLength == CX_AES_BLOCK_SIZE); STATIC_ASSERT(SIZEOF(ctx->IV) == CX_AES_BLOCK_SIZE, "Incompatible IV size"); // We work in CBC mode @@ -63,34 +73,28 @@ static WARN_UNUSED_RESULT uint16_t processDHOneBlockFromCache(dh_context_t *ctx, } // 2. encrypt the result to obtain cyphertext 3. use cyphertext as new IV - cx_err_t err = cx_aes_enc_block(&aesKey->aesKey, ctx->cache, ctx->IV); - if (err != CX_OK) { - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_aes_enc_block(&aesKey->aesKey, ctx->cache, ctx->IV)); // append cyphertext (not base64 encrypted) to hmac and clear cache - err = cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, ctx->IV, CX_AES_BLOCK_SIZE); - if (err != CX_OK) { - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, ctx->IV, CX_AES_BLOCK_SIZE)); explicit_bzero(ctx->cache, SIZEOF(ctx->cache)); ctx->cacheLength = 0; // base64 encode - // sanity check, we expect that cache does not contain whole block first - if (ctx->base64EncodingCacheLen >= BASE64_IN_BLOCK_SIZE) { - return ERR_ASSERT; - } + CRYPTO_ASSERT(ctx->base64EncodingCacheLen < BASE64_IN_BLOCK_SIZE); STATIC_ASSERT(SIZEOF(ctx->base64EncodingCache) >= BASE64_IN_BLOCK_SIZE + CX_AES_BLOCK_SIZE, "Cache too small"); memmove(ctx->base64EncodingCache + ctx->base64EncodingCacheLen, ctx->IV, CX_AES_BLOCK_SIZE); ctx->base64EncodingCacheLen += CX_AES_BLOCK_SIZE; // This also sets outSize as desired - return base64EncWholeBlocks(ctx->base64EncodingCache, - &ctx->base64EncodingCacheLen, - outBuffer, - outSize); + CRYPTO_FORWARD_ERROR(base64EncWholeBlocks(ctx->base64EncodingCache, + &ctx->base64EncodingCacheLen, + outBuffer, + outSize)); + error_to_return = SUCCESS; +end: // CRYPTO_ macros jump here in case of error + return error_to_return; } //---------------------------- DH ENCODING --------------------------------------- @@ -99,66 +103,48 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_key_t *publicKey) { // Crypto assets to be cleared private_key_t privateKey; - explicit_bzero(&privateKey, SIZEOF(privateKey)); unsigned char basicSecret[32]; - explicit_bzero(basicSecret, SIZEOF(basicSecret)); unsigned char secret[SHA_512_SIZE]; - explicit_bzero(secret, SIZEOF(secret)); uint8_t K[SHA_512_SIZE]; + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(basicSecret, SIZEOF(basicSecret)); + explicit_bzero(secret, SIZEOF(secret)); explicit_bzero(K, SIZEOF(K)); explicit_bzero(dhKey, SIZEOF(*dhKey)); + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + TRACE_STACK_USAGE(); TRACE("dh_init_aesKey"); - { - uint16_t err = derivePrivateKey(pathSpec, &privateKey); - if (err != SUCCESS) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - return err; - } - } - - cx_err_t err = cx_ecdh_no_throw(&privateKey, - CX_ECDH_X, - publicKey->W, - publicKey->W_len, - basicSecret, - SIZEOF(basicSecret)); - explicit_bzero(&privateKey, SIZEOF(privateKey)); - if (err != CX_OK) { - explicit_bzero(basicSecret, SIZEOF(basicSecret)); - return ERR_ASSERT; - } + CRYPTO_FORWARD_ERROR(derivePrivateKey(pathSpec, &privateKey)); - err = sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret)); - explicit_bzero(basicSecret, SIZEOF(basicSecret)); - if (err != CX_OK) { - explicit_bzero(secret, SIZEOF(secret)); - return ERR_ASSERT; - } - - err = sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K)); - explicit_bzero(secret, SIZEOF(secret)); - if (err != CX_OK) { - explicit_bzero(K, SIZEOF(K)); - return ERR_ASSERT; - } + // Derive aes key. + CRYPTO_CX_CHECK(cx_ecdh_no_throw(&privateKey, + CX_ECDH_X, + publicKey->W, + publicKey->W_len, + basicSecret, + SIZEOF(basicSecret))); + CRYPTO_CX_CHECK(sha_512_hash(basicSecret, SIZEOF(basicSecret), secret, SIZEOF(secret))); + CRYPTO_CX_CHECK(sha_512_hash(secret, SIZEOF(secret), K, SIZEOF(K))); // First DH_AES_SECRET_SIZE bytes are used to compute shared secret, then DH_KM_SIZE are // used as Km for HMAC calculation STATIC_ASSERT(SIZEOF(K) == DH_AES_SECRET_SIZE + DH_KM_SIZE, "Incompatible types"); - err = cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey); - if (err != CX_OK) { - explicit_bzero(K, SIZEOF(K)); - explicit_bzero(dhKey, SIZEOF(*dhKey)); - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_aes_init_key_no_throw(K, DH_AES_SECRET_SIZE, &dhKey->aesKey)); STATIC_ASSERT(SIZEOF(dhKey->km) == DH_KM_SIZE, "Incompatible types"); memmove(dhKey->km, K + DH_AES_SECRET_SIZE, DH_KM_SIZE); + + error_to_return = SUCCESS; +end: // CRYPTO_ macros jump here in case of error + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(basicSecret, SIZEOF(basicSecret)); + explicit_bzero(secret, SIZEOF(secret)); explicit_bzero(K, SIZEOF(K)); - return SUCCESS; + return error_to_return; } __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode_init(dh_context_t *ctx, @@ -172,52 +158,47 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode_init(dh_context_ dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + TRACE_STACK_USAGE(); TRACE("dh_encode_init"); STATIC_ASSERT(SIZEOF(ctx->IV) == CX_AES_BLOCK_SIZE, "Unexpected IV length"); - if (ivSize != SIZEOF(ctx->IV)) { - return ERR_ASSERT; - } + CRYPTO_ASSERT(ivSize == SIZEOF(ctx->IV)); - // initialize DH context + // Initialize DH context ctx->cacheLength = 0; explicit_bzero(ctx->cache, SIZEOF(ctx->cache)); ctx->base64EncodingCacheLen = 0; explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); memcpy(ctx->IV, iv, SIZEOF(ctx->IV)); - // Compute AES key - { - uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } - } + // Compute AES key and forward error + CRYPTO_FORWARD_ERROR(dh_init_aes_key(&aesKey, pathSpec, publicKey)); explicit_bzero(&ctx->hmacCtx, SIZEOF(ctx->hmacCtx)); - cx_err_t err = cx_hmac_sha256_init_no_throw(&ctx->hmacCtx, aesKey.km, SIZEOF(aesKey.km)); - explicit_bzero(&aesKey, SIZEOF(aesKey)); - if (err != CX_OK) { - return ERR_ASSERT; - } - err = cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, iv, ivSize); - if (err != CX_OK) { - return ERR_ASSERT; - } - - ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&ctx->hmacCtx, aesKey.km, SIZEOF(aesKey.km))); + CRYPTO_CX_CHECK(cx_hmac_update((cx_hmac_t *) &ctx->hmacCtx, iv, ivSize)); // Base64 We encode IV STATIC_ASSERT(SIZEOF(ctx->base64EncodingCache) >= SIZEOF(ctx->IV), "Cache too small"); memcpy(ctx->base64EncodingCache, ctx->IV, SIZEOF(ctx->IV)); ctx->base64EncodingCacheLen = SIZEOF(ctx->IV); - // This also sets outSize as desired - return base64EncWholeBlocks(ctx->base64EncodingCache, - &ctx->base64EncodingCacheLen, - outBuffer, - outSize); + + ctx->initialized_magic = HASH_CONTEXT_INITIALIZED_MAGIC; + // Context initialization finished + + // This also sets outSize as desired. + CRYPTO_FORWARD_ERROR(base64EncWholeBlocks(ctx->base64EncodingCache, + &ctx->base64EncodingCacheLen, + outBuffer, + outSize)); + error_to_return = SUCCESS; +end: + // CX_CHECK macro jumps her in case of an error. Cleanup. + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return error_to_return; } __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t @@ -232,29 +213,20 @@ dh_encode_append(dh_context_t *ctx, dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + TRACE("dh_encode_append"); TRACE_STACK_USAGE(); // sanity checks - if (inSize >= BUFFER_SIZE_PARANOIA) { - return ERR_ASSERT; - } - if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { - return ERR_ASSERT; - } - if (ctx->cacheLength >= CX_AES_BLOCK_SIZE) { - return ERR_ASSERT; - } + CRYPTO_ASSERT(inSize < BUFFER_SIZE_PARANOIA); + CRYPTO_ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); + CRYPTO_ASSERT(ctx->cacheLength < CX_AES_BLOCK_SIZE); STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); - // Compute AES key - { - uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } - } + // Compute AES key and forward error + CRYPTO_FORWARD_ERROR(dh_init_aes_key(&aesKey, pathSpec, publicKey)); uint16_t processedInput = 0; uint16_t written = 0; @@ -273,18 +245,18 @@ dh_encode_append(dh_context_t *ctx, } uint16_t restLength = *outSize - written; // remaining buffer - uint16_t err = processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } + CRYPTO_FORWARD_ERROR( + processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength)); written += restLength; // processDHOneBlockFromCache returns number of bytes written } - - explicit_bzero(&aesKey, SIZEOF(aesKey)); TRACE("Leaving dh_encode_append, written: %d", (int) written); + + // Cleanup *outSize = written; - return SUCCESS; + error_to_return = SUCCESS; +end: + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return error_to_return; } __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t @@ -296,34 +268,25 @@ dh_encode_finalize(dh_context_t *ctx, // Crypto assets to be cleared dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); - // ctx->base64EncodingCache will contain final HMAC at one point - // Crypto assets to be cleared in case of failure - explicit_bzero(outBuffer, *outSize); // It suffices to clear this after final hmac + // ctx->base64EncodingCache + + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; TRACE("dh_encode_finalize"); TRACE_STACK_USAGE(); // sanity checks - if (ctx->initialized_magic != HASH_CONTEXT_INITIALIZED_MAGIC) { - return ERR_ASSERT; - } - if (ctx->cacheLength >= CX_AES_BLOCK_SIZE) { - return ERR_ASSERT; - } + CRYPTO_ASSERT(ctx->initialized_magic == HASH_CONTEXT_INITIALIZED_MAGIC); + CRYPTO_ASSERT(ctx->cacheLength < CX_AES_BLOCK_SIZE); STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); TRACE_BUFFER(ctx->cache, ctx->cacheLength); // Compute AES key - { - uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } - } + CRYPTO_FORWARD_ERROR(dh_init_aes_key(&aesKey, pathSpec, publicKey)); - // fill the last cache block with integers equal to number of elements missing - // if the next block is empty we create a block full of 0x10 (CX_AES_BLOCK_SIZE) + // Fill the last cache block with integers equal to number of elements missing. + // If the next block is empty we create a block full of 0x10 (CX_AES_BLOCK_SIZE) STATIC_ASSERT(SIZEOF(ctx->cache) >= CX_AES_BLOCK_SIZE, "dh_context_t->cache too small"); uint8_t fillValue = CX_AES_BLOCK_SIZE - ctx->cacheLength; for (uint16_t i = ctx->cacheLength; i < CX_AES_BLOCK_SIZE; i++) { @@ -336,40 +299,28 @@ dh_encode_finalize(dh_context_t *ctx, // Encode last block { uint16_t restLength = *outSize - written; // remaining buffer - uint16_t err = processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength); - explicit_bzero(&aesKey, SIZEOF(aesKey)); - if (err != SUCCESS) { - return err; - } + CRYPTO_FORWARD_ERROR( + processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength)); written += restLength; } // finalize hmac and append base64 encode it and append to cyphertext { size_t hmacOutSize = SIZEOF(ctx->base64EncodingCache) - ctx->base64EncodingCacheLen; - cx_err_t err = cx_hmac_final((cx_hmac_t *) &ctx->hmacCtx, - ctx->base64EncodingCache + ctx->base64EncodingCacheLen, - &hmacOutSize); - // cache now contains possibly sensitive data - if (err != CX_OK || hmacOutSize != DH_HMAC_SIZE) { - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_hmac_final((cx_hmac_t *) &ctx->hmacCtx, + ctx->base64EncodingCache + ctx->base64EncodingCacheLen, + &hmacOutSize)); + CRYPTO_ASSERT(hmacOutSize == DH_HMAC_SIZE); ctx->base64EncodingCacheLen += DH_HMAC_SIZE; } // Encode cache content { uint16_t restLength = *outSize - written; // remaining buffer - uint16_t err = base64EncWholeBlocks(ctx->base64EncodingCache, - &ctx->base64EncodingCacheLen, - outBuffer + written, - &restLength); - if (err != SUCCESS) { - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); - explicit_bzero(outBuffer, *outSize); - return err; - } + CRYPTO_FORWARD_ERROR(base64EncWholeBlocks(ctx->base64EncodingCache, + &ctx->base64EncodingCacheLen, + outBuffer + written, + &restLength)); written += restLength; // processDHOneBlockFromCache returns number of bytes written } @@ -379,11 +330,7 @@ dh_encode_finalize(dh_context_t *ctx, case 0: break; case 1: - if (*outSize < written + BASE64_OUT_BLOCK_SIZE) { - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); - explicit_bzero(outBuffer, *outSize); - return ERR_ASSERT; - } + CRYPTO_ASSERT(*outSize >= written + BASE64_OUT_BLOCK_SIZE); lastBlock[0] = ctx->base64EncodingCache[0]; base64EncBlock(lastBlock, outBuffer + written); *(outBuffer + written + 2) = '='; @@ -391,11 +338,7 @@ dh_encode_finalize(dh_context_t *ctx, written += BASE64_OUT_BLOCK_SIZE; break; case 2: - if (*outSize < written + BASE64_OUT_BLOCK_SIZE) { - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); - explicit_bzero(outBuffer, *outSize); - return ERR_ASSERT; - } + CRYPTO_ASSERT(*outSize >= written + BASE64_OUT_BLOCK_SIZE); lastBlock[0] = ctx->base64EncodingCache[0]; lastBlock[1] = ctx->base64EncodingCache[1]; base64EncBlock(lastBlock, outBuffer + written); @@ -403,13 +346,15 @@ dh_encode_finalize(dh_context_t *ctx, written += BASE64_OUT_BLOCK_SIZE; break; default: - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); - explicit_bzero(outBuffer, *outSize); - return ERR_ASSERT; + CRYPTO_ASSERT(false); } - explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + *outSize = written; - return SUCCESS; + error_to_return = SUCCESS; +end: + explicit_bzero(&aesKey, SIZEOF(aesKey)); + explicit_bzero(ctx->base64EncodingCache, SIZEOF(ctx->base64EncodingCache)); + return error_to_return; } #ifdef DEVEL @@ -421,71 +366,62 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_encode(bip44_path_t *pa uint16_t inSize, uint8_t *outBuffer, uint16_t *outSize) { + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + TRACE("dh_encode"); - ASSERT(inSize < BUFFER_SIZE_PARANOIA); + CRYPTO_ASSERT(inSize < BUFFER_SIZE_PARANOIA); dh_context_t ctx; uint16_t written = 0; uint16_t remaining = *outSize - written; - uint16_t err = - dh_encode_init(&ctx, pathSpec, publicKey, iv, ivSize, outBuffer + written, &remaining); - if (err != SUCCESS) { - return err; - } + CRYPTO_FORWARD_ERROR( + dh_encode_init(&ctx, pathSpec, publicKey, iv, ivSize, outBuffer + written, &remaining)); written += remaining; remaining = *outSize - written; - err = dh_encode_append(&ctx, - pathSpec, - publicKey, - inBuffer, - inSize, - outBuffer + written, - &remaining); - if (err != SUCCESS) { - return err; - } + CRYPTO_FORWARD_ERROR(dh_encode_append(&ctx, + pathSpec, + publicKey, + inBuffer, + inSize, + outBuffer + written, + &remaining)); written += remaining; remaining = *outSize - written; - err = dh_encode_finalize(&ctx, pathSpec, publicKey, outBuffer + written, &remaining); - if (err != SUCCESS) { - return err; - } + CRYPTO_FORWARD_ERROR( + dh_encode_finalize(&ctx, pathSpec, publicKey, outBuffer + written, &remaining)); written += remaining; *outSize = written; - return SUCCESS; + error_to_return = SUCCESS; +end: + return error_to_return; } #endif __noinline_due_to_stack__ static WARN_UNUSED_RESULT uint16_t validateHmac(dh_aes_key_t *aesKey, const uint8_t *buffer, uint16_t inSize) { - if (inSize < DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE) { - return ERR_INVALID_DATA; - } + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + + CRYPTO_VALIDATE(inSize >= DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE, ERR_INVALID_DATA); cx_hmac_sha256_t hmac; - cx_err_t err = cx_hmac_sha256_init_no_throw(&hmac, aesKey->km, SIZEOF(aesKey->km)); - if (err != CX_OK) { - return ERR_ASSERT; - } - err = cx_hmac_update((cx_hmac_t *) &hmac, buffer, inSize - DH_HMAC_SIZE); - if (err != CX_OK) { - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, aesKey->km, SIZEOF(aesKey->km))); + CRYPTO_CX_CHECK(cx_hmac_update((cx_hmac_t *) &hmac, buffer, inSize - DH_HMAC_SIZE)); uint8_t hmacBuf[DH_HMAC_SIZE]; size_t outLen = SIZEOF(hmacBuf); - err = cx_hmac_final((cx_hmac_t *) &hmac, hmacBuf, &outLen); - if (err != CX_OK || outLen != DH_HMAC_SIZE) { - return ERR_ASSERT; - } - if (memcmp(hmacBuf, buffer + inSize - DH_HMAC_SIZE, DH_HMAC_SIZE)) { - return ERR_INVALID_HMAC; - } - return SUCCESS; + CRYPTO_CX_CHECK(cx_hmac_final((cx_hmac_t *) &hmac, hmacBuf, &outLen)); + CRYPTO_ASSERT(outLen == DH_HMAC_SIZE); + CRYPTO_VALIDATE(!memcmp(hmacBuf, buffer + inSize - DH_HMAC_SIZE, DH_HMAC_SIZE), + ERR_INVALID_HMAC); + error_to_return = SUCCESS; +end: + return error_to_return; } __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pathSpec, @@ -495,37 +431,21 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pa // Crypto assets to be cleared dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); - // Crypto assets to be cleared in case of failure - TRACE_BUFFER(buffer, *size); // only after decoding - if (*size < DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE) { - return ERR_INVALID_DATA; - } - if (*size % CX_AES_BLOCK_SIZE != 0) { - return ERR_INVALID_DATA; - } + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + + CRYPTO_VALIDATE(*size >= DH_AES_IV_SIZE + CX_AES_BLOCK_SIZE + DH_HMAC_SIZE, ERR_INVALID_DATA); + CRYPTO_VALIDATE(*size % CX_AES_BLOCK_SIZE == 0, ERR_INVALID_DATA); // we do not decode IV, this also creates a buffer so we can decode inplace uint16_t read = DH_AES_IV_SIZE; uint16_t written = 0; - { - uint16_t err = dh_init_aes_key(&aesKey, pathSpec, publicKey); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } - } + CRYPTO_FORWARD_ERROR(dh_init_aes_key(&aesKey, pathSpec, publicKey)); // validate HMAC - { - uint16_t err = validateHmac(&aesKey, buffer, *size); - if (err != SUCCESS) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - return err; - } - - TRACE("HMAC validation succesfull."); - } + CRYPTO_FORWARD_ERROR(validateHmac(&aesKey, buffer, *size)); + TRACE("HMAC validation succesfull."); // initiate DH decryptions uint8_t IV[CX_AES_BLOCK_SIZE]; @@ -533,17 +453,8 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pa for (; read < *size - DH_HMAC_SIZE; read += CX_AES_BLOCK_SIZE) { // 1. Decode next block - if (read - written != CX_AES_BLOCK_SIZE) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - explicit_bzero(buffer, *size); - return ERR_ASSERT; - } - cx_err_t err = cx_aes_dec_block(&aesKey.aesKey, buffer + read, buffer + written); - if (err != CX_OK) { - explicit_bzero(&aesKey, SIZEOF(aesKey)); - explicit_bzero(buffer, *size); - return ERR_ASSERT; - } + CRYPTO_ASSERT(read - written == CX_AES_BLOCK_SIZE); + CRYPTO_CX_CHECK(cx_aes_dec_block(&aesKey.aesKey, buffer + read, buffer + written)); // 2. XOR with IV for (uint16_t i = 0; i < CX_AES_BLOCK_SIZE; i++) { buffer[written + i] ^= IV[i]; @@ -554,15 +465,13 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t dh_decode(bip44_path_t *pa written += CX_AES_BLOCK_SIZE; } - explicit_bzero(&aesKey, SIZEOF(aesKey)); - TRACE("Finishing decription, written:%d, lastCharacter:%d", written, buffer[written - 1]); // Calculate resulting length based on the last decoded value - if (written == 0 || written < buffer[written - 1]) { - explicit_bzero(buffer, *size); - return ERR_INVALID_DATA; - } + CRYPTO_VALIDATE(written != 0 && written >= buffer[written - 1], ERR_INVALID_DATA); *size = written - buffer[written - 1]; - return SUCCESS; + error_to_return = SUCCESS; +end: + explicit_bzero(&aesKey, SIZEOF(aesKey)); + return error_to_return; } diff --git a/src/keyDerivation.c b/src/keyDerivation.c index 977f845b..a2351ae4 100644 --- a/src/keyDerivation.c +++ b/src/keyDerivation.c @@ -10,58 +10,49 @@ #define PRIVATE_KEY_SEED_LEN 64 +// This is crypto module, we do not want to jump from it +// To avoid mistakes, we undef most common macros we do not want to use +#undef ASSERT +#undef THROW +#undef VALIDATE + // Taken from EOS app. Needed to produce signatures. static uint8_t const SECP256K1_N[] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe, 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41}; -#define FORWARD_CX_ERROR(call) \ - { \ - cx_err_t callResult = (call); \ - if (callResult != CX_OK) return callResult; \ - } - __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip44_path_t *pathSpec, private_key_t *privateKey) { // Crypto assets to be cleared uint8_t privateKeySeed[PRIVATE_KEY_SEED_LEN]; explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); - // Crypto assets to be cleared in case of failure explicit_bzero(privateKey, SIZEOF(*privateKey)); - if (policyDerivePrivateKey(pathSpec) == POLICY_DENY) { - return ERR_REJECTED_BY_POLICY; - } + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; - // Sanity check - if (pathSpec->length >= ARRAY_LEN(pathSpec->path)) { - return ERR_ASSERT; - } + CRYPTO_VALIDATE(policyDerivePrivateKey(pathSpec) != POLICY_DENY, ERR_REJECTED_BY_POLICY); + // Sanity check + CRYPTO_ASSERT(pathSpec->length < ARRAY_LEN(pathSpec->path)); STATIC_ASSERT(CX_APILEVEL >= 5, "unsupported api level"); TRACE(); - cx_err_t err = os_derive_bip32_with_seed_no_throw(HDW_NORMAL, - CX_CURVE_SECP256K1, - pathSpec->path, - pathSpec->length, - privateKeySeed, - NULL, - NULL, - 0); - if (err != CX_OK) { - explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); - return ERR_ASSERT; - } - - err = cx_ecfp_init_private_key_no_throw(CX_CURVE_SECP256K1, privateKeySeed, 32, privateKey); + CRYPTO_CX_CHECK(os_derive_bip32_with_seed_no_throw(HDW_NORMAL, + CX_CURVE_SECP256K1, + pathSpec->path, + pathSpec->length, + privateKeySeed, + NULL, + NULL, + 0)); + CRYPTO_CX_CHECK( + cx_ecfp_init_private_key_no_throw(CX_CURVE_SECP256K1, privateKeySeed, 32, privateKey)); + + error_to_return = SUCCESS; +end: explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); - if (err != CX_OK) { - explicit_bzero(privateKey, SIZEOF(*privateKey)); - return ERR_ASSERT; - } - - return SUCCESS; + return error_to_return; } __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip44_path_t *pathSpec, @@ -70,31 +61,20 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 private_key_t privateKey; explicit_bzero(&privateKey, SIZEOF(privateKey)); - { - uint16_t err = derivePrivateKey(pathSpec, &privateKey); - if (err != SUCCESS) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - return err; - } - } + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; - cx_err_t err = cx_ecfp_init_public_key_no_throw(CX_CURVE_SECP256K1, NULL, 0, publicKey); - if (err != CX_OK) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - return ERR_ASSERT; - } + CRYPTO_FORWARD_ERROR(derivePrivateKey(pathSpec, &privateKey)); - err = cx_ecfp_generate_pair_no_throw(CX_CURVE_SECP256K1, - publicKey, - &privateKey, - 1); // 1 - private key preserved - explicit_bzero(&privateKey, SIZEOF(privateKey)); - if (err != CX_OK) { - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_ecfp_init_public_key_no_throw(CX_CURVE_SECP256K1, NULL, 0, publicKey)); - TRACE(); - return SUCCESS; + CRYPTO_CX_CHECK(cx_ecfp_generate_pair_no_throw(CX_CURVE_SECP256K1, + publicKey, + &privateKey, + 1)); // 1 - private key preserved + error_to_return = SUCCESS; +end: + return error_to_return; } // EOS way to check if a signature is canonical :/ @@ -155,7 +135,7 @@ static int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { * - V, out * - K, out */ -static cx_err_t rng_rfc6979(unsigned char *rnd, +static uint16_t rng_rfc6979(unsigned char *rnd, unsigned char *h1, unsigned char *x, unsigned int x_len, @@ -163,6 +143,9 @@ static cx_err_t rng_rfc6979(unsigned char *rnd, unsigned int q_len, unsigned char *V, unsigned char *K) { + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; + unsigned int h_len, found, i; cx_hmac_sha256_t hmac; @@ -179,32 +162,32 @@ static cx_err_t rng_rfc6979(unsigned char *rnd, memset(K, 0x00, h_len); // d. Set: K = HMAC_K(V || 0x00 || int2octets(x) || bits2octets(h1)) V[h_len] = 0; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); // e. Set: V = HMAC_K(V) - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); // f. Set: K = HMAC_K(V || 0x01 || int2octets(x) || bits2octets(h1)) V[h_len] = 1; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, V, h_len + 1, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, 0, x, x_len, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, h1, h_len, K, 32)); // g. Set: V = HMAC_K(V) -- - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); // initial setup only once x = NULL; } else { // h.3 K = HMAC_K(V || 0x00) V[h_len] = 0; - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len + 1, K, 32)); // h.3 V = HMAC_K(V) - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); } // generate candidate @@ -220,8 +203,8 @@ static cx_err_t rng_rfc6979(unsigned char *rnd, if (x_len < h_len) { h_len = x_len; } - FORWARD_CX_ERROR(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); - FORWARD_CX_ERROR(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); + CRYPTO_CX_CHECK(cx_hmac_sha256_init_no_throw(&hmac, K, 32)); + CRYPTO_CX_CHECK(cx_hmac_no_throw((cx_hmac_t *) &hmac, CX_LAST, V, h_len, V, 32)); memcpy(rnd, V, h_len); x_len -= h_len; } @@ -235,7 +218,9 @@ static cx_err_t rng_rfc6979(unsigned char *rnd, } } - return CX_OK; + error_to_return = SUCCESS; +end: + return error_to_return; } // This function contains code producing signatures that is taken from EOS app @@ -246,78 +231,50 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path size_t signatureLen) { // Crypto assets to be cleared private_key_t privateKey; - explicit_bzero(&privateKey, SIZEOF(privateKey)); uint8_t V[33]; - explicit_bzero(V, SIZEOF(V)); uint8_t K[32]; + explicit_bzero(&privateKey, SIZEOF(privateKey)); + explicit_bzero(V, SIZEOF(V)); explicit_bzero(K, SIZEOF(K)); - // Crypto assets to be cleared in case of failure explicit_bzero(signature, signatureLen); - if (signatureLen < 200) { - return ERR_ASSERT; - } + // Variable for CRYPTO_ error handling macros + uint16_t error_to_return = ERR_ASSERT; - // We derive the private key - { - uint16_t err = derivePrivateKey(wittnessPath, &privateKey); - if (err != SUCCESS) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - return err; - } - TRACE("privateKey.d:"); - TRACE_BUFFER(privateKey.d, privateKey.d_len); - } + CRYPTO_ASSERT(signatureLen >= 200); + + CRYPTO_FORWARD_ERROR(derivePrivateKey(wittnessPath, &privateKey)); + TRACE("privateKey.d:"); + TRACE_BUFFER(privateKey.d, privateKey.d_len); // We sign the hash int tries = 0; - // Loop until a candidate matching the canonical signature is found for (;;) { if (tries == 0) { - cx_err_t err = rng_rfc6979(signature + 100, - hashBuf, - privateKey.d, - privateKey.d_len, - SECP256K1_N, - 32, - V, - K); - if (err != CX_OK) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - explicit_bzero(signature, signatureLen); - explicit_bzero(V, SIZEOF(V)); - explicit_bzero(K, SIZEOF(K)); - return ERR_ASSERT; - } + CRYPTO_FORWARD_ERROR(rng_rfc6979(signature + 100, + hashBuf, + privateKey.d, + privateKey.d_len, + SECP256K1_N, + 32, + V, + K)); } else { - cx_err_t err = rng_rfc6979(signature + 100, hashBuf, NULL, 0, SECP256K1_N, 32, V, K); - if (err != CX_OK) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - explicit_bzero(signature, signatureLen); - explicit_bzero(V, SIZEOF(V)); - explicit_bzero(K, SIZEOF(K)); - return ERR_ASSERT; - } + CRYPTO_FORWARD_ERROR( + rng_rfc6979(signature + 100, hashBuf, NULL, 0, SECP256K1_N, 32, V, K)); } uint32_t infos; size_t sig_len_ = 100; - cx_err_t err = cx_ecdsa_sign_no_throw(&privateKey, - CX_NO_CANONICAL | CX_RND_PROVIDED | CX_LAST, - CX_SHA256, - hashBuf, - 32, - signature + 100, - &sig_len_, - &infos); - if (err != CX_OK) { - explicit_bzero(&privateKey, SIZEOF(privateKey)); - explicit_bzero(signature, signatureLen); - explicit_bzero(V, SIZEOF(V)); - explicit_bzero(K, SIZEOF(K)); - return ERR_ASSERT; - } + CRYPTO_CX_CHECK(cx_ecdsa_sign_no_throw(&privateKey, + CX_NO_CANONICAL | CX_RND_PROVIDED | CX_LAST, + CX_SHA256, + hashBuf, + 32, + signature + 100, + &sig_len_, + &infos)); TRACE_BUFFER(signature + 100, 100); if ((infos & CX_ECCINFO_PARITY_ODD) != 0) { @@ -336,8 +293,10 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path } } + error_to_return = SUCCESS; +end: explicit_bzero(&privateKey, sizeof(privateKey)); explicit_bzero(V, SIZEOF(V)); explicit_bzero(K, SIZEOF(K)); - return SUCCESS; + return error_to_return; } diff --git a/src/signTransaction.c b/src/signTransaction.c index 2f500d54..da680227 100644 --- a/src/signTransaction.c +++ b/src/signTransaction.c @@ -803,6 +803,7 @@ __noinline_due_to_stack__ void signTx_handleEndDHEncodingAPDU( G_io_apdu_buffer, &bufferLen); if (err != SUCCESS) { + explicit_bzero(G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); THROW(err); } ctx->responseLength = bufferLen; @@ -932,6 +933,7 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( uint16_t err = signTransaction(&ctx->wittnessPath, hashBuf, G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); if (err != SUCCESS) { + explicit_bzero(G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); THROW(err); } diff --git a/src/utils.h b/src/utils.h index 1124e405..576dfb16 100644 --- a/src/utils.h +++ b/src/utils.h @@ -119,4 +119,43 @@ extern unsigned int app_stack_canary; #define TRACE_STACK_USAGE() #endif // DEVEL +// In crypto functions we do not want to throw errors, just return them +// To make this easy we use these macros +// They expect that the function has +// uint16_t error_to_return variable +// end: that contains cleanup +#define CRYPTO_CX_CHECK(call) \ + { \ + if ((call) != CX_OK) { \ + TRACE(); \ + error_to_return = ERR_ASSERT; \ + goto end; \ + } \ + } +#define CRYPTO_FORWARD_ERROR(call) \ + { \ + uint16_t err = (call); \ + if (err != SUCCESS) { \ + TRACE(); \ + error_to_return = err; \ + goto end; \ + } \ + } +#define CRYPTO_ASSERT(condition) \ + { \ + if (!(condition)) { \ + TRACE(); \ + error_to_return = ERR_ASSERT; \ + goto end; \ + } \ + } +#define CRYPTO_VALIDATE(condition, error) \ + { \ + if (!(condition)) { \ + TRACE(); \ + error_to_return = error; \ + goto end; \ + } \ + } + #endif // H_FIO_APP_UTILS From 7d38be1d37841d43a2f734c5babaca0e5da34847 Mon Sep 17 00:00:00 2001 From: Robert Lukotka Date: Mon, 30 Oct 2023 10:18:38 +0100 Subject: [PATCH 6/6] Incorporate review findings --- src/diffieHellman.c | 29 +++++++++++-------- src/diffieHellmann_test.c | 8 ++---- src/keyDerivation.c | 18 ++++++++---- src/keyDerivation.h | 4 +-- src/signTransaction.c | 59 ++++++++++++++++++++++----------------- src/signTransaction.h | 2 +- src/utils.h | 2 +- 7 files changed, 70 insertions(+), 52 deletions(-) diff --git a/src/diffieHellman.c b/src/diffieHellman.c index a7d9c6e7..c6a98074 100644 --- a/src/diffieHellman.c +++ b/src/diffieHellman.c @@ -15,7 +15,7 @@ static const uint8_t BASE64[64] = {'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '+', '/'}; -static void base64EncBlock(uint8_t in[3], uint8_t out[4]) { +static void base64EncBlock(uint8_t in[BASE64_IN_BLOCK_SIZE], uint8_t out[BASE64_OUT_BLOCK_SIZE]) { out[0] = BASE64[(in[0] / 0x04) & 0x3F]; out[1] = BASE64[(in[0] * 0x10 + in[1] / 0x10) & 0x3F]; out[2] = BASE64[(in[1] * 0x04 + in[2] / 0x40) & 0x3F]; @@ -140,6 +140,9 @@ dh_init_aes_key(dh_aes_key_t *dhKey, const bip44_path_t *pathSpec, const public_ error_to_return = SUCCESS; end: // CRYPTO_ macros jump here in case of error + if (error_to_return != SUCCESS) { + explicit_bzero(dhKey, SIZEOF(*dhKey)); + } explicit_bzero(&privateKey, SIZEOF(privateKey)); explicit_bzero(basicSecret, SIZEOF(basicSecret)); explicit_bzero(secret, SIZEOF(secret)); @@ -232,6 +235,7 @@ dh_encode_append(dh_context_t *ctx, uint16_t written = 0; while (1) { // fill ctx->buffer + CRYPTO_ASSERT(inSize >= processedInput); uint16_t to_read = MIN(CX_AES_BLOCK_SIZE - ctx->cacheLength, inSize - processedInput); memcpy(ctx->cache + ctx->cacheLength, inBuffer + processedInput, to_read); ctx->cacheLength += to_read; @@ -244,10 +248,10 @@ dh_encode_append(dh_context_t *ctx, break; } - uint16_t restLength = *outSize - written; // remaining buffer + uint16_t remainingBufferLength = *outSize - written; // remaining buffer CRYPTO_FORWARD_ERROR( - processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength)); - written += restLength; // processDHOneBlockFromCache returns number of bytes written + processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &remainingBufferLength)); + written += remainingBufferLength; } TRACE("Leaving dh_encode_append, written: %d", (int) written); @@ -268,7 +272,7 @@ dh_encode_finalize(dh_context_t *ctx, // Crypto assets to be cleared dh_aes_key_t aesKey; explicit_bzero(&aesKey, SIZEOF(aesKey)); - // ctx->base64EncodingCache + // ctx->base64EncodingCache needs to be cleared as it will contain sensitive HMAC // Variable for CRYPTO_ error handling macros uint16_t error_to_return = ERR_ASSERT; @@ -298,10 +302,10 @@ dh_encode_finalize(dh_context_t *ctx, // Encode last block { - uint16_t restLength = *outSize - written; // remaining buffer + uint16_t remainingBufferLength = *outSize - written; // remaining buffer CRYPTO_FORWARD_ERROR( - processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &restLength)); - written += restLength; + processDHOneBlockFromCache(ctx, &aesKey, outBuffer + written, &remainingBufferLength)); + written += remainingBufferLength; } // finalize hmac and append base64 encode it and append to cyphertext @@ -316,12 +320,12 @@ dh_encode_finalize(dh_context_t *ctx, // Encode cache content { - uint16_t restLength = *outSize - written; // remaining buffer + uint16_t remainingBufferLength = *outSize - written; // remaining buffer CRYPTO_FORWARD_ERROR(base64EncWholeBlocks(ctx->base64EncodingCache, &ctx->base64EncodingCacheLen, outBuffer + written, - &restLength)); - written += restLength; // processDHOneBlockFromCache returns number of bytes written + &remainingBufferLength)); + written += remainingBufferLength; } // the last base64 encoding block @@ -331,6 +335,7 @@ dh_encode_finalize(dh_context_t *ctx, break; case 1: CRYPTO_ASSERT(*outSize >= written + BASE64_OUT_BLOCK_SIZE); + STATIC_ASSERT(BASE64_OUT_BLOCK_SIZE >= 3, "BASE64_OUT_BLOCK_SIZE too small"); lastBlock[0] = ctx->base64EncodingCache[0]; base64EncBlock(lastBlock, outBuffer + written); *(outBuffer + written + 2) = '='; @@ -339,6 +344,7 @@ dh_encode_finalize(dh_context_t *ctx, break; case 2: CRYPTO_ASSERT(*outSize >= written + BASE64_OUT_BLOCK_SIZE); + STATIC_ASSERT(BASE64_OUT_BLOCK_SIZE >= 3, "BASE64_OUT_BLOCK_SIZE too small"); lastBlock[0] = ctx->base64EncodingCache[0]; lastBlock[1] = ctx->base64EncodingCache[1]; base64EncBlock(lastBlock, outBuffer + written); @@ -421,6 +427,7 @@ __noinline_due_to_stack__ static WARN_UNUSED_RESULT uint16_t validateHmac(dh_aes ERR_INVALID_HMAC); error_to_return = SUCCESS; end: + explicit_bzero(hmacBuf, SIZEOF(hmacBuf)); return error_to_return; } diff --git a/src/diffieHellmann_test.c b/src/diffieHellmann_test.c index e26f9e72..fedf1df9 100644 --- a/src/diffieHellmann_test.c +++ b/src/diffieHellmann_test.c @@ -66,9 +66,7 @@ __noinline_due_to_stack__ static void run_dh_encode_tests() { msgLen, \ encMsg, \ &encMsgLength); \ - if (err != SUCCESS) { \ - THROW(err); \ - } \ + ASSERT(err == SUCCESS); \ TRACE_BUFFER(encMsg, encMsgLength); \ ASSERT(encMsgLength == strlen(expectedEncMsg)); \ EXPECT_EQ_BYTES(encMsg, expectedEncMsg, strlen(expectedEncMsg)); \ @@ -292,9 +290,7 @@ __noinline_due_to_stack__ static void run_dh_decode_tests() { size_t expectedMsgLen = \ decode_hex(expectedDecMsgHex, expectedDecMsg, SIZEOF(expectedDecMsg)); \ uint16_t err = dh_decode(&pathSpec, &publicKey, msg, &msgLen); \ - if (err != SUCCESS) { \ - THROW(err); \ - } \ + ASSERT(err == SUCCESS); \ TRACE("Decoded mesage %d, %d", msgLen, expectedMsgLen); \ ASSERT(msgLen == expectedMsgLen); \ EXPECT_EQ_BYTES(msg, expectedDecMsg, msgLen); \ diff --git a/src/keyDerivation.c b/src/keyDerivation.c index a2351ae4..54df89bf 100644 --- a/src/keyDerivation.c +++ b/src/keyDerivation.c @@ -51,6 +51,9 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePrivateKey(const bip error_to_return = SUCCESS; end: + if (error_to_return != SUCCESS) { + explicit_bzero(privateKey, SIZEOF(*privateKey)); + } explicit_bzero(privateKeySeed, SIZEOF(privateKeySeed)); return error_to_return; } @@ -74,6 +77,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 1)); // 1 - private key preserved error_to_return = SUCCESS; end: + explicit_bzero(&privateKey, SIZEOF(privateKey)); return error_to_return; } @@ -84,7 +88,7 @@ static unsigned char check_canonical(uint8_t *rs) { } static int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { - int length; + int length = -1; int offset = 2; int delta = 0; if (der[offset + 2] == 0) { @@ -124,7 +128,7 @@ static int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { /** * The nonce generated by internal library CX_RND_RFC6979 is not compatible - * with EOS. So this is the way to generate nonve for EOS. + * with EOS. So this is the way to generate nonce for EOS. * Arguments (deduced by relatko): * - rnd - out * - h1 - hash, in @@ -134,6 +138,7 @@ static int ecdsa_der_to_sig(const uint8_t *der, uint8_t *sig) { * - q_len - 32, in * - V, out * - K, out + * This code is taken from EOS app. */ static uint16_t rng_rfc6979(unsigned char *rnd, unsigned char *h1, @@ -154,7 +159,7 @@ static uint16_t rng_rfc6979(unsigned char *rnd, // loop for a candidate found = 0; - while (!found) { + while (found == 0) { if (x) { // b. Set: V = 0x01 0x01 0x01 ... 0x01 memset(V, 0x01, h_len); @@ -225,7 +230,7 @@ static uint16_t rng_rfc6979(unsigned char *rnd, // This function contains code producing signatures that is taken from EOS app // To be sure that the functionality is unchanged, constants are left as they were -__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t *wittnessPath, +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t *witnessPath, uint8_t hashBuf[SHA_256_SIZE], uint8_t *signature, size_t signatureLen) { @@ -243,7 +248,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path CRYPTO_ASSERT(signatureLen >= 200); - CRYPTO_FORWARD_ERROR(derivePrivateKey(wittnessPath, &privateKey)); + CRYPTO_FORWARD_ERROR(derivePrivateKey(witnessPath, &privateKey)); TRACE("privateKey.d:"); TRACE_BUFFER(privateKey.d, privateKey.d_len); @@ -295,6 +300,9 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path error_to_return = SUCCESS; end: + if (error_to_return != SUCCESS) { + explicit_bzero(signature, signatureLen); + } explicit_bzero(&privateKey, sizeof(privateKey)); explicit_bzero(V, SIZEOF(V)); explicit_bzero(K, SIZEOF(K)); diff --git a/src/keyDerivation.h b/src/keyDerivation.h index 7c083682..c9d11ea1 100644 --- a/src/keyDerivation.h +++ b/src/keyDerivation.h @@ -49,7 +49,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 /** * @brief Signs transaction hash. * - * @param[in] wittnessPath Derivation path. + * @param[in] witnessPath Derivation path. * * @param[in] hashBuf Hash to sign. * @@ -63,7 +63,7 @@ __noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t derivePublicKey(const bip4 * - ERR_REJECTED_BY_POLICY * - ERR_ASSERT for other unexpected errors */ -__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t* wittnessPath, +__noinline_due_to_stack__ WARN_UNUSED_RESULT uint16_t signTransaction(bip44_path_t* witnessPath, uint8_t hashBuf[SHA_256_SIZE], uint8_t* signature, size_t signatureLen); diff --git a/src/signTransaction.c b/src/signTransaction.c index da680227..40fee75a 100644 --- a/src/signTransaction.c +++ b/src/signTransaction.c @@ -36,23 +36,28 @@ enum { // Uses ctx->dataToAppendToTx, ctx->dataToAppendToTxLen to extend hash // If ctx->dhIsActive then, we extend hash with encrypted data and prepare resulting encrypted -// blocks to G_io_apdu_buffer, ctx->responseLength Variables (&ctx->wittnessPath, &ctx->otherPubkey, +// blocks to G_io_apdu_buffer, ctx->responseLength Variables (&ctx->witnessPath, &ctx->otherPubkey, // &ctx->dhContext) are needed for encryption static void processShaAndPosibleDHAndPrepareResponse() { if (ctx->dhIsActive) { uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); - uint16_t err = dh_encode_append(&ctx->dhContext, - &ctx->wittnessPath, - &ctx->otherPubkey, - ctx->dataToAppendToTx, - ctx->dataToAppendToTxLen, - G_io_apdu_buffer, - &bufferLen); - if (err != SUCCESS) { - THROW(err); + { + uint16_t err = dh_encode_append(&ctx->dhContext, + &ctx->witnessPath, + &ctx->otherPubkey, + ctx->dataToAppendToTx, + ctx->dataToAppendToTxLen, + G_io_apdu_buffer, + &bufferLen); + if (err != SUCCESS) { + THROW(err); + } } ctx->responseLength = bufferLen; - CX_THROW(sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength)); + { + cx_err_t err = sha_256_append(&ctx->hashContext, G_io_apdu_buffer, ctx->responseLength); + ASSERT(err == CX_OK); + } VALIDATE(ctx->countedSectionDifference + ctx->responseLength >= ctx->dataToAppendToTxLen, ERR_INVALID_STATE); ctx->countedSectionDifference = @@ -69,17 +74,17 @@ static void processShaAndPosibleDHAndPrepareResponse() { } } -// Takes &ctx->wittnessPath and modifies ctx->value to be null terminated scting to display the +// Takes &ctx->witnessPath and modifies ctx->value to be null terminated scting to display the // pubkey static void prepareOurPubkeyForDisplay() { - public_key_t wittnessPathPubkey; - explicit_bzero(&wittnessPathPubkey, SIZEOF(wittnessPathPubkey)); - uint16_t err = derivePublicKey(&ctx->wittnessPath, &wittnessPathPubkey); + public_key_t witnessPathPubkey; + explicit_bzero(&witnessPathPubkey, SIZEOF(witnessPathPubkey)); + uint16_t err = derivePublicKey(&ctx->witnessPath, &witnessPathPubkey); ASSERT(err == SUCCESS); - TRACE_BUFFER(wittnessPathPubkey.W, SIZEOF(wittnessPathPubkey.W)); + TRACE_BUFFER(witnessPathPubkey.W, SIZEOF(witnessPathPubkey.W)); - uint32_t outlen = public_key_to_wif(wittnessPathPubkey.W, - SIZEOF(wittnessPathPubkey.W), + uint32_t outlen = public_key_to_wif(witnessPathPubkey.W, + SIZEOF(witnessPathPubkey.W), ctx->value, SIZEOF(ctx->value)); ASSERT(outlen != 0); @@ -139,17 +144,17 @@ __noinline_due_to_stack__ void signTx_handleInitAPDU(uint8_t p2, }* varData = (void*) varDataBuffer; VALIDATE(varSize >= SIZEOF(varData->chainId), ERR_INVALID_DATA); - // Parsing: network, ctx->wittnessPath, ctx->dataToAppendToTx, + // Parsing: network, ctx->witnessPath, ctx->dataToAppendToTx, network_type_t network = NETWORK_UNKNOWN; { network = getNetworkByChainId(varData->chainId, SIZEOF(varData->chainId)); TRACE("Chain: %d", (int) network); VALIDATE(network == NETWORK_MAINNET || network == NETWORK_TESTNET, ERR_INVALID_DATA); - const size_t parsedSize = bip44_parseFromWire(&ctx->wittnessPath, + const size_t parsedSize = bip44_parseFromWire(&ctx->witnessPath, varData->derivationPath, varSize - SIZEOF(varData->chainId)); - BIP44_PRINTF(&ctx->wittnessPath); + BIP44_PRINTF(&ctx->witnessPath); PRINTF("\n"); VALIDATE(parsedSize == varSize - SIZEOF(varData->chainId), ERR_INVALID_DATA); @@ -193,7 +198,7 @@ __noinline_due_to_stack__ void signTx_handleInitAPDU(uint8_t p2, // Security policy security_policy_t policy = POLICY_DENY; { - policy = policyForSignTxInit(&ctx->wittnessPath); + policy = policyForSignTxInit(&ctx->witnessPath); TRACE("Policy: %d", (int) policy); ENSURE_NOT_DENIED(policy); // select UI step @@ -682,7 +687,7 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( SIZEOF(ctx->otherPubkey.W), ctx->value, SIZEOF(ctx->value)); - ASSERT(outlen != 0); + ASSERT(outlen > 0); ASSERT(outlen < SIZEOF(ctx->value)); ctx->value[outlen] = 0; } @@ -706,7 +711,7 @@ __noinline_due_to_stack__ void signTx_handleStartDHEncodingAPDU( uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); uint16_t err = dh_encode_init(&ctx->dhContext, - &ctx->wittnessPath, + &ctx->witnessPath, &ctx->otherPubkey, IV, SIZEOF(IV), @@ -798,7 +803,7 @@ __noinline_due_to_stack__ void signTx_handleEndDHEncodingAPDU( uint16_t bufferLen = SIZEOF(G_io_apdu_buffer); uint16_t err = dh_encode_finalize(&ctx->dhContext, - &ctx->wittnessPath, + &ctx->witnessPath, &ctx->otherPubkey, G_io_apdu_buffer, &bufferLen); @@ -931,7 +936,7 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( } uint16_t err = - signTransaction(&ctx->wittnessPath, hashBuf, G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); + signTransaction(&ctx->witnessPath, hashBuf, G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); if (err != SUCCESS) { explicit_bzero(G_io_apdu_buffer, SIZEOF(G_io_apdu_buffer)); THROW(err); @@ -939,6 +944,8 @@ __noinline_due_to_stack__ void signTx_handleFinishAPDU( // We add hash to the response TRACE_BUFFER(G_io_apdu_buffer, PUBKEY_LENGTH); + STATIC_ASSERT(SIZEOF(G_io_apdu_buffer) >= PUBKEY_LENGTH + SIZEOF(hashBuf), + "G_io_apdu_buffer too small"); memcpy(G_io_apdu_buffer + PUBKEY_LENGTH, hashBuf, SIZEOF(hashBuf)); signTx_handleFinish_ui_runStep(); diff --git a/src/signTransaction.h b/src/signTransaction.h index 8436a08d..40f046b6 100644 --- a/src/signTransaction.h +++ b/src/signTransaction.h @@ -32,7 +32,7 @@ typedef struct { typedef struct { uint16_t initWasCalledMagic; - bip44_path_t wittnessPath; + bip44_path_t witnessPath; sha_256_context_t hashContext; tx_integrity_t integrity; tx_counted_section_t countedSections; diff --git a/src/utils.h b/src/utils.h index 576dfb16..e81cd4f0 100644 --- a/src/utils.h +++ b/src/utils.h @@ -153,7 +153,7 @@ extern unsigned int app_stack_canary; { \ if (!(condition)) { \ TRACE(); \ - error_to_return = error; \ + error_to_return = (error); \ goto end; \ } \ }