From 4edc10826548f013cbf1daabb15ff552da00ba73 Mon Sep 17 00:00:00 2001 From: Charles-Edouard de la Vergne Date: Wed, 1 Apr 2026 09:58:08 +0200 Subject: [PATCH 1/5] Fix CodeQL warnings --- .../cmd_provide_nft_info.c | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/features/provide_nft_information/cmd_provide_nft_info.c b/src/features/provide_nft_information/cmd_provide_nft_info.c index 44f7c58b2..fcc7f95c7 100644 --- a/src/features/provide_nft_information/cmd_provide_nft_info.c +++ b/src/features/provide_nft_information/cmd_provide_nft_info.c @@ -74,10 +74,14 @@ 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, @@ -85,22 +89,28 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer, 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,6 +120,7 @@ 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, @@ -117,6 +128,7 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer, 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,21 +153,30 @@ 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; @@ -159,6 +184,7 @@ uint16_t handle_provide_nft_information(const uint8_t *workBuffer, 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; From 240b6bfac4e3ce5ce94a1a4e585df3912d9c82b9 Mon Sep 17 00:00:00 2001 From: Charles-Edouard de la Vergne Date: Wed, 1 Apr 2026 11:49:38 +0200 Subject: [PATCH 2/5] Check OoB size before copy --- src/features/provide_network_info/cmd_network_info.c | 4 ++++ src/features/sign_message/cmd_sign_message.c | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/features/provide_network_info/cmd_network_info.c b/src/features/provide_network_info/cmd_network_info.c index dd4c11fc0..01ab6b6fe 100644 --- a/src/features/provide_network_info/cmd_network_info.c +++ b/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); diff --git a/src/features/sign_message/cmd_sign_message.c b/src/features/sign_message/cmd_sign_message.c index 3b37938dd..55c5d1550 100644 --- a/src/features/sign_message/cmd_sign_message.c +++ b/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; From 7c4ddd8c4a22ab185ba76d47dcaccbd79b7c0ed2 Mon Sep 17 00:00:00 2001 From: Charles-Edouard de la Vergne Date: Wed, 1 Apr 2026 12:01:23 +0200 Subject: [PATCH 3/5] Fix check return value --- src/features/sign_message_eip712/field_hash.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/features/sign_message_eip712/field_hash.c b/src/features/sign_message_eip712/field_hash.c index 97e333b6b..18a318ec8 100644 --- a/src/features/sign_message_eip712/field_hash.c +++ b/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) { From 180cb0730f7542efd221d86b1c68b0c66aa6e87b Mon Sep 17 00:00:00 2001 From: Charles-Edouard de la Vergne Date: Wed, 1 Apr 2026 13:30:17 +0200 Subject: [PATCH 4/5] Fix 'has_selector' dropped in proxy_info --- src/features/provide_proxy_info/proxy_info.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/features/provide_proxy_info/proxy_info.c b/src/features/provide_proxy_info/proxy_info.c index 456018fdb..8a490f6e5 100644 --- a/src/features/provide_proxy_info/proxy_info.c +++ b/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; } /** From d6d1be6f3a727ff03b21e194436c0383d42758bf Mon Sep 17 00:00:00 2001 From: Charles-Edouard de la Vergne Date: Wed, 1 Apr 2026 13:49:41 +0200 Subject: [PATCH 5/5] Fix 'use-after-free' or 'double-free' issues --- src/features/generic_tx_parser/tx_ctx.c | 27 ++++++++++++++----- .../provide_network_info/network_info.c | 9 ++++--- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/features/generic_tx_parser/tx_ctx.c b/src/features/generic_tx_parser/tx_ctx.c index b6a094f0c..5e616e139 100644 --- a/src/features/generic_tx_parser/tx_ctx.c +++ b/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(); } diff --git a/src/features/provide_network_info/network_info.c b/src/features/provide_network_info/network_info.c index e9b3978c6..7ac9db111 100644 --- a/src/features/provide_network_info/network_info.c +++ b/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();