Hotfix secu review - #1093
Merged
Merged
Conversation
INS 0x04 (sign TX), INS 0x08 (sign personal message) and INS 0x0C (sign EIP-712 v0) each called reset_app_context() or set_idle() when appState was not idle, silently tearing down an active review before returning an error. Replace those resets with an immediate SWO_COMMAND_NOT_ALLOWED: a new signing session is refused if one is already in progress. The active review is left untouched. Also move the g_msg_hash_ctx free from set_idle() into message_cleanup() so that rejecting a concurrent EIP-191 request does not release a hash context that belongs to the current signing session.
Code Coverage OverviewLanguages: C C / code-coverage/unittestsThe overall coverage in commit d4b6308 in the Show a code coverage summary of the most covered files.
Updated |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1093 +/- ##
===========================================
- Coverage 55.02% 42.12% -12.90%
===========================================
Files 16 10 -6
Lines 1592 1016 -576
Branches 201 193 -8
===========================================
- Hits 876 428 -448
+ Misses 712 534 -178
- Partials 4 54 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cedelavergne-ledger
force-pushed
the
cev/hotfix_secu_review
branch
2 times, most recently
from
August 11, 2026 08:27
050f373 to
041e1fa
Compare
cedelavergne-ledger
force-pushed
the
cev/hotfix_secu_review
branch
from
August 11, 2026 11:54
041e1fa to
7e5557c
Compare
bboilot-ledger
previously approved these changes
Aug 11, 2026
cedelavergne-ledger
force-pushed
the
cev/hotfix_secu_review
branch
from
August 12, 2026 09:08
7e5557c to
af3336c
Compare
cedelavergne-ledger
force-pushed
the
cev/hotfix_secu_review
branch
2 times, most recently
from
August 12, 2026 11:53
336c004 to
f957f4c
Compare
apaillier-ledger
force-pushed
the
cev/hotfix_secu_review
branch
from
August 13, 2026 09:40
bdafd67 to
adf25b1
Compare
- Move appState = APP_STATE_IDLE to the end of reset_app_context() instead of reporting idle while cleanup is still running - Main loop: dismiss an ongoing review before the reset frees its UI buffers - EIP-7702: on TLV failure, keep the displayed error screen and only reset the state; set APP_STATE_SIGNING_EIP7702 only on success - EIP-7702 ok callback: add the missing reset_app_context() after signing; let the status screen handle going idle - Blind signing rejection during TX parsing: keep the displayed error screen by resetting only the state
handle_sign() read P2 from the current APDU to decide whether to display the review UI on finalization. A host could open a signing stream with P2=SIGN_MODE_STORE (no UI) and close it with P2=SIGN_MODE_BASIC (show UI and hold the response), or vice versa, silently changing the flow's behavior on the last chunk. Add a sign_mode field to transactionContext_t. Capture the mode from the first APDU in handle_first_sign_chunk() and use the stored value at finalization instead of the incoming P2.
handle_sign() accepted SIGN_MODE_BASIC and SIGN_MODE_STORE in a shared fall-through case, so a P1_MORE chunk with a different P2 than the P1_FIRST chunk passed the outer switch silently. The pinned sign_mode (stored at P1_FIRST) was correctly used at the finalization check, so there was no functional impact, but the inconsistency went unreported. Add an explicit check in the P1_MORE path: if the incoming P2 does not match tmpCtx.transactionContext.sign_mode, reject with SWO_WRONG_P1_P2.
The four approval callbacks (TX, EIP-191, EIP-712, EIP-7702) signed and returned a signature without verifying that the application was still in the expected signing state. Add an appState check at the start of each callback. If the state does not match the expected value, reject with SWO_CONDITIONS_NOT_SATISFIED and reset the application context without signing.
appState was only set to APP_STATE_SIGNING_EIP712 inside ui_712_start(), which is called only when the UI is about to appear — either at the FILTERING MESSAGE_INFO step (FULL mode) or at the SIGN APDU (BASIC mode). Every preceding STRUCT_DEF / STRUCT_IMPL / FILTERING APDU ran without any appState guard, so: - A host could interleave a full EIP-712 v1 sequence into an active TX / EIP-191 / EIP-7702 review. The SIGN handler would reach ui_712_start() with appState != IDLE, call reset_app_context() (destroying the active review and zeroing tmpCtx immediately after td_hash_pass() had written the domain and message hashes), then present an approval screen over zeroed data. - Conversely, an EIP-712 v1 preparation session had no lock, so any other signing command could silently abort it. Fix with three changes: 1. Add APP_STATE_PREPARING_EIP712 to the app_state_t enum. This state covers the interval between the first STRUCT_DEF and the moment the UI is started. The existing guards on other entry points (INS 0x04, 0x08, 0x0C, EIP-7702) already check appState != IDLE, so they will block concurrent sessions automatically once this state is set. 2. In handle_eip712_v1_struct_def(), reject the first STRUCT_DEF (which triggers context allocation) with SWO_COMMAND_NOT_ALLOWED if appState != IDLE. Set appState = APP_STATE_PREPARING_EIP712 inside eip712_v1_context_init() on successful allocation so the lock is held for all subsequent preparatory APDUs. 3. In ui_712_start(), extend the no-reset condition to also accept APP_STATE_PREPARING_EIP712. This covers the normal transition from preparation to signing: the EIP-712 context and tmpCtx are preserved, and appState is promoted to APP_STATE_SIGNING_EIP712.
handle_get_public_key() called reset_app_context() unconditionally, silently tearing down any active signing review. During the P1_CONFIRM flow, appState stayed APP_STATE_IDLE, so the existing per-handler guards on signing entry points did not protect the pending address review either: a host could interleave an EIP-712 v1 STRUCT_DEF, which would call ui_712_init() and explicit_bzero the strings union while NBGL was still rendering strings.common.toAddress from it. Apply the same pattern used for signing flows: - Replace the unconditional reset_app_context() with an appState check: reject with SWO_COMMAND_NOT_ALLOWED when another flow is already active. In the P1_NON_CONFIRM path no review is shown, so no state change is needed. - Set appState = APP_STATE_VERIFYING_ADDRESS just before presenting the address review. All existing entry-point guards check appState != IDLE, so they block concurrent commands automatically for the duration of the review. - Add an appState guard to io_seproxyhal_touch_address_ok() consistent with the guards added to the four signing sinks in commit 2df93ff.
handle_get_eth2_public_key() shared io_seproxyhal_touch_address_ok() with the standard GET_PUBLIC_KEY flow but did not set APP_STATE_VERIFYING_ADDRESS before showing the review, causing the guard added in the previous commit to reject every ETH2 confirm. Apply the same two-change pattern: replace the unconditional reset_app_context() with an appState != IDLE check, and set APP_STATE_VERIFYING_ADDRESS before calling ui_display_public_eth2().
format_string() was calling str_cpy_explicit_trunc(), which silently truncated strings that did not fit the 380-byte display buffer and stopped copying at the first embedded NUL byte. In both cases the user would see a partial string on screen while the full byte sequence was incorporated into the instruction hash, letting a host hide content from the review. Fix format_string() to reject (return false) when: - value->length + 1 > buf_size (no room for NUL terminator) - the byte sequence contains an embedded NUL The field hash: the field was hashed into fields_hash_ctx before format_field() was called, so a rejected field was absent from the displayed review yet still contributed to the instruction hash. Reorder handle_tlv_payload() so cx_hash_no_throw() runs only after format_field() succeeds, ensuring that every byte in the instruction hash was actually shown to the user. Add unit tests (test_raw_string_oversize_rejected, test_raw_string_embedded_nul_rejected) and functional tests (test_gcs_raw_string_oversize_rejected, test_gcs_raw_string_embedded_nul_rejected) covering both rejection paths.
INS_PERFORM_PRIVACY_OPERATION (0x18): no appState check allowed it to arrive while a signing or address-review flow was in progress, where it would overwrite tmpCtx via the publicKeyContext union. Reject with SWO_COMMAND_NOT_ALLOWED unless appState == APP_STATE_IDLE. Then set APP_STATE_VERIFYING_ADDRESS before the review and guard the approval callback, completing the sink-guard coverage for this flow. INS_PROVIDE_GATING (0x38): no appState check meant the gating descriptor could be provisioned in any state, including while an unrelated review was shown or no signing was in progress. Restrict handle_gating() to APP_STATE_SIGNING_TX, APP_STATE_SIGNING_EIP712, and APP_STATE_PREPARING_EIP712 — the only states where gating is meaningful.
Reusing APP_STATE_VERIFYING_ADDRESS for the privacy-operation review created an ambiguity: io_seproxyhal_touch_address_ok() checks appState == APP_STATE_VERIFYING_ADDRESS and would have passed its guard if called during a privacy review, then called set_result_get_publicKey() on a tmpCtx that actually holds privacy-op material. Add a dedicated APP_STATE_PERFORMING_PRIVACY_OP state so the two review paths are independently discriminated. Update cmd_perform_privacy_operation.c to set it before showing the review and ui_common_perform_privacy_operation.c to check it in the approval callback.
When handle_field() is invoked in SIGNING_TX state but get_current_tx_info() returns NULL, it was calling gcs_cleanup() before returning SWO_COMMAND_NOT_ALLOWED. gcs_cleanup() frees g_pairs and g_pairsList. If a GCS review is currently shown on screen, NBGL still holds pointers into those buffers and dereferences them the next time it redraws the page, causing a SIGSEGV. The cleanup is not needed at this site: when no tx_info is registered, nothing has been allocated for this field session yet. If a review is active, the proper cleanup happens in the approve/reject callbacks and in reset_app_context(), which already calls ui_idle() before ui_all_cleanup() so NBGL always drops its references first.
Commands that arrive while a review is already active now return SWO_COMMAND_NOT_ALLOWED instead of resetting or calling ui_idle. The four affected test suites were still asserting the old behaviour: - test_cmd_sign_tx: rename test_first_resets_when_app_not_idle to test_first_rejected_when_app_not_idle; assert rejection + no reset. - test_cmd_sign_message: ui_idle is no longer called on rejection; both tests that asserted g_ui_idle_calls==1 corrected to 0. - test_cmd_get_public_key: handler no longer calls reset_app_context() unconditionally; test renamed and assertion flipped to g_reset_calls==0. - test_cmd_get_eth2_public_key: reset() fixture now sets appState=IDLE so BLS-pipeline tests that run after test_confirm_defers_reply_via_ui (which leaves appState=VERIFYING_ADDRESS) are not blocked by the new guard. Also drop the stale g_reset_calls==1 assertion.
Four issues found after the previous round of fixes:
- cmd_get_gating.c: PROVIDE_GATING (0x38) is sent before P1_FIRST in
the normal test flow, so appState is IDLE at that point. Widen the
allowlist from {SIGNING_TX, SIGNING_EIP712, PREPARING_EIP712} to also
include IDLE. All other states (address review, privacy op, message
signing, EIP7702) remain blocked.
- cmd_field.c: handle_tlv_payload did not call cleanup_field() on the
cx_hash_no_throw failure path introduced by a previous fix.
Added the missing cleanup to match the pattern used for the earlier
failure paths and prevent a resource leak.
- test_cmd_field_tx_info.c: test_field_no_tx_info_triggers_cleanup
asserted gcs_cleanup_calls==1, but previous fix deliberately
removed that call to avoid freeing NBGL-held buffers mid-review.
Corrected the assertion to ==0 and updated the comment.
- test_cmd_perform_privacy_operation.c: reset() fixture did not restore
appState to IDLE. test_public_encryption_key_confirm_calls_ui sets
appState=PERFORMING_PRIVACY_OP, causing the next test to hit the new
guard. Added appState=APP_STATE_IDLE to the fixture.
add_to_field_table() calls validate_instruction_hash() to mark the last field of each batch sub-transaction with end_intent=true, which is the flag that drives the NBGL page-break separator between sub-transactions. The previous fix moved cx_hash_no_throw() after format_field() so that rejected fields would not appear in the signed digest. This inadvertently broke the page break: validate_instruction_hash() inside add_to_field_table() would run before the hash was updated, would always return false, and end_intent would never be set. Correct approach: hash first (restoring the original call order), then format. If format_field() rejects the field (oversize string, embedded NUL, etc.), roll back the hash context to the snapshot taken before the hash call. The rejected field is then absent from both the display and the signed digest while the page break flag continues to work correctly. The snapshot is heap-allocated (APP_MEM_CALLOC) rather than stack or static: cx_sha3_t is ~220 bytes, and the call chain already places a second cx_sha3_t on the stack inside validate_inst_hash_on(). Using the heap avoids a stack overflow in deep call chains and keeps the allocation visible to the MEMORY_PROFILING instrumentation. The test stub for get_fields_hash_ctx() is updated from cx_hash_t to cx_sha3_t so that the memcpy of sizeof(cx_sha3_t) does not over-read the static buffer.
SET_EXTERNAL_PLUGIN (0x12) and SET_PLUGIN (0x16) are now rejected with SWO_COMMAND_NOT_ALLOWED when appState != IDLE: they overwrite the shared tokenContext fields while a signing review is on screen. PROVIDE_TX_SIMULATION (0x32) gets two guards: - P1=0x00 (data): rejected once G_transaction_check_info.received is set, preventing the host from replacing the displayed simulation result after the review has started (finding 195). reset_app_context() now calls clear_tx_simulation() so successive independent transactions each get their own simulation slot. - P1=0x01 (opt-in): rejected when appState != IDLE so the opt-in dialog cannot be drawn over an active signing review. Unit tests updated and new guard tests added.
When ETH_PLUGIN_QUERY_CONTRACT_UI fails, the function was calling reset_app_context() and io_send_sw() before returning false. The caller's error path (ux_init error: label) then sends a second APDU response, violating the one-response-per-command rule and risking a protocol desync. The error: label in ux_init() already calls io_seproxyhal_send_status() which handles the response and context reset; just return false.
The safe address and threshold strings were written into strings.tmp.tmp (a shared scratch union) and the pointers were passed to NBGL for an async review. Any subsequent APDU that writes to strings.tmp.tmp would silently overwrite the displayed content while the review was on screen. Allocate dedicated heap buffers (s_safe_addr_str, s_threshold_str) so the NBGL pointers remain stable for the lifetime of the review.
The handler dereferenced *workBuffer before checking dataLength, consuming uninitialized IO buffer memory when lc=0. Add an explicit dataLength < 1 guard so the pointer is never read without at least one byte of valid APDU data. Add a matching unit test for the zero-length case.
…ainnet gate - pluginContext not zeroed on INIT_CONTRACT: The 1 024-byte pluginContext scratch buffer is shared across plugin registrations. erc721, erc1155, and eth2 now call explicit_bzero on the cast context at the top of their INIT_CONTRACT handler so no previous plugin's state leaks into the new invocation. - ids_array_len underflow in erc1155 batch parsing: When safeBatchTransferFrom carries an empty ids array (len == 0) the previous code decremented ids_array_len via --ids_array_len before reaching 0, wrapping the uint16_t to 65 535 and corrupting subsequent parsing. next_param now goes directly to VALUE_LENGTH when ids_array_len == 0. - erc1155 screen count can overflow uint8_t: Added a compile-time _Static_assert to catch this at build time, and a runtime bounds check in set_batch_transfer_ui that returns ETH_PLUGIN_RESULT_ERROR if pair_idx >= ERC1155_BATCH_DISPLAY_MAX. - eth2 / eip7002 / eip7251 lack Ethereum mainnet gate: The ETH2 deposit contract, the EIP-7002 withdrawal predeploy, and the EIP-7251 consolidation predeploy are all Ethereum-mainnet-only. Their FINALIZE handlers now verify get_tx_chain_id() == ETHEREUM_MAINNET_CHAINID and return ETH_PLUGIN_RESULT_ERROR on any other chain. Unit tests added for every new security property.
Two issues in the same function: 1. path_struct was NULL-checked after being dereferenced at depth pointer computation. 2. depth_count - 1 underflows to 255 when depth_count == 0 (uint8_t), producing an out-of-bounds pointer before the depth_count > 0 guard. Fix: move NULL check first; move depth pointer computation inside the depth_count > 0 guard so it is only evaluated when safe.
apaillier-ledger
force-pushed
the
cev/hotfix_secu_review
branch
from
August 13, 2026 09:56
adf25b1 to
d4b6308
Compare
Contributor
Code coverage reportPer-file coverage
|
apaillier-ledger
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Security Review hotfix
Changes include