Skip to content

Solana signer check is skipped on account overflow #462

Description

@BitHighlander

Triaged during the 7.14.2 residual pass and deferred out of 7.14.2 as medium. Still present at release/7.14.2 head.

Evidence

lib/firmware/solana.c:479 and :530 (return SOL_TX_REVIEW_OPAQUE precedes tx->num_accounts = ...) against the guard at lib/firmware/fsm_msg_solana.h:394-396

Assessment

Still present at HEAD, unchanged by the five Solana commits on this branch. solana_parseLegacyTx memsets the struct (solana.c:462) and returns OPAQUE at :479 before assigning tx->num_accounts at :480; solana_parseVersionedTx repeats it at :530-531. The guard at fsm_msg_solana.h:394-395 runs solana_signerInTx only for VERIFIED, or for OPAQUE when parsed.num_accounts > 0 — so a declared account count above SOL_MAX_ACCOUNTS (32, solana.h:33) lands in exactly the one combination the guard excludes. I confirm the candidate's correction that solana_signerInTx is a static in fsm_msg_solana.h:300, not in solana.c.

I reject the finding's implied fix, and that reframes the severity. Hoisting tx->num_accounts = ... above the return would not restore the check: tx->accounts[] was never populated on this path, so solana_signerInTx would compare the derived pubkey against 32 zeroed entries and always return false, refusing every transaction with more than 32 accounts. The guard is therefore doing what its own comment says — checking only when the header was parseable — and the real question is a policy one: fail closed on an unverifiable signer, or allow blind signing. The code currently allows it.

Medium, not high. The remaining gates are intact and honest: AdvancedMode must be enabled (fsm_msg_solana.h:405-411), the user must clear an explicit 'Blind Sign / Sign unverified Solana transaction? The device cannot fully verify the contents.' screen (:417-419), and then a final confirm (:443). The primary screen tells the truth about what the device cannot verify; only the secondary signer check is lost, which is the rubric's medium band. I also could not build a fund-theft path: a signature by a key that is not a required signer occupies no slot in the transaction's positional signature array and is unusable, and in the case where the key IS a required signer the check would have passed anyway. The practical cost is that real Solana transactions routinely exceed 32 accounts, so the device silently degrades to blind-sign for a common, legitimate shape. File an issue framing the choice (raise SOL_MAX_ACCOUNTS, or refuse when the signer cannot be verified) rather than a one-line reorder; defer past the tag.


Deferred because 7.14.2 is a minimal security release; this did not clear the bar for a late change. Filed so it is not lost.

Metadata

Metadata

Assignees

No one assigned

    Labels

    securitySecurity-relevant defect or hardening

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions