fix: review

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
 
 

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

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,

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,53 +234,29 @@ 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);
 				ASSERT(p + len < end);
 				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();

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;

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,14 +304,26 @@ 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);
 
 		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

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