fix: review

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/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,53 +230,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 +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;

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;

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,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