Fix cerberus#1025
Closed
cedelavergne-ledger wants to merge 5 commits into
Closed
Conversation
The home-screen plugin tagline builder accumulated the FORMAT_PLUGIN template length and the caller-controlled caller_app->name length into a uint8_t. A sufficiently long caller name could wrap that accumulator, leading to an undersized allocation for g_tag_line and a subsequent snprintf overflow into adjacent memory across the on-device library-call trust boundary. Bound the plugin name with strnlen() against an explicit maximum and perform the length math in size_t to make the wrap unreachable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1025 +/- ##
========================================
Coverage 56.04% 56.04%
========================================
Files 26 26
Lines 2414 2414
Branches 323 323
========================================
Hits 1353 1353
Misses 1057 1057
Partials 4 4
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
|
…th swap The EIP-7702 authorization handler kept no signing state. handle_sign_ eip7702_authorization() accepted a new P1_FIRST_CHUNK at any time and immediately overwrote tmpCtx.authSigningContext7702.bip32 before the TLV payload was complete. Because tlv_from_apdu() returns success for buffered partial payloads, a hostile host could start a second authorization mid-review, swap the BIP32 path, and have auth_7702_ok_cb sign the displayed authorization hash with a different derivation path than the one shown on screen. Introduce a dedicated APP_STATE_SIGNING_EIP7702 and gate the handler on it: refuse P1_FIRST_CHUNK unless idle, refuse continuation chunks without an active session, and clear the state on error or on approval (auth_7702_cancel_cb already resets via send_status). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Both staking plugins set uiType=ETH_UI_TYPE_GENERIC, which suppresses the standard Amount/To pair. The review then displayed only the withdrawal-request amount or validator pubkeys parsed from calldata, never the native tx.value carried by the transaction. A malicious dApp could attach an arbitrary ETH/native amount to a staking-style request and have the user authorize it without seeing the value on screen. Add a "Tx value" review screen to both EIP-7002 and EIP-7251 plugins whenever the transaction's native value is non-zero, and adjust the screen index → screen kind mapping so the existing validator/amount screens remain in their expected order. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ERC1155 safeBatchTransferFrom (0x2eb2c2d6) review path explicitly skipped copying token IDs (`// don't copy anything since we won't display it`) and summed every value into a single context->value. The device then only rendered "<total> from <count> NFT IDs", which let a malicious dApp slip a high-value token ID into a batch of innocuous ones without the user being able to detect it on-screen. Capture the first ERC1155_BATCH_DISPLAY_MAX (id, value) pairs into the plugin context, render each pair on its own pair of screens, and add an explicit "WARNING — Only first N of M IDs shown" screen whenever the batch is larger than what we can surface. The aggregate Total Quantity screen is preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
handle_set_plugin() parsed the signed chain_id only to call app_compatible_with_chain_id() and then discarded it. The stored tokenContext kept the plugin name, contract address, and selector but never the chain the registration was signed for. eth_plugin_perform_ init_default() therefore authorized the plugin solely on address + selector match, which let a host preload a valid signed registration for chain A and then route a transaction on chain B through the same plugin UI, masking attacker-controlled calldata behind a familiar review flow. Store the signed chain_id alongside the registration and refuse to activate the plugin when the transaction's chain_id differs. set_external_plugin keeps its chain-unbound semantics (its signed payload contains no chain_id) and is marked explicitly via PLUGIN_CHAIN_ID_ANY; cross-chain protection there requires a protocol change and is left out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cedelavergne-ledger
force-pushed
the
cev/fix_cerberus
branch
from
50867ce to
da96a88
Compare
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
Please provide a detailed description of what was done in this PR.
(And mentioned if linked to an issue docs)
Changes include