From 05433ebc1991d8176c265ef592372c5930eaf432 Mon Sep 17 00:00:00 2001 From: Jan Mazak Date: Mon, 19 Feb 2024 10:42:23 +0100 Subject: [PATCH] fix: review --- CHANGELOG.md | 1 + src/bufView.h | 13 ++++ src/messageSigning.c | 4 ++ src/signMsg.c | 146 +++++++++++++++++++++++-------------------- src/signMsg.h | 4 +- src/signMsg_ui.c | 20 ++++-- src/signTxOutput.c | 13 +++- 7 files changed, 126 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a6c0af7..4ca4ef16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Message signing (CIP-8) ### Changed +- usage of chunks of maximum allowed size is now enforced (datums and reference scripts in outputs) - TODO updated list of native tokens recognized by the app with correct decimal places 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/messageSigning.c b/src/messageSigning.c index dab0e403..4c2d4fd6 100644 --- a/src/messageSigning.c +++ b/src/messageSigning.c @@ -23,6 +23,10 @@ void signRawMessageWithPath(bip44_path_t* pathSpec, #ifndef FUZZING { + TRACE("signing with path:"); + BIP44_PRINTF(pathSpec); + PRINTF("\n"); + cx_err_t error = crypto_eddsa_sign(pathSpec->path, pathSpec->length, messageBuffer, diff --git a/src/signMsg.c b/src/signMsg.c index 945739ca..433917c5 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 @@ -48,15 +32,15 @@ void signMsg_handleInitAPDU( TRACE("Msg length: %d", ctx->msgLength); ctx->remainingBytes = ctx->msgLength; - view_skipBytes(&view, bip44_parseFromWire(&ctx->witnessPath, VIEW_REMAINING_TO_TUPLE_BUF_SIZE(&view))); - TRACE("Witness path:"); - BIP44_PRINTF(&ctx->witnessPath); + view_skipBytes(&view, bip44_parseFromWire(&ctx->signingPath, VIEW_REMAINING_TO_TUPLE_BUF_SIZE(&view))); + TRACE("Signing path:"); + BIP44_PRINTF(&ctx->signingPath); 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); @@ -77,7 +61,7 @@ void signMsg_handleInitAPDU( // Check security policy security_policy_t policy = policyForSignMsg( - &ctx->witnessPath, + &ctx->signingPath, ctx->addressFieldType, &ctx->addressParams ); @@ -90,7 +74,7 @@ void signMsg_handleInitAPDU( // key is sent back at the end and possibly needed when displaying address field extendedPublicKey_t extPubKey; deriveExtendedPublicKey( - &ctx->witnessPath, + &ctx->signingPath, &extPubKey ); STATIC_ASSERT(SIZEOF(extPubKey.pubKey) == SIZEOF(ctx->witnessKey), "wrong witness key size"); @@ -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,27 @@ 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: { + STATIC_ASSERT(SIZEOF(ctx->addressField) >= ADDRESS_KEY_HASH_LENGTH, "wrong address field size"); + bip44_pathToKeyHash(&ctx->signingPath, ctx->addressField, ADDRESS_KEY_HASH_LENGTH); + ctx->addressFieldSize = ADDRESS_KEY_HASH_LENGTH; + break; + } + + default: + ASSERT(false); + } +} + static size_t _createProtectedHeader(uint8_t* protectedHeaderBuffer, size_t maxSize) { // protectedHeader = { @@ -202,6 +234,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 +242,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 +272,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,7 +337,11 @@ static void signMsg_handleConfirmAPDU(const uint8_t* wireDataBuffer MARK_UNUSED, const size_t sigStructureSize = written; TRACE_BUFFER(sigStructure, sigStructureSize); - signRawMessageWithPath(&ctx->witnessPath, sigStructure, sigStructureSize, ctx->signature, SIZEOF(ctx->signature)); + // we do not sign anything that could be a transaction hash + // Note: in the v7.1 implementation Sig_structure has more than 40 bytes + ASSERT(sigStructureSize != TX_HASH_LENGTH); + + signRawMessageWithPath(&ctx->signingPath, sigStructure, sigStructureSize, ctx->signature, SIZEOF(ctx->signature)); ctx->ui_step = HANDLE_CONFIRM_STEP_MSG_HASH; signMsg_handleConfirm_ui_runStep(); diff --git a/src/signMsg.h b/src/signMsg.h index 6a97244c..2da3d2ea 100644 --- a/src/signMsg.h +++ b/src/signMsg.h @@ -26,7 +26,7 @@ typedef enum { } sign_msg_stage_t; typedef struct { - bip44_path_t witnessPath; + bip44_path_t signingPath; cip8_address_field_type_t addressFieldType; addressParams_t addressParams; @@ -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..469ff4a6 100644 --- a/src/signMsg_ui.c +++ b/src/signMsg_ui.c @@ -43,10 +43,10 @@ void signMsg_handleInit_ui_runStep() } UI_STEP(HANDLE_INIT_WITNESS_PATH) { #ifdef HAVE_BAGL - ui_displayPathScreen("Signing path", &ctx->witnessPath, this_fn); + ui_displayPathScreen("Signing path", &ctx->signingPath, this_fn); #elif defined(HAVE_NBGL) char pathStr[BIP44_PATH_STRING_SIZE_MAX + 1] = {0}; - ui_getPathScreen(pathStr, SIZEOF(pathStr), &ctx->witnessPath); + ui_getPathScreen(pathStr, SIZEOF(pathStr), &ctx->signingPath); fill_and_display_if_required("Signing path", pathStr, this_fn, respond_with_user_reject); #endif // HAVE_BAGL } @@ -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 diff --git a/src/signTxOutput.c b/src/signTxOutput.c index a412c65d..579ec09b 100644 --- a/src/signTxOutput.c +++ b/src/signTxOutput.c @@ -743,6 +743,10 @@ static void handleDatumInline(read_view_t* view) VALIDATE(chunkSize > 0, ERR_INVALID_DATA); VALIDATE(chunkSize <= MAX_CHUNK_SIZE, ERR_INVALID_DATA); VALIDATE(chunkSize <= subctx->stateData.datumRemainingBytes, ERR_INVALID_DATA); + if (subctx->stateData.datumRemainingBytes >= MAX_CHUNK_SIZE) { + // forces to use chunks of maximum allowed size + VALIDATE(chunkSize == MAX_CHUNK_SIZE, ERR_INVALID_DATA); + } view_parseBuffer(subctx->stateData.datumChunk, view, chunkSize); VALIDATE(view_remainingSize(view) == 0, ERR_INVALID_DATA); @@ -871,7 +875,7 @@ static void handleRefScriptAPDU(const uint8_t* wireDataBuffer, size_t wireDataSi // parse data subctx->stateData.refScriptRemainingBytes = parse_u4be(&view); - TRACE("refScriptRemainingBytes = %u", subctx->stateData.datumRemainingBytes); + TRACE("refScriptRemainingBytes = %u", subctx->stateData.refScriptRemainingBytes); VALIDATE(subctx->stateData.refScriptRemainingBytes > 0, ERR_INVALID_DATA); size_t chunkSize = parse_u4be(&view); @@ -936,7 +940,10 @@ static void handleRefScriptChunkAPDU(const uint8_t* wireDataBuffer, size_t wireD TRACE("chunkSize = %u", chunkSize); VALIDATE(chunkSize > 0, ERR_INVALID_DATA); VALIDATE(chunkSize <= MAX_CHUNK_SIZE, ERR_INVALID_DATA); - + if (subctx->stateData.refScriptRemainingBytes >= MAX_CHUNK_SIZE) { + // forces to use chunks of maximum allowed size + VALIDATE(chunkSize == MAX_CHUNK_SIZE, ERR_INVALID_DATA); + } VALIDATE(chunkSize <= subctx->stateData.refScriptRemainingBytes, ERR_INVALID_DATA); subctx->stateData.refScriptRemainingBytes -= chunkSize; @@ -947,7 +954,7 @@ static void handleRefScriptChunkAPDU(const uint8_t* wireDataBuffer, size_t wireD } { // add to tx - TRACE("Adding inline datum chunk to tx hash"); + TRACE("Adding reference script chunk to tx hash"); txHashBuilder_addOutput_referenceScript_dataChunk( &BODY_CTX->txHashBuilder, subctx->stateData.scriptChunk, subctx->stateData.refScriptChunkSize