Fix secu review#1031
Open
cedelavergne-ledger wants to merge 29 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1031 +/- ##
===========================================
+ Coverage 59.83% 60.07% +0.23%
===========================================
Files 26 26
Lines 2612 2645 +33
Branches 328 340 +12
===========================================
+ Hits 1563 1589 +26
- Misses 986 1051 +65
+ Partials 63 5 -58
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
elf sizes
Stack consumption summary (clone_app_stack_consumption)
Stack consumption summary
|
cedelavergne-ledger
force-pushed
the
cev/fix_secu_review
branch
from
c7b1b5c to
594b9c6
Compare
verify_map_entry_struct() appended every validly-signed map entry to g_map_entry_list without bound. The list is cleared on reset_app_context() between transactions, so the growth isn't permanent, but a host with N legitimately-signed descriptors can call INS_PROVIDE_MAP_ENTRY repeatedly during a single signing flow and exhaust the shared app-memory pool, denying allocation to other features (trusted_name, enum_value, safe_account, gating, GCS). Cap the list at MAX_MAP_ENTRIES (32). 32 distinct (chain, contract, selector, id, key) tuples is well above realistic per-transaction clear-signing needs while keeping the worst-case footprint around ~3.5 KB. This only addresses the DoS component of the finding. Replay protection (binding the descriptor to a per-session challenge in its signed TLV) requires a backend payload change and is left out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add the missing break; after the error assignment so the success path is taken only when address formatting actually succeeds.
cedelavergne-ledger
force-pushed
the
cev/fix_secu_review
branch
3 times, most recently
from
8c42623 to
514d3ed
Compare
check_bytes_constraint() and format_bytes() in the generic TX parser
rendered byte values as "0x" + hex via snprintf("0x%.*h", ...) and
silenced GCC with a localized -Wformat suppression because the %h
length-prefixed hex specifier is a BOLOS extension the compiler does
not recognize. The pragma block silences any -Wformat issue inside
its scope, hiding genuine format/argument mismatches that may arise
from later edits in the same window.
Prefix the destination buffer with "0x" and call
bytes_to_lowercase_hex() to fill the rest, which preserves the
lowercase casing the original %h specifier produced (and that the
existing GCS snapshots match) without needing a diagnostic
suppression.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
format_bytes() in the generic TX parser refuses an oversize raw-bytes field at field-description time rather than rendering it truncated. That behavior protects the user from a hash/display mismatch on the review screen, but it was previously exercised only implicitly: the existing GCS tests use small payloads, so any regression that silently re-introduced truncation would not show up in CI. Add test_gcs_raw_bytes_oversize_rejected, which injects a 200-byte payload into the `signature` parameter of the existing PoAP `mintToken` ABI and exposes it through a PARAM_TYPE_RAW / TypeFamily.BYTES field. With "0x" + 400 hex digits + NUL = 403 chars needed against the 380-byte strings.tmp.tmp buffer, the device must return SWO_INCORRECT_DATA on the field-descriptor APDU. Asserts that status word via pytest.raises(ExceptionRAPDU). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
field_hash_prepare() read the 2-byte declared-size prefix of an EIP-712 dynamic field with `*(uint16_t *) &data[0]`. data points into the APDU buffer and is not guaranteed to be 2-byte aligned, which is undefined behavior on strict-alignment ARM targets and risks a bus fault depending on incoming chunk boundaries. Use the SDK's read_u16_be() helper, which does a byte-by-byte big- endian decode and works regardless of alignment, removing both the UB and the __builtin_bswap16() that was paired with it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…dler handle_extra_data_enum() returns a `const nbgl_contentValueExt_t *` but the failure path of its snprintf() guard returned `false`. C lets the implicit bool→pointer conversion compile (0 → NULL), so the behavior was accidentally correct, but the literal is misleading and masks the intent of the early-return. Return NULL explicitly to match the pointer return type. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ERC-1155 batch_transfer parser accumulates per-id values into context->value via add256() to compute the total quantity displayed on the review screen. add256 wraps on uint256 overflow without signalling, so a crafted calldata whose values sum past 2^256 would silently produce a truncated total - a hostile dApp could pair a benign-looking aggregate with adversarial per-id quantities. After each accumulation, detect overflow by checking gt256(&new_value, &context->value): when the running total is now smaller than the addend, the sum has wrapped. Set the plugin result to ERROR so the host cannot present a misleading review screen. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rlp_decode_length() decoded RLP_*_LEN_OF_BYTES_4 strings and lists with `*(buffer + 1) << 24` where buffer is a uint8_t pointer. The C integer promotions widen the byte to a signed `int`, and a shift by 24 of an `int` whose result sets the sign bit is undefined behavior under the standard. In practice this triggers whenever the most- significant byte of the encoded length is >= 0x80. Cast each byte to uint32_t before shifting (and switch from `+` to `|` so the byte composition matches the surrounding intent), removing the UB without changing the decoded value. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The signed calldata-info filter payload allows the spender to be provided dynamically (CALLDATA_FLAG_ADDR_FILTER), the same way the callee already supports. The accept-path switch on spender_flag handled CALLDATA_FLAG_ADDR_VERIFYING_CONTRACT and CALLDATA_FLAG_ADDR_NONE explicitly but fell into the default branch for CALLDATA_FLAG_ADDR_FILTER, leaving calldata_info->spender_state at its calloc-zero value (CALLDATA_INFO_PARAM_NONE). The downstream filter-ingestion check requires spender_state == PARAM_UNSET, so a matching filter would be rejected and the spender screen never populated. Add the missing case: set spender_state to PARAM_UNSET to mirror the callee_flag handling so a follow-up filter can populate the value. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…igned The gated-signing screen is meant to render every GATED_SIGNING_MAX_COUNT (10) signing operations, gated by a uint8_t counter in N_storage. Because 256 is not a multiple of 10, the natural uint8_t wrap shifts the cadence: a display would land on sign #251, then nothing until sign #257 (gap of 6 instead of 10), then the cycle is permanently out of phase relative to its starting point. Detect the wrap (`counter + 1 == 0` after `uint8_t` promotion) and re-anchor the stored counter to 1, which forces a display on the wrap-spanning signing operation and resumes the every-10th cadence from a known phase. The wrap-adjacent interval is 5 signs instead of 10, accepted as a one-time anomaly; every subsequent cycle is exactly 10. Note: the modulo expression itself (`counter % MAX == 1`) is equivalent to the original `(counter - 1) % MAX == 0` for every uint8_t value, because C integer promotions make `counter - 1` signed (`-1 % 10 == -1`, not 255). The previous form was readable and correct; the fix above addresses the genuine wrap-cadence drift the previous form did not cover. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
safe_descriptor_t::address and four gating_t fields (hash_selector, intro_msg, tiny_url, address) were declared `const` to document that they should not change after parsing, but the TLV handlers wrote into them through `(uint8_t *)` / `(char *)` casts that strip the qualifier. The underlying storage comes from APP_MEM_CALLOC() so this is not strict undefined behaviour, but it is a pattern several static analyzers flag and it weakens the type signal at every read site. Remove the `const` from the five fields and drop the corresponding casts at the parser call sites where they were stripping the qualifier. The two remaining `(uint8_t *)` casts on `char` fields stay since they still perform a char->uint8_t conversion required by the TLV helper signatures. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
type_hash() allocated a sorted dependency list via get_struct_dependencies() to enumerate the structs that need to be hashed alongside the root type. The list was only freed at the very end of the happy path: any early return after the allocation - including a partial population on get_struct_dependencies() failure - skipped the flist_clear() and leaked nodes into the shared app-memory pool for the rest of the session. Restructure with a single `end:` label that runs flist_clear() unconditionally. The two pre-allocation early returns (missing struct lookup, keccak init failure) keep their direct returns since no list exists yet. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
get_appname_and_tagline() allocates g_tag_line on every home-screen transition where the caller is a plugin, but nothing freed the prior buffer first ? the comment at the allocation point even acknowledged "will never be deallocated". Each return to the home screen during a plugin-driven session (post-tx, post-cancel, multi-flow plugins) therefore leaked roughly the plugin name length plus the tagline template into the shared app-memory pool, slowly starving the rest of the session. Release the previous allocation with APP_MEM_FREE_AND_NULL() before the next APP_MEM_CALLOC() so the pool only ever holds one live tagline buffer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ETH2 deposit plugin reassembled the 48-byte BLS12-381 G1 compressed pubkey from two parameter chunks, then passed the raw buffer to getEthDisplayableAddress() to produce the screen string. That helper is hard-coded for 20-byte Ethereum addresses: its inner getEthAddressStringFromBinary() loops `for (i = 0; i < 20; i++)` and silently ignores the trailing 28 bytes of the pubkey. The validator "deposit address" shown to the user was therefore only the first 20 bytes of the 48-byte pubkey, hex-encoded, leaving the user unable to verify they were approving the intended validator. Keep the pubkey stored as raw 48 bytes and render it on screen as "0x" + bytes_to_lowercase_hex() inside the QUERY_CONTRACT_UI handler. Lowercase matches the casing the original getEthDisplayableAddress path produced for the truncated display. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ui_pairs_init() allocates the per-pair array with `nbPairs * sizeof(nbgl_contentTagValueList_t)` but g_pairs is typed as `nbgl_contentTagValue_t *`. The list struct is roughly three times the size of one tag-value entry, so each pair was being over-allocated by the size difference, wasting app-memory pool space on every generic-clear-signing review without any functional benefit. Use `sizeof(nbgl_contentTagValue_t)` instead so the allocation matches what the surrounding code reads and writes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cmd_get_challenge.c uses `#ifdef HAVE_CHALLENGE_NO_CHECK` to (a) seed the rolling challenge with a hardcoded constant and (b) skip the verification of host-supplied challenges. The flag is meant for test runs, but nothing in the source surfaced its presence to a release build — a stray CHALLENGE_NO_CHECK=1 in a Makefile invocation would ship a binary that accepts arbitrary challenge values without any trace in the build log. Emit a `#warning` whenever the macro is defined, so the build log shows the bypass and CI can grep for it before promoting an artifact to release. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_param_constraint() assigned `data->value.size` (uint16_t from the TLV) into the constraint node's `size` field, which is uint8_t. Anything over 255 bytes silently truncated to `size mod 256`. The buffer itself is allocated with the full uint16 size and the payload is copied in full, so no OOB occurs, but the truncated `node->size` later drives `bytes_to_lowercase_hex()` in check_bytes_constraint(), which then formats only `size mod 256` bytes — the constraint comparison silently never matches and the visibility filter behaves as if no constraint was set. Reject any constraint payload past UINT8_MAX before assigning, so a malformed descriptor is surfaced as an error instead of becoming a silent visibility bypass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… sentinel The PLUGIN_TYPE_OLD_INTERNAL branch of eth_plugin_call() walked INTERNAL_ETH_PLUGINS with an open-ended `for (i = 0;; i++)` loop and broke out when `alias[0] == 0`. The array initializer never emits a zero-byte sentinel — its last entry is "-eip7251" — so the only way the break could fire was the loop reading past the end of the array into adjacent .rodata and happening to land on a zero byte. Any non-zero byte in that adjacent memory turns the loop into an out-of- bounds dereference and an attempt to call PIC() on garbage. Bound the loop by ARRAYLEN(INTERNAL_ETH_PLUGINS) so iteration stays inside the table regardless of how its initializer is extended. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The 11 EIP-712 filter command handlers (calldata value/callee/chain- id/selector/amount/spender, plus the various other filter kinds) called hash_filtering_path() purely for its side effect on the signature-verification hash and ignored its bool return. When the helper bails on path_get_nth_field()/ui_712_get_discarded_path() returning NULL, the hash context is left mid-update; the subsequent sig_verif_end() then verifies an incomplete hash against the host- supplied signature. The downstream signature check still catches the mismatch in practice, so this isn't a bypass, but it produces a generic "signature failed" error several lines later instead of identifying the actual problem. Wrap each call site in `if (!hash_filtering_path(...)) return false;` so the failure is surfaced at the point where the inconsistency appears. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…th_read handle_owner_deriv_path() assigned the host-supplied length byte straight into context->owner_deriv_path.length and forwarded it to bip32_path_read(), whose internal guard already rejects out_len > MAX_BIP32_PATH. The OOB write is therefore unreachable in practice, but the code reads as if the length were trusted and relies on an SDK invariant that lives in a separate translation unit. Validate the length against MAX_BIP32_PATH in the handler itself, rejecting both 0 and any value larger than the fixed-size path buffer, so the bound is enforced locally rather than implicitly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_set_external_plugin() trusted the host-supplied length byte end-to-end: a zero-length name passed every existing check, the SHA-256 hash was computed over an empty name prefix, and only the Ledger PKI signature verification stood between the request and a plugin registration with pluginName = "". The downstream alias matching uses strcmp() against entries like "-erc20", so an empty alias would never match any internal plugin and the registration would be inert — but the device would still have stored a useless plugin context and surfaced a stale name on subsequent screens. Reject pluginNameLength == 0 explicitly before any further work. This is defense in depth against a backend that signs a malformed payload, not a bypass of the PKI gate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_check_address() read params->address_parameters[0] without first checking that the pointer was non-NULL or that address_parameters_length covered at least the length byte. The function is reachable only from the Exchange app via os_lib_call(), so this isn't exploitable from a host APDU today, but the contract between the two libraries lives across a trust boundary and would silently OOB-read on any future caller that wires up the same interface less carefully. Reject params->address_parameters == NULL and address_parameters_length < 1 early so the function is safe regardless of who calls it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rlp_decode_length() dereferenced its `buffer` argument and then indexed up to four bytes past the prefix without any length parameter. Safety relied entirely on the caller running rlp_can_decode() first — a sibling helper that does check bufferLength. The single in-tree caller (eth_ustream.c) follows that discipline, but a future call site that skipped rlp_can_decode would silently OOB-read into adjacent memory. Add a bufferLength parameter to rlp_decode_length(), reject zero- length input up front, and reject inputs shorter than the encoded length prefix in the long-string / long-list branches. Update the existing caller to pass context->rlpBufferPos. The function now upholds its own invariant instead of relying on its companion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
format_field() and format_param_group() in the generic-clear-signing formatter are mutually recursive: a PARAM_TYPE_GROUP entry recurses through every sub-field, and a nested PARAM_TYPE_GROUP recurses again. The only existing bound is the per-APDU TLV buffer size, which still leaves room for a couple hundred nesting levels — enough to walk through several KB of device stack with a hostile descriptor on the Nano S+ where total stack is tight. Thread an explicit `depth` parameter through format_field() and format_param_group(): the top-level call from cmd_field.c starts at 0, and format_param_group() bumps it before recursing. When depth reaches MAX_PARAM_GROUP_DEPTH (8), the formatter refuses the descriptor. Eight nesting levels is well past any realistic GCS layout and keeps the worst-case stack usage of this code path bounded. The unit-test mock for format_field() is updated to match the new signature, and a regression test asserts that the cap short-circuits before any sub-field is visited. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…chema injection handle_eip712_filtering() under P2_FILT_ACTIVATE computes the schema hash that downstream filter signatures are issued against, but left the EIP-712 type system unlocked: the host could continue sending INS_EIP712_STRUCT_DEF APDUs after the activate, append fields or even entire structs to the schema, and then sign a typed message whose actual schema differs from the one the filter metadata was issued for. Compute the schema hash first, and only on success switch the UI into filtering-full mode and set struct_state to DEFINED so subsequent handle_eip712_struct_def() calls are refused by the existing gate at commands_712.c:94. The whole branch is inside the `!N_storage.verbose_eip712` check (verbose mode never computes the hash). All three state changes (hash, filtering mode, type-system lock) move together, so a transient compute_schema_hash() failure leaves the handle clean and the host can retry the activate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ui_712_format_datetime() decoded the timestamp with u64_from_BE(data, length), which reads at most 8 bytes from the start of the buffer. EIP-712 typically declares timestamps as uint256, so a host-supplied 32-byte value carries its trailing 8 bytes as the meaningful payload and zero-pads the first 24. The formatter therefore read 8 bytes of padding, displayed 1970-01-01, and let the user sign a hash computed over the actual timestamp. Detect that the buffer is wider than uint64_t, verify the leading bytes are zero, and advance the data pointer past the padding so the decoded timestamp matches what is being hashed. Non-zero leading bytes mean the value cannot fit a time_t — refuse the screen in that case rather than wrap to an arbitrary date. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…view The ERC-20 internal plugin only cleared `extra_data` at ETH_PLUGIN_INIT_CONTRACT, leaving `destinationAddress`, `amount`, `ticker`, `decimals`, `contract_name`, and `selectorIndex` from the previous signing flow alive in the per-plugin global context that backs dataContext.tokenContext.pluginContext. A host that started a new transfer with selector-compatible but malformed calldata (wrong ABI offsets, truncated payload) could then reach ETH_PLUGIN_FINALIZE with neither the destination nor the amount parameter actually observed, and the trusted review screen would display the stale values from the previous flow as if they were the current ones. Zero the entire context at INIT and add `destination_parsed` / `amount_parsed` flags that the parameter handlers flip when the two mandatory ABI offsets fire. FINALIZE refuses the screen if either flag is missing, so non-conforming calldata is reported as ERROR instead of rendering stale state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_map_entry() compared P1 to P1_FIRST_CHUNK to decide whether to reset the TLV state, but any other P1 value silently fell into the "following chunk" branch — including reserved/unknown values that should be rejected. P2 was explicitly discarded via `(void) p2;` and any non-zero P2 was accepted as well. Reject P1 values that are neither P1_FIRST_CHUNK nor P1_FOLLOWING_CHUNK, and reject any non-zero P2. Returns SWO_WRONG_P1_P2 in both cases, matching the convention used by the other multi-chunk handlers (privacy_operation, sign_tx, safe_account, eth2_withdrawal_index, network_icon). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cedelavergne-ledger
force-pushed
the
cev/fix_secu_review
branch
from
514d3ed to
e59e5c9
Compare
The plugin-SDK auto-bump workflow used ad-m/github-push-action pinned to the upstream master branch. That is a textbook CWE-829: a supply-chain compromise of the action's master would inject arbitrary shell into every Ethereum-plugin auto-PR run, with write access to every LedgerHQ/app-plugin-* repository through the CI bot token. Pinning to a SHA would reduce but not remove that risk. Instead, replace the action with the same `git remote set-url` + `git push` pattern already used by ledger-app-workflows' _open_pr_with_new_snapshots.yml. The push step now runs only the shell visible in this file and uses the same CI_BOT_TOKEN as before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
3 tasks
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
2 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
https://ledgerhq.atlassian.net/browse/B2CA-2627
Changes include