Skip to content

Commit

Permalink
Incorporate review findings
Browse files Browse the repository at this point in the history
  • Loading branch information
@relatko
relatko committed Oct 30, 2023
1 parent 7eb02b9 commit e0797fc
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 61 deletions.
2 changes: 1 addition & 1 deletion src/bip44.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ size_t bip44_printToStr(const bip44_path_t* pathSpec, char* out, size_t outSize)
{ \
ASSERT(ptr <= end); \
STATIC_ASSERT(sizeof(end - ptr) == sizeof(size_t), "bad size_t size"); \
size_t availableSize = (size_t) (end - ptr); \
size_t availableSize = (size_t)(end - ptr); \
/* Note(ppershing): We do not bother checking return */ \
/* value of snprintf as it always returns 0. */ \
/* Go figure out ... */ \
Expand Down
29 changes: 18 additions & 11 deletions src/diffieHellman.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
Expand All @@ -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);

Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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) = '=';
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down
8 changes: 2 additions & 6 deletions src/diffieHellmann_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)); \
Expand Down Expand Up @@ -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); \
Expand Down
2 changes: 1 addition & 1 deletion src/hexUtils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ uint8_t hex_parseNibble(const char c) {
uint8_t hex_parseNibblePair(const char* buffer) {
uint8_t first = hex_parseNibble(buffer[0]);
uint8_t second = hex_parseNibble(buffer[1]);
return (uint8_t) ((first << 4) + second);
return (uint8_t)((first << 4) + second);
}

size_t decode_hex(const char* inStr, uint8_t* outBuffer, size_t outMaxSize) {
Expand Down
18 changes: 13 additions & 5 deletions src/keyDerivation.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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);

Expand Down Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions src/keyDerivation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ static void fio_main(void) {
rx = tx;
tx = 0; // ensure no race in CATCH_OTHER if io_exchange throws an error
ASSERT((unsigned int) rx < sizeof(G_io_apdu_buffer));
rx = (unsigned int) io_exchange((uint8_t) (CHANNEL_APDU | flags), (uint16_t) rx);
rx = (unsigned int) io_exchange((uint8_t)(CHANNEL_APDU | flags), (uint16_t) rx);
flags = 0;

// We should be awaiting APDU
Expand Down
59 changes: 33 additions & 26 deletions src/signTransaction.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand All @@ -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),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -931,14 +936,16 @@ __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);
}

// 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();
Expand Down
Loading

0 comments on commit e0797fc

Please sign in to comment.