fix APDU dispatch and metadata parsing edge cases

Author: abenso

app/src/addr.c

@@ -46,7 +46,7 @@ zxerr_t addr_getItem(int8_t displayIdx,
             snprintf(outKey, outKeyLen, "Address");
             if (scheme == secp256k1) {
                 // Size to comply with array_to_hexstr size check plus the 0x prefix
-                char tmp[SECP256K1_ADDRESS_LEN * 2 + 3] = {0};
+                char tmp[(SECP256K1_ADDRESS_LEN * 2) + 3] = {0};
                 tmp[0] = '0';
                 tmp[1] = 'x';
                 array_to_hexstr(tmp + 2, sizeof(tmp) - 2, G_io_apdu_buffer + SECP256K1_PK_LEN, SECP256K1_ADDRESS_LEN);
@@ -55,10 +55,12 @@ zxerr_t addr_getItem(int8_t displayIdx,
                 pageString(outVal, outValLen, (char *)(G_io_apdu_buffer + PK_LEN_25519), pageIdx, pageCount);
             }
             return zxerr_ok;
-        case 1:
+        case 1: {
             snprintf(outKey, outKeyLen, "Public key");
-            pageStringHex(outVal, outValLen, (char *)G_io_apdu_buffer, PK_LEN_25519, pageIdx, pageCount);
+            const uint16_t pkLen = (scheme == secp256k1) ? SECP256K1_PK_LEN : PK_LEN_25519;
+            pageStringHex(outVal, outValLen, (char *)G_io_apdu_buffer, pkLen, pageIdx, pageCount);
             return zxerr_ok;
+        }
         case 2: {
             if (!app_mode_expert()) {
                 return zxerr_no_data;

app/src/apdu_handler.c

@@ -38,6 +38,8 @@ static bool tx_initialized = false;
 uint16_t blobLen = 0;
 scheme_type_e scheme = ed25519;
 
+bool review_pending = false;
+
 static void extractHDPath(uint32_t rx, uint32_t offset) {
     tx_initialized = false;
 
@@ -75,11 +77,11 @@ __Z_INLINE bool process_chunk(__Z_UNUSED volatile uint32_t *tx, uint32_t rx) {
             extractHDPath(rx, OFFSET_DATA);
 
             // check if we have blobLen available
-            if ((rx - OFFSET_DATA) < sizeof(uint32_t) * HDPATH_LEN_DEFAULT + sizeof(uint16_t)) {
+            if ((rx - OFFSET_DATA) < (sizeof(uint32_t) * HDPATH_LEN_DEFAULT) + sizeof(uint16_t)) {
                 THROW(APDU_CODE_WRONG_LENGTH);
             }
             // read blobLen, right after hdPath
-            memcpy(&blobLen, G_io_apdu_buffer + OFFSET_DATA + sizeof(uint32_t) * HDPATH_LEN_DEFAULT, sizeof(uint16_t));
+            memcpy(&blobLen, G_io_apdu_buffer + OFFSET_DATA + (sizeof(uint32_t) * HDPATH_LEN_DEFAULT), sizeof(uint16_t));
             tx_initialized = true;
             return false;
         case P1_ADD:
@@ -143,15 +145,18 @@ __Z_INLINE void handleGetAddr(volatile uint32_t *flags, volatile uint32_t *tx, u
 
     // Get address type from P2
     scheme = G_io_apdu_buffer[OFFSET_P2];
+    if (scheme != ed25519 && scheme != secp256k1) {
+        THROW(APDU_CODE_INVALIDP1P2);
+    }
 
     if (scheme == ed25519) {
         // check if we have ss58prefix available
-        if ((rx - OFFSET_DATA) < sizeof(uint32_t) * HDPATH_LEN_DEFAULT + sizeof(uint16_t)) {
+        if ((rx - OFFSET_DATA) < (sizeof(uint32_t) * HDPATH_LEN_DEFAULT) + sizeof(uint16_t)) {
             THROW(APDU_CODE_WRONG_LENGTH);
         }
 
         // read ss58prefix, right after hdPath
-        memcpy(&ss58prefix, G_io_apdu_buffer + OFFSET_DATA + sizeof(uint32_t) * HDPATH_LEN_DEFAULT, sizeof(uint16_t));
+        memcpy(&ss58prefix, G_io_apdu_buffer + OFFSET_DATA + (sizeof(uint32_t) * HDPATH_LEN_DEFAULT), sizeof(uint16_t));
     } else {
         if ((rx - OFFSET_DATA) < sizeof(uint32_t) * HDPATH_LEN_DEFAULT) {
             THROW(APDU_CODE_WRONG_LENGTH);
@@ -169,6 +174,7 @@ __Z_INLINE void handleGetAddr(volatile uint32_t *flags, volatile uint32_t *tx, u
     }
     if (requireConfirmation) {
         view_review_init(addr_getItem, addr_getNumItems, app_reply_address);
+        set_review_pending(true);
         view_review_show(REVIEW_ADDRESS);
         *flags |= IO_ASYNCH_REPLY;
         return;
@@ -212,6 +218,7 @@ __Z_INLINE void handleSign(volatile uint32_t *flags, volatile uint32_t *tx, uint
     } else {
         view_review_init(tx_getItem, tx_getNumItems, app_sign_secp256k1);
     }
+    set_review_pending(true);
     view_review_show(REVIEW_TXN);
     *flags |= IO_ASYNCH_REPLY;
 }
@@ -240,6 +247,7 @@ __Z_INLINE void handleSignRaw(volatile uint32_t *flags, volatile uint32_t *tx, u
     } else {
         view_review_init(tx_raw_getItem, tx_raw_getNumItems, app_sign_secp256k1);
     }
+    set_review_pending(true);
     view_review_show(REVIEW_MSG);
     *flags |= IO_ASYNCH_REPLY;
 }
@@ -257,6 +265,10 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
                 THROW(APDU_CODE_WRONG_LENGTH);
             }
 
+            if (is_review_pending()) {
+                THROW(APDU_CODE_COMMAND_NOT_ALLOWED);
+            }
+
             switch (G_io_apdu_buffer[OFFSET_INS]) {
                 case INS_GET_VERSION: {
                     handle_getversion(flags, tx);
@@ -285,6 +297,7 @@ void handleApdu(volatile uint32_t *flags, volatile uint32_t *tx, uint32_t rx) {
             }
         }
         CATCH(EXCEPTION_IO_RESET) {
+            set_review_pending(false);
             THROW(EXCEPTION_IO_RESET);
         }
         // NOLINTNEXTLINE(readability-identifier-length): `e` is descriptive

app/src/common/actions.h

@@ -16,6 +16,7 @@
 #pragma once
 
 #include <os_io_seproxyhal.h>
+#include <stdbool.h>
 #include <stdint.h>
 
 #include "addr.h"
@@ -28,11 +29,23 @@
 extern uint16_t action_addrResponseLen;
 extern uint16_t blobLen;
 
+extern bool review_pending;
+
+__Z_INLINE void set_review_pending(bool val) {
+    review_pending = val;
+}
+
+__Z_INLINE bool is_review_pending(void) {
+    return review_pending;
+}
+
 __Z_INLINE void app_sign_ed25519() {
     const uint8_t *message = tx_get_buffer();
 
     zxerr_t err = crypto_sign_ed25519(G_io_apdu_buffer, IO_APDU_BUFFER_SIZE - 3, message, blobLen);
 
+    set_review_pending(false);
+
     if (err != zxerr_ok) {
         set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
         io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
@@ -47,6 +60,9 @@ __Z_INLINE void app_sign_secp256k1() {
 
     uint16_t sigLen = 0;
     zxerr_t err = crypto_sign_secp256k1(G_io_apdu_buffer, IO_APDU_BUFFER_SIZE - 3, message, blobLen, &sigLen);
+
+    set_review_pending(false);
+
     if (err != zxerr_ok) {
         set_code(G_io_apdu_buffer, 0, APDU_CODE_SIGN_VERIFY_ERROR);
         io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
@@ -57,6 +73,7 @@ __Z_INLINE void app_sign_secp256k1() {
 }
 
 __Z_INLINE void app_reject() {
+    set_review_pending(false);
     set_code(G_io_apdu_buffer, 0, APDU_CODE_COMMAND_NOT_ALLOWED);
     io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
 }
@@ -68,11 +85,13 @@ __Z_INLINE zxerr_t app_fill_address(const uint16_t ss58prefix, const scheme_type
 }
 
 __Z_INLINE void app_reply_error() {
+    set_review_pending(false);
     set_code(G_io_apdu_buffer, 0, APDU_CODE_DATA_INVALID);
     io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2);
 }
 
 __Z_INLINE void app_reply_address() {
+    set_review_pending(false);
     if (action_addrResponseLen == 0) {
         app_reply_error();
         return;

app/src/common/tx.c

@@ -84,6 +84,11 @@ const char *tx_raw_parse() {
         return parser_getErrorDescription(parser_no_data);
     }
 
+    // Check the actual buffered payload
+    if (dataLen != blobLen) {
+        return parser_getErrorDescription(parser_unexpected_value);
+    }
+
     // we need to have, at least, prefix and postfix
     if (dataLen < prefixLen + postfixLen) {
         return parser_getErrorDescription(parser_unexpected_value);

app/src/crypto.c

@@ -139,6 +139,7 @@ zxerr_t crypto_sign_ed25519(uint8_t *signature, uint16_t signatureMaxlen, const
 catch_cx_error:
     MEMZERO(&cx_privateKey, sizeof(cx_privateKey));
     MEMZERO(privateKeyData, SK_LEN_25519);
+    MEMZERO(messageDigest, BLAKE2B_DIGEST_SIZE);
 
     if (error != zxerr_ok) {
         MEMZERO(signature, signatureMaxlen);
@@ -155,12 +156,12 @@ zxerr_t crypto_sign_secp256k1(
 
     // Hash the message
     uint8_t messageDigest[CX_KECCAK_256_SIZE] = {0};
+    uint8_t intermediate_digest[BLAKE2B_DIGEST_SIZE] = {0};
     zxerr_t error = zxerr_unknown;
 
     // When the payload is bigger than MAX_SIGN_SIZE, it needs to be hashed with blake2b_256 before hashing with keccak_256.
     // https://github.com/paritytech/polkadot-sdk/blob/1a512570552119a49a8ecb2abfb7021954c4422d/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs#L567
     if (messageLen > MAX_SIGN_SIZE) {
-        uint8_t intermediate_digest[BLAKE2B_DIGEST_SIZE];
         // Hash it with blake2b
         cx_blake2b_t ctx;
         CATCH_CXERROR(cx_blake2b_init_no_throw(&ctx, 256));
@@ -201,6 +202,8 @@ zxerr_t crypto_sign_secp256k1(
 catch_cx_error:
     MEMZERO(&cx_privateKey, sizeof(cx_privateKey));
     MEMZERO(privateKeyData, sizeof(privateKeyData));
+    MEMZERO(messageDigest, sizeof(messageDigest));
+    MEMZERO(intermediate_digest, sizeof(intermediate_digest));
 
     if (error != zxerr_ok) {
         MEMZERO(output, outputLen);
@@ -251,24 +254,36 @@ static zxerr_t crypto_fillAddress_secp256k1(uint8_t *buffer,
     // Clear the buffer
     MEMZERO(buffer, bufferLen);
 
-    // Extract the uncompressed public key
     uint8_t uncompressedPubkey[SECP256K1_PK_LEN_UNCOMPRESSED] = {0};
-    CHECK_ZXERR(crypto_extractPublicKey_secp256k1(uncompressedPubkey, sizeof(uncompressedPubkey), hdPath_to_use))
+    uint8_t hashed1_pk[CX_KECCAK_256_SIZE] = {0};
+    char *const addr = (char *)(buffer + SECP256K1_PK_LEN);
+    zxerr_t err = zxerr_unknown;
+
+    // Extract the uncompressed public key
+    if (crypto_extractPublicKey_secp256k1(uncompressedPubkey, sizeof(uncompressedPubkey), hdPath_to_use) != zxerr_ok) {
+        goto fill_secp256k1_cleanup;
+    }
 
     // Compress the public key
-    CHECK_ZXERR(compressPubkey(uncompressedPubkey, sizeof(uncompressedPubkey), buffer, bufferLen))
-    char *addr = (char *)(buffer + SECP256K1_PK_LEN);
+    if (compressPubkey(uncompressedPubkey, sizeof(uncompressedPubkey), buffer, bufferLen) != zxerr_ok) {
+        goto fill_secp256k1_cleanup;
+    }
 
     // Encode the address - skip the first byte (0x04) that indicates the uncompressed format
-    uint8_t hashed1_pk[CX_KECCAK_256_SIZE] = {0};
-    CHECK_CX_OK(cx_keccak_256_hash(&uncompressedPubkey[1], sizeof(uncompressedPubkey) - 1, hashed1_pk));
+    if (cx_keccak_256_hash(&uncompressedPubkey[1], sizeof(uncompressedPubkey) - 1, hashed1_pk) != CX_OK) {
+        goto fill_secp256k1_cleanup;
+    }
 
     // Take the last 20 bytes of the Keccak hash as the address
     MEMCPY(addr, hashed1_pk + CX_KECCAK_256_SIZE - SECP256K1_ADDRESS_LEN, SECP256K1_ADDRESS_LEN);
 
     *addrResponseLen = SECP256K1_PK_LEN + SECP256K1_ADDRESS_LEN;
+    err = zxerr_ok;
 
-    return zxerr_ok;
+fill_secp256k1_cleanup:
+    MEMZERO(uncompressedPubkey, sizeof(uncompressedPubkey));
+    MEMZERO(hashed1_pk, sizeof(hashed1_pk));
+    return err;
 }
 
 // fill a crypto address using the global hdpath

app/src/metadata_parser.c

@@ -132,7 +132,7 @@ parser_error_t parseTypeRef(parser_context_t *blob,
     }
 
     if (type->index != Void && type->index != ById) {
-        printItem->itemCount++;
+        CHECK_ERROR(addItemCount(printItem, 1));
     }
 
     if (printItem->printing && printItem->target == printItem->itemCount) {
@@ -336,7 +336,7 @@ static parser_error_t parseEnumerationVariantType(parser_context_t *blob,
         }
     } else {
         if (!isOption && !isSome) {
-            printItem->itemCount++;
+            CHECK_ERROR(addItemCount(printItem, 1));
             if (printItem->printing && printItem->target == printItem->itemCount) {
                 printItem->item.val = tmpEntry->enumeration.name;
                 printItem->item.valEnc = EncString;
@@ -398,15 +398,15 @@ static parser_error_t parseSequenceType(parser_context_t *blob,
     CHECK_ERROR(readCompactU32(blob, &sequenceLen));
     TypeRef_t sequenceType = tmpEntry->sequence;
     if (sequenceLen == 0) {
-        printItem->itemCount++;
+        CHECK_ERROR(addItemCount(printItem, 1));
         if (printItem->printing && printItem->target == printItem->itemCount) {
             printItem->item.valEnc = EncEmptyVec;
         }
         return parser_ok;
     }
 
     if (sequenceType.index == U8) {
-        printItem->itemCount++;
+        CHECK_ERROR(addItemCount(printItem, 1));
         if (printItem->printing && printItem->target == printItem->itemCount) {
             printItem->item.valEnc = EncHexString;
             printItem->item.val.ptr = blob->buffer + blob->offset;
@@ -454,7 +454,7 @@ static parser_error_t parseArrayType(parser_context_t *blob,
     const uint32_t arrLen = tmpEntry->array.len;
     const TypeRef_t arrType = tmpEntry->array.typeParam;
     if (arrType.index == U8) {
-        printItem->itemCount++;
+        CHECK_ERROR(addItemCount(printItem, 1));
         if (printItem->printing && printItem->target == printItem->itemCount) {
             printItem->item.valEnc = EncHexString;
             printItem->item.val.ptr = blob->buffer + blob->offset;
@@ -523,13 +523,8 @@ static parser_error_t parseTupleType(parser_context_t *blob,
  * @param printItem Pointer to the print item to update.
  * @return parser_error_t Error code indicating the result of the operation.
  */
-parser_error_t parseMetadataEntry(
+static parser_error_t parseMetadataEntryInner(
     parser_context_t *blob, parser_context_t *metadata, uint32_t typeId, RegistryEntry_t *tmpEntry, PrintItem_t *printItem) {
-    if (metadata == NULL || blob == NULL || tmpEntry == NULL || printItem == NULL) {
-        return parser_no_data;
-    }
-
-    CHECK_ERROR(checkStack());
     MEMZERO(tmpEntry, sizeof(*tmpEntry));
 
     // Get the requested entry corresponding to typeId
@@ -553,11 +548,23 @@ parser_error_t parseMetadataEntry(
             break;
         case BitSequenceType:
             // TODO: implement BitSequence
-            return parser_value_out_of_range;
-
         default:
             return parser_value_out_of_range;
     }
 
-    return freeStack();
+    return parser_ok;
+}
+
+parser_error_t parseMetadataEntry(
+    parser_context_t *blob, parser_context_t *metadata, uint32_t typeId, RegistryEntry_t *tmpEntry, PrintItem_t *printItem) {
+    if (metadata == NULL || blob == NULL || tmpEntry == NULL || printItem == NULL) {
+        return parser_no_data;
+    }
+
+    // Acquire a recursion slot. On failure no slot was taken, so don't free.
+    CHECK_ERROR(checkStack());
+
+    const parser_error_t err = parseMetadataEntryInner(blob, metadata, typeId, tmpEntry, printItem);
+    (void)freeStack();
+    return err;
 }

app/src/metadata_proof.c

@@ -243,6 +243,13 @@ parser_error_t getMetadataDigest(Metadata_t *metadata, uint8_t metadataDigest[BL
     // get rootHash of registry
     CHECK_ERROR(getHash(ROOT_IDX, &proof));
 
+    // Require the proof to fully consume the supplied registry, indices and lemmas.
+    if (proof.registry.registry.offset != proof.registry.registry.bufferLen ||
+        proof.indices.indices.offset != proof.indices.indices.bufferLen ||
+        proof.lemmas.lemmas.offset != proof.lemmas.lemmas.bufferLen) {
+        return parser_unexpected_unparsed_bytes;
+    }
+
     // get hash of descriptor
     uint8_t extrinsicMetadataHash[BLAKE3_OUT_LEN] = {0};
     CHECK_ERROR(zxblake3_hash(metadata->shortMetadata.extrinsic.ctx.buffer, metadata->shortMetadata.extrinsic.ctx.bufferLen,