Security Audit for v1.22

src/features/generic_tx_parser/tx_ctx.c

@@ -165,23 +165,29 @@ static bool process_empty_tx(const s_tx_ctx *tx_ctx) {
 }
 
 bool process_empty_txs_before(void) {
-    for (list_node_t *tmp = ((list_node_t *) g_tx_ctx_current)->prev;
-         (tmp != NULL) && (((s_tx_ctx *) tmp)->calldata == NULL);
-         tmp = tmp->prev) {
+    list_node_t *tmp = ((list_node_t *) g_tx_ctx_current)->prev;
+    while ((tmp != NULL) && (((s_tx_ctx *) tmp)->calldata == NULL)) {
+        // process_empty_tx calls list_remove + delete_tx_ctx, which frees tmp.
+        // Ensure reading tmp->prev before the call to avoid use-after-free.
+        list_node_t *prev = tmp->prev;
         if (!process_empty_tx((s_tx_ctx *) tmp)) {
             return false;
         }
+        tmp = prev;
     }
     return true;
 }
 
 bool process_empty_txs_after(void) {
-    for (flist_node_t *tmp = ((flist_node_t *) g_tx_ctx_current)->next;
-         (tmp != NULL) && (((s_tx_ctx *) tmp)->calldata == NULL);
-         tmp = tmp->next) {
+    flist_node_t *tmp = ((flist_node_t *) g_tx_ctx_current)->next;
+    while ((tmp != NULL) && (((s_tx_ctx *) tmp)->calldata == NULL)) {
+        // process_empty_tx calls list_remove + delete_tx_ctx, which frees tmp.
+        // Ensure reading tmp->next before the call to avoid use-after-free.
+        flist_node_t *next = tmp->next;
         if (!process_empty_tx((s_tx_ctx *) tmp)) {
             return false;
         }
+        tmp = next;
     }
     return true;
 }
@@ -291,6 +297,15 @@ bool tx_ctx_init(s_calldata *calldata,
         return false;
     }
     list_push_back((list_node_t **) &g_tx_ctx_list, (list_node_t *) node);
+
+    // Ownership of the calldata has been transferred to the node.
+    // Clear g_parked_calldata now so callers cannot double-free it if we return
+    // false below (e.g. when field_table_init fails after the node is in the list
+    // and will be freed by tx_ctx_cleanup via delete_tx_ctx).
+    if (g_parked_calldata == calldata) {
+        g_parked_calldata = NULL;
+    }
+
     if ((appState == APP_STATE_SIGNING_TX) && (node == g_tx_ctx_list)) {
         return field_table_init();
     }

src/features/provide_network_info/cmd_network_info.c

@@ -28,6 +28,10 @@ static uint16_t handle_get_config(void) {
         network_info_t *net_info = (network_info_t *) node;
         if (net_info->chain_id != 0) {
             PRINTF("[NETWORK] - Found dynamic '%s'\n", net_info->name);
+            if (tx + sizeof(uint64_t) > sizeof(G_io_tx_buffer)) {
+                PRINTF("Error: Not enough space to return all networks!\n");
+                break;
+            }
             // Convert chain_id
             explicit_bzero(chain_str, sizeof(chain_str));
             write_u64_be(chain_str, 0, net_info->chain_id);

src/features/provide_network_info/network_info.c

@@ -417,11 +417,14 @@ void network_info_cleanup(network_info_t *network) {
         // Remove from list
         flist_remove((flist_node_t **) &g_dynamic_network_list, (flist_node_t *) network, NULL);
 
-        // Free the network info structure
-        APP_MEM_FREE_AND_NULL((void **) &network);
+        // Save address before freeing to check if it's the last added network
+        bool was_last = (g_last_added_network == network);
+
+        // Free the network info structure (local parameter — no need to null it)
+        APP_MEM_FREE((void *) network);
 
         // Reset last added network pointer if it was this network
-        if (g_last_added_network == network) {
+        if (was_last) {
             g_last_added_network = NULL;
             // Also cleanup temporary buffers associated with this network
             clear_icon();

src/features/provide_nft_information/cmd_provide_nft_info.c

@@ -74,33 +74,43 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
 
     PRINTF("In handle provide NFTInformation\n");
 
+    // Retrieve the NFT sub-structure from the current asset info slot.
     nft = &get_current_asset_info()->nft;
 
     PRINTF("Provisioning currentAssetIndex %d\n", tmpCtx.transactionContext.currentAssetIndex);
 
+    // --- Header validation ---
+    // The buffer must be large enough to hold at least the fixed-size header
+    // (type + version + collection name length).
     if (dataLength <= HEADER_SIZE) {
         PRINTF("Data too small for headers: expected at least %d, got %d\n",
                HEADER_SIZE,
                dataLength);
         return SWO_INCORRECT_DATA;
     }
 
+    // Reject unknown structure types so that future formats are not silently
+    // mis-parsed by this implementation.
     if (workBuffer[offset] != TYPE_1) {
         PRINTF("Unsupported type %d\n", workBuffer[offset]);
         return SWO_INCORRECT_DATA;
     }
     offset += TYPE_SIZE;
 
+    // Reject unknown versions for the same reason.
     if (workBuffer[offset] != VERSION_1) {
         PRINTF("Unsupported version %d\n", workBuffer[offset]);
         return SWO_INCORRECT_DATA;
     }
     offset += VERSION_SIZE;
 
+    // Read the collection name length declared in the header.
     collectionNameLength = workBuffer[offset];
     offset += NAME_LENGTH_SIZE;
 
-    // Size of the payload (everything except the signature)
+    // --- Payload size validation ---
+    // Size of the payload (everything except the signature).
+    // This is computed now that the variable-length collection name size is known.
     payloadSize = HEADER_SIZE + collectionNameLength + ADDRESS_LENGTH + CHAIN_ID_SIZE +
                   KEY_ID_SIZE + ALGORITHM_ID_SIZE;
     if (dataLength < payloadSize) {
@@ -110,13 +120,15 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
         return SWO_INCORRECT_DATA;
     }
 
+    // Guard against a collection name that would overflow the destination buffer.
     if (collectionNameLength > COLLECTION_NAME_MAX_LEN) {
         PRINTF("CollectionName too big: expected max %d, got %d\n",
                COLLECTION_NAME_MAX_LEN,
                collectionNameLength);
         return SWO_INCORRECT_DATA;
     }
 
+    // --- Collection name parsing ---
     // Safe because we've checked the size before.
     memcpy(nft->collectionName, workBuffer + offset, collectionNameLength);
     nft->collectionName[collectionNameLength] = '\0';
@@ -125,10 +137,14 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
     PRINTF("CollectionName: %s\n", nft->collectionName);
     offset += collectionNameLength;
 
+    // --- Contract address parsing ---
     memcpy(nft->contractAddress, workBuffer + offset, ADDRESS_LENGTH);
     PRINTF("Address: %.*H\n", ADDRESS_LENGTH, workBuffer + offset);
     offset += ADDRESS_LENGTH;
 
+    // --- Chain ID parsing and compatibility check ---
+    // The chain ID is encoded as a big-endian 64-bit integer.
+    // Reject it if the running app was built for a different chain.
     chain_id = u64_from_BE(workBuffer + offset, CHAIN_ID_SIZE);
     PRINTF("ChainID: %llu\n", chain_id);
     if (!app_compatible_with_chain_id(&chain_id)) {
@@ -137,28 +153,38 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
     }
     offset += CHAIN_ID_SIZE;
 
+    // --- Key ID validation ---
+    // Depending on the build configuration (staging vs. production), only one
+    // specific key identifier is accepted, ensuring the correct trust anchor is used.
     if (workBuffer[offset] != valid_keyId) {
         PRINTF("Unsupported KeyID %d\n", workBuffer[offset]);
         return SWO_INCORRECT_DATA;
     }
     offset += KEY_ID_SIZE;
 
+    // --- Algorithm ID validation ---
+    // Only algorithm ID 1 (ECDSA over secp256k1 with SHA-256) is supported.
     if (workBuffer[offset] != ALGORITHM_ID_1) {
         PRINTF("Incorrect algorithmId %d\n", workBuffer[offset]);
         return SWO_INCORRECT_DATA;
     }
     offset += ALGORITHM_ID_SIZE;
 
+    // --- Payload hashing ---
+    // Hash the structured payload (header through algorithm ID) so that the
+    // signature can be verified against a fixed-size digest.
     PRINTF("hashing: %.*H\n", payloadSize, workBuffer);
     cx_hash_sha256(workBuffer, payloadSize, hash, sizeof(hash));
 
+    // --- Signature length parsing and validation ---
     if (dataLength < payloadSize + SIGNATURE_LENGTH_SIZE) {
         PRINTF("Data too short to hold signature length\n");
         return SWO_INCORRECT_DATA;
     }
 
     signatureLen = workBuffer[offset];
     PRINTF("Signature len: %d\n", signatureLen);
+    // DER-encoded ECDSA signatures over a 256-bit curve fall within a known size range.
     if (signatureLen < MIN_DER_SIG_SIZE || signatureLen > MAX_DER_SIG_SIZE) {
         PRINTF("SignatureLen too big or too small. Must be between %d and %d, got %d\n",
                MIN_DER_SIG_SIZE,
@@ -168,11 +194,15 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
     }
     offset += SIGNATURE_LENGTH_SIZE;
 
+    // Verify that the declared signature bytes are actually present in the buffer.
     if (dataLength < payloadSize + SIGNATURE_LENGTH_SIZE + signatureLen) {
         PRINTF("Signature could not fit in data\n");
         return SWO_INCORRECT_DATA;
     }
 
+    // --- Signature verification ---
+    // Verify the DER signature against the Ledger NFT metadata public key.
+    // Reject the payload if the signature is invalid.
     if (check_signature_with_pubkey(hash,
                                     sizeof(hash),
                                     LEDGER_NFT_METADATA_PUBLIC_KEY,
@@ -183,6 +213,9 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer,
         return SWO_INCORRECT_DATA;
     }
 
+    // --- Commit the validated metadata ---
+    // Write the asset index into the response buffer, mark the asset info as
+    // validated, and advance the response length by one byte.
     G_io_tx_buffer[0] = tmpCtx.transactionContext.currentAssetIndex;
     validate_current_asset_info();
     *tx += 1;

src/features/provide_proxy_info/proxy_info.c

@@ -86,7 +86,12 @@ static bool handle_chain_id(const tlv_data_t *data, s_proxy_info_ctx *context) {
  * @return whether the handling was successful
  */
 static bool handle_selector(const tlv_data_t *data, s_proxy_info_ctx *context) {
-    return tlv_get_hash(data, context->proxy_info.selector, sizeof(context->proxy_info.selector));
+    if (!tlv_get_hash(data, context->proxy_info.selector, sizeof(context->proxy_info.selector))) {
+        PRINTF("SELECTOR: error\n");
+        return false;
+    }
+    context->proxy_info.has_selector = true;
+    return true;
 }
 
 /**

src/features/sign_message/cmd_sign_message.c

@@ -145,6 +145,17 @@ static uint16_t final_process(void) {
         goto end;
     }
 
+    // Guard against uint16_t overflow in the display buffer length calculation.
+    // In the worst case (hex path) the length becomes msg_length * 2 + 3, so
+    // reject messages that would cause that expression to exceed UINT16_MAX.
+    if (signMsgCtx->msg_length > ((UINT16_MAX - 3) / 2)) {
+        PRINTF("Error: message too long (%u > %u)\n",
+               signMsgCtx->msg_length,
+               ((UINT16_MAX - 3) / 2));
+        error = SWO_INCORRECT_DATA;
+        goto end;
+    }
+
     // Display buffer length
     buffer_length = signMsgCtx->msg_length;
 

src/features/sign_message_eip712/field_hash.c

@@ -264,6 +264,10 @@ bool field_hash(const uint8_t *data, uint8_t data_length, bool partial) {
         }
 
         data = field_hash_prepare(field_ptr, data, &data_length);
+        if (data == NULL) {
+            apdu_response_code = SWO_INCORRECT_DATA;
+            return false;
+        }
         total_length = fh->remaining_size;
     }
     if (data_length > fh->remaining_size) {