Fix cerberus Medium#1028
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1028 +/- ##
===========================================
+ Coverage 56.04% 59.83% +3.79%
===========================================
Files 26 26
Lines 2414 2612 +198
Branches 313 334 +21
===========================================
+ Hits 1353 1563 +210
- Misses 1005 1045 +40
+ Partials 56 4 -52
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_cerberus_medium
branch
2 times, most recently
from
75784b3 to
191e4a5
Compare
The packaged Python client's TxAuth7702 constructor accepted chain_id=None and silently coerced it to 0. The TLV with chain_id=0 crossed the host->device boundary and was interpreted by the firmware as CHAIN_ID_ALL, producing an authorization signature valid on every chain. A bridge or dApp integration carrying chain_id through Optional types could end up requesting a far broader signing scope than intended without any client-side warning. Tighten the type contract: chain_id is now a required int, with no Optional in the signature and no silent coercion of None. Callers that genuinely want an all-chains authorization must pass the explicit TxAuth7702.CHAIN_ID_ALL sentinel. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_first_sign_chunk() allocates the global keccak context g_tx_hash_ctx via APP_MEM_CALLOC and only the success path (finalize_parsing_helper) and a subset of post-init parsing errors (handle_sign:180) release it. Other early-error returns inside handle_first_sign_chunk (init_tx failure, invalid data offset, unsupported tx type, cx_hash_no_throw failure) propagate the SW directly to handle_sign and out to main.c without freeing the allocation. Likewise, reset_app_context() never released the context, so app_quit() and the implicit reset at the top of handle_first_sign_chunk left previous-session allocations dangling. A host repeatedly triggering aborted INS_SIGN first-chunk APDUs could exhaust the device's app heap. Add a NULL-checked APP_MEM_FREE_AND_NULL of g_tx_hash_ctx to reset_app_context(). Errors that bubble out of handle_sign reach main.c with reset=true, which now centrally releases the context. The existing late-error free in handle_sign is kept as defense in depth; the NULL check makes the double path safe. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…play The legacy signing path accepted transactions that omit the EIP-155 chain_id encoding (vLength == 0) and produced a signature with the legacy v base 27/28. Such signatures are not domain-separated by chain and can be replayed on any EVM-compatible chain that still accepts unprotected legacy transactions. The existing chain-consistency check in finalize_parsing_helper only enforced strict matching for non-mainnet app configurations, so on Ethereum mainnet the replayable mode was never rejected. Reject any LEGACY transaction whose RLP V field is empty, regardless of app configuration. EIP-155 has been mandatory for safe replay protection since 2016, and no current ragger tests rely on the unprotected form. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fields format_param_token() declared `ticker` and `token_info` outside the collection loop. When an iteration matched the native-currency branch, only `ticker` was overwritten while `token_info` kept the value left over from a previous ERC-20 match. The stale `token_info` was then handed to add_to_field_table(), so a generic-parser descriptor that mixes native and unknown ERC-20 addresses could mislabel later entries with the previous token's metadata. Move `ticker` and `token_info` into the per-iteration scope so they always start at NULL, and force the matching branch to repopulate them from scratch (the native branch keeps `token_info == NULL` intentionally). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ields
format_param_raw() accepted MUST_BE and IF_NOT_IN visibility constraints
at the field level but format_int(), format_bool() and format_string()
never called apply_visibility_constraint(). The hide/reject rule encoded
in a signed generic-parser descriptor was therefore a no-op for any
TF_INT, TF_BOOL or TF_STRING parameter, letting attacker-controlled
calldata be approved with misleading on-device output (e.g. a signed
delta the descriptor was meant to constrain, or a boolean toggle hidden
behind an IF_NOT_IN list).
Reshape the three formatters to mirror the existing UINT/ADDRESS/BYTES
flow: they now take the `s_field` and a `to_be_displayed` flag, format
the value first, then evaluate the constraints and call
apply_visibility_constraint(). Per-type comparisons:
* INT — canonical decimal-string equality (matches sign-extended
encodings of varying byte widths).
* BOOL — both sides normalized to 0/1 before comparison.
* STRING — byte-level length + memcmp equality.
Also reset `to_be_displayed = true` at the start of every iteration so
that an IF_NOT_IN hit on one collection element no longer drags
subsequent elements out of the review.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
INS_PERFORM_PRIVACY_OPERATION (0x18) accepted P1_NON_CONFIRM for both the public encryption key (P2=0x00) and the shared secret (P2=0x01). The shared secret is computed via X25519 from the device-derived private key and an attacker-supplied Curve25519 public key, so it is secret material that should never leave the device without the user physically approving the action. The non-confirm path turned the device into a silent ECDH oracle reachable by any host that could send APDUs. Reject P2_SHARED_SECRET + P1_NON_CONFIRM with SWO_CONDITIONS_NOT_SATISFIED. The P2_PUBLIC_ENCRYPTION_KEY path keeps its existing non-confirm semantics — that operation only exposes a public key. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_safe_tlv_payload() and handle_signer_tlv_payload() verify the signed CHALLENGE field of their respective descriptors but never call roll_challenge() afterwards. The global challenge issued by INS_GET_CHALLENGE stayed valid until the next explicit GET_CHALLENGE, so any previously captured signed Safe / Signer descriptor could be replayed within the same app session and the device would display stale signer / threshold information as freshly authorized. Roll the challenge at the end of both TLV payload handlers, on both success and failure, mirroring the pattern already used by the trusted name and proxy info flows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
path_update() walks nested TYPE_CUSTOM fields and allocates a fresh
keccak hash context on every step via push_new_hash_depth(). When the
descent reached MAX_PATH_DEPTH, path_depth_list_push() returned false
but its return value was ignored, so the next iteration kept
descending and kept calling push_new_hash_depth(). For a self-referential
or cyclic schema (A { A a; } or A -> B -> A), this turned into an
unbounded allocation loop until the app's heap was exhausted, denying
service to typed-message signing.
Propagate the path_depth_list_push() failure: when the path stack is
full, abort the descent with SWO_INCORRECT_DATA. The depth cap bounds
the loop for any recursive schema while leaving legitimate nested
non-recursive types unaffected.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…matter
Cover the constraint paths added by the CWE-693 fix. The pre-existing
test_param_raw.c had no TF_INT coverage at all, and TF_BOOL / TF_STRING
were only exercised for the unconstrained 'always display' path.
Add:
* TF_INT - positive value, negative value (regression for CWE-451
8/16-bit signed rendering), and the four constraint matrix entries
(MUST_BE valid/invalid, IF_NOT_IN match/no_match).
* TF_BOOL - four constraint matrix entries.
* TF_STRING - four constraint matrix entries.
A new CREATE_INT_PARAM helper macro fills in the s_value type_size that
format_signed_int_be needs for sign extension.
Many serialize() methods declared '-> bytes' but returned a bytearray they had built up incrementally. Python's mypy does not consider bytearray to be a bytes, so each return-value was flagged. SigningPartner from ragger.pki also accepts strictly bytes for sign(), but the codebase was passing the bytearray buffers directly. Convert at the API boundary: wrap returns with bytes(), wrap sign() args with bytes(). Widen the local _serialize() helper's cdata parameter to Union[bytes, bytearray] since it's an internal type and many callers pass the bytearray buffer they're building. No behavioral change.
cedelavergne-ledger
force-pushed
the
cev/fix_cerberus_medium
branch
from
191e4a5 to
66b9844
Compare
apaillier-ledger
approved these changes
May 12, 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
3 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
Breaking changes
Please complete this section if any breaking changes have been made, otherwise delete it.
Additional comments
Please post additional comments in this section if you have them, otherwise delete it.