From 5ec8ac1880cd0f3c292a2c2aed37f6e5f42d4cce Mon Sep 17 00:00:00 2001 From: Jan Mazak Date: Mon, 19 Feb 2024 10:42:23 +0100 Subject: [PATCH] fix: review --- src/bufView.h | 13 +++++ src/signMsg.c | 129 +++++++++++++++++++++++++---------------------- src/signMsg.h | 2 + src/signMsg_ui.c | 16 +++++- 4 files changed, 97 insertions(+), 63 deletions(-) diff --git a/src/bufView.h b/src/bufView.h index 53c1d9ef..00735a41 100644 --- a/src/bufView.h +++ b/src/bufView.h @@ -142,5 +142,18 @@ static inline int64_t parse_int64be(read_view_t* view) return (int64_t) parse_u8be(view); }; +static inline bool parse_bool(read_view_t* view) +{ + uint8_t value = parse_u1be(view); + + switch (value) { + case 0: + return false; + case 1: + return true; + default: + THROW(ERR_INVALID_DATA); + } +} #endif // H_CARDANO_APP_BUF_VIEW diff --git a/src/signMsg.c b/src/signMsg.c index 945739ca..7932e0b7 100644 --- a/src/signMsg.c +++ b/src/signMsg.c @@ -19,22 +19,6 @@ static ins_sign_msg_context_t* ctx = &(instructionState.signMsgContext); - -static bool parseBool(read_view_t* view) -{ - uint8_t value = parse_u1be(view); - TRACE("bool value: %d", value); - - switch (value) { - case 0: - return false; - case 1: - return true; - default: - THROW(ERR_INVALID_DATA); - } -} - void signMsg_handleInitAPDU( const uint8_t* wireDataBuffer, size_t wireDataSize @@ -53,10 +37,10 @@ void signMsg_handleInitAPDU( BIP44_PRINTF(&ctx->witnessPath); PRINTF("\n"); - ctx->hashPayload = parseBool(&view); + ctx->hashPayload = parse_bool(&view); TRACE("Hash payload: %d", ctx->hashPayload); - ctx->isAscii = parseBool(&view); + ctx->isAscii = parse_bool(&view); TRACE("Is ascii: %d", ctx->isAscii); ctx->addressFieldType = parse_u1be(&view); @@ -113,31 +97,58 @@ static void signMsg_handleMsgChunkAPDU(const uint8_t* wireDataBuffer, size_t wir } } { + TRACE_BUFFER(wireDataBuffer, wireDataSize); + ctx->receivedChunks += 1; - TRACE_BUFFER(wireDataBuffer, wireDataSize); + { + // validates if the previous chunk has been of maximum allowed size; + const size_t previousSize = ctx->chunkSize; + if (ctx->receivedChunks == 2) { + if (ctx->isAscii) { + VALIDATE(previousSize == MAX_CIP8_MSG_FIRST_CHUNK_ASCII_SIZE, ERR_INVALID_DATA); + } else { + VALIDATE(previousSize == MAX_CIP8_MSG_FIRST_CHUNK_HEX_SIZE, ERR_INVALID_DATA); + } + } else if (ctx->receivedChunks > 2) { + VALIDATE(previousSize == MAX_CIP8_MSG_HIDDEN_CHUNK_SIZE, ERR_INVALID_DATA); + } + } read_view_t view = make_read_view(wireDataBuffer, wireDataBuffer + wireDataSize); const size_t chunkSize = parse_u4be(&view); TRACE("chunkSize = %u", chunkSize); - // we allow empty message, displayed in the ui at this stage; it comes with empty chunk - if (ctx->msgLength > 0) { - VALIDATE(chunkSize > 0, ERR_INVALID_DATA); - } + VALIDATE(chunkSize <= ctx->remainingBytes, ERR_INVALID_DATA); + // the current chunk should have maximum allowed size; + // there is no point in allowing unnecessarily small chunks + // and it is a security risk if the first chunk (possibly the only one displayed) + // is artificially small if (ctx->receivedChunks == 1) { + // the first chunk must be displayable + // the check below works for empty message (with special UI) too if (ctx->isAscii) { - VALIDATE(chunkSize <= MAX_CIP8_MSG_FIRST_CHUNK_ASCII_SIZE, ERR_INVALID_DATA); + VALIDATE( + chunkSize == MIN(ctx->msgLength, MAX_CIP8_MSG_FIRST_CHUNK_ASCII_SIZE), + ERR_INVALID_DATA + ); } else { - VALIDATE(chunkSize <= MAX_CIP8_MSG_FIRST_CHUNK_HEX_SIZE, ERR_INVALID_DATA); + VALIDATE( + chunkSize == MIN(ctx->msgLength, MAX_CIP8_MSG_FIRST_CHUNK_HEX_SIZE), + ERR_INVALID_DATA + ); } } else { - VALIDATE(chunkSize <= MAX_CIP8_MSG_HIDDEN_CHUNK_SIZE, ERR_INVALID_DATA); + // ctx->receivedChunks >= 2 + VALIDATE( + chunkSize == MIN(ctx->remainingBytes, MAX_CIP8_MSG_HIDDEN_CHUNK_SIZE), + ERR_INVALID_DATA + ); } - VALIDATE(chunkSize <= ctx->remainingBytes, ERR_INVALID_DATA); + ASSERT(chunkSize <= ctx->remainingBytes); ctx->remainingBytes -= chunkSize; ctx->chunkSize = chunkSize; @@ -175,6 +186,23 @@ static void signMsg_handleMsgChunkAPDU(const uint8_t* wireDataBuffer, size_t wir } } +static void _prepareAddressField() +{ + switch (ctx->addressFieldType) { + + case CIP8_ADDRESS_FIELD_ADDRESS: + ctx->addressFieldSize = deriveAddress(&ctx->addressParams, ctx->addressField, SIZEOF(ctx->addressField)); + break; + + case CIP8_ADDRESS_FIELD_KEYHASH: + bip44_pathToKeyHash(&ctx->witnessPath, ctx->addressField, SIZEOF(ctx->addressField)); + break; + + default: + ASSERT(false); + } +} + static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxSize) { // protectedHeader = { @@ -202,6 +230,7 @@ static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxS size_t len = cbor_writeToken(CBOR_TYPE_TEXT, 7, p, end - p); p += len; } + { const char* text = "address"; const size_t len = strlen(text); @@ -209,46 +238,21 @@ static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxS memmove(p, text, len); p += len; } - - switch (ctx->addressFieldType) { - case CIP8_ADDRESS_FIELD_ADDRESS: { - uint8_t addressBuffer[MAX_ADDRESS_SIZE]; - size_t addressLen = deriveAddress(&ctx->addressParams, addressBuffer, SIZEOF(addressBuffer)); - { - size_t len = cbor_writeToken(CBOR_TYPE_BYTES, addressLen, p, end - p); - p += len; - } - ASSERT(p + addressLen < end); - { - memmove(p, addressBuffer, addressLen); - p += addressLen; - } - break; - } - case CIP8_ADDRESS_FIELD_KEYHASH: { - uint8_t hashedKey[ADDRESS_KEY_HASH_LENGTH] = {0}; - bip44_pathToKeyHash(&ctx->witnessPath, hashedKey, SIZEOF(hashedKey)); - { - size_t len = cbor_writeToken(CBOR_TYPE_BYTES, SIZEOF(hashedKey), p, end - p); - p += len; - } - ASSERT(p + SIZEOF(hashedKey) < end); - { - memmove(p, hashedKey, SIZEOF(hashedKey)); - p += SIZEOF(hashedKey); - } - break; + _prepareAddressField(); + { + size_t len = cbor_writeToken(CBOR_TYPE_BYTES, ctx->addressFieldSize, p, end - p); + p += len; } - default: - ASSERT(false); + { + ASSERT(p + ctx->addressFieldSize < end); + memmove(p, ctx->addressField, ctx->addressFieldSize); + p += ctx->addressFieldSize; } const size_t protectedHeaderSize = p - protectedHeaderBuffer; ASSERT(protectedHeaderSize > 0); ASSERT(protectedHeaderSize < maxSize); - TRACE_BUFFER(protectedHeaderBuffer, protectedHeaderSize); // TODO remove later - return protectedHeaderSize; } CATCH(ERR_DATA_TOO_LARGE) { ASSERT(false); @@ -264,7 +268,7 @@ static void signMsg_handleConfirmAPDU(const uint8_t* wireDataBuffer MARK_UNUSED, VALIDATE(wireDataSize == 0, ERR_INVALID_DATA); // it seems Ledger can sign 400 B, more is not needed since non-hashed msg is capped at 200 B - uint8_t sigStructure[400]; + uint8_t sigStructure[400] = {0}; explicit_bzero(sigStructure, SIZEOF(sigStructure)); size_t written = 0; const size_t maxWritten = SIZEOF(sigStructure); @@ -329,6 +333,9 @@ static void signMsg_handleConfirmAPDU(const uint8_t* wireDataBuffer MARK_UNUSED, const size_t sigStructureSize = written; TRACE_BUFFER(sigStructure, sigStructureSize); + // we do not sign anything that could be a transaction hash + VALIDATE(sigStructureSize != TX_HASH_LENGTH, ERR_REJECTED_BY_POLICY); + signRawMessageWithPath(&ctx->witnessPath, sigStructure, sigStructureSize, ctx->signature, SIZEOF(ctx->signature)); ctx->ui_step = HANDLE_CONFIRM_STEP_MSG_HASH; diff --git a/src/signMsg.h b/src/signMsg.h index 6a97244c..3008ab64 100644 --- a/src/signMsg.h +++ b/src/signMsg.h @@ -44,6 +44,8 @@ typedef struct { uint8_t msgHash[CIP8_MSG_HASH_LENGTH]; uint8_t signature[ED25519_SIGNATURE_LENGTH]; uint8_t witnessKey[PUBLIC_KEY_SIZE]; + uint8_t addressField[MAX_ADDRESS_SIZE]; + size_t addressFieldSize; sign_msg_stage_t stage; int ui_step; diff --git a/src/signMsg_ui.c b/src/signMsg_ui.c index 52a75084..a66e2382 100644 --- a/src/signMsg_ui.c +++ b/src/signMsg_ui.c @@ -148,7 +148,7 @@ void _displayMsgIntro(ui_callback_fn_t* callback) { char l1[30] = {0}; if (ctx->isAscii) { - snprintf(l1, SIZEOF(l1), "Message (ascii)"); + snprintf(l1, SIZEOF(l1), "Message (ASCII)"); } else { snprintf(l1, SIZEOF(l1), "Message (hex)"); } @@ -174,7 +174,7 @@ void _displayMsgFull(ui_callback_fn_t* callback) { char l1[30]; if (ctx->isAscii) { - snprintf(l1, SIZEOF(l1), "Message (ascii)"); + snprintf(l1, SIZEOF(l1), "Message (ASCII)"); } else { snprintf(l1, SIZEOF(l1), "Message (hex)"); } @@ -304,7 +304,10 @@ void signMsg_handleConfirm_ui_runStep() struct { uint8_t signature[ED25519_SIGNATURE_LENGTH]; uint8_t witnessKey[PUBLIC_KEY_SIZE]; + uint32_t addressFieldSize; + uint8_t addressField[MAX_ADDRESS_SIZE]; } wireResponse = {0}; + STATIC_ASSERT(SIZEOF(wireResponse) <= 255, "too large msg signing wire response"); STATIC_ASSERT(SIZEOF(ctx->signature) == ED25519_SIGNATURE_LENGTH, "wrong signature buffer size"); memmove(wireResponse.signature, ctx->signature, ED25519_SIGNATURE_LENGTH); @@ -312,6 +315,15 @@ void signMsg_handleConfirm_ui_runStep() STATIC_ASSERT(SIZEOF(ctx->witnessKey) == PUBLIC_KEY_SIZE, "wrong key buffer size"); memmove(wireResponse.witnessKey, ctx->witnessKey, PUBLIC_KEY_SIZE); + #ifndef FUZZING + STATIC_ASSERT(sizeof(wireResponse.addressFieldSize) == 4, "wrong address field size type"); + STATIC_ASSERT(sizeof(ctx->addressFieldSize) == 4, "wrong address field size type"); + u4be_write((uint8_t*) &wireResponse.addressFieldSize, ctx->addressFieldSize); + #endif + + STATIC_ASSERT(SIZEOF(ctx->addressField) == SIZEOF(wireResponse.addressField), "wrong address field size"); + memmove(wireResponse.addressField, ctx->addressField, ctx->addressFieldSize); + io_send_buf(SUCCESS, (uint8_t*) &wireResponse, SIZEOF(wireResponse)); #ifdef HAVE_BAGL ui_displayBusy(); // displays dots, called only after I/O to avoid freezing