Skip to content

fix TOCTOU RC: reject incoming APDU if there is already a pending one#1588

Open
iartemov-ledger wants to merge 1 commit into
masterfrom
reject_if_pending
Open

fix TOCTOU RC: reject incoming APDU if there is already a pending one#1588
iartemov-ledger wants to merge 1 commit into
masterfrom
reject_if_pending

Conversation

@iartemov-ledger

@iartemov-ledger iartemov-ledger commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Description

TOCTOU consent latch to the legacy IO loop.

When the app accepts a command that defers its reply (e.g. a transaction review), it sets a reply_pending latch (os_io_set_reply_pending(true)); the reply path clears it (in io_legacy_apdu_tx and u2f_message_reply).
While the latch is set, any new incoming APDU is rejected with SWO_COMMAND_NOT_ACCEPTED (0x6901) instead of being processed.

U2F/CTAP framing is deliberately exempt from the reject gate (CANCEL/RAW/CBOR), with CBOR still arming/clearing the latch like a normal command — so FIDO traffic isn't disturbed (CTAPHID does its own CHANNEL_BUSY arbitration above this layer).

The fix, in one paragraph

A static bool io_reply_pending (latch) in io_legacy/src/os_io_legacy.c:

  • armed when a command is accepted in io_legacy_apdu_rx (excludes CANCEL/RAW);
  • reject gate: while armed, a new command is refused with SWO_COMMAND_NOT_ACCEPTED
    (0x6901) (exempts CANCEL/RAW/CBOR);
  • cleared on the app's reply — io_legacy_apdu_tx (sync + async/deferred) and
    u2f_message_reply (U2F/CTAP, unconditional).

This hooks the shared io_legacy_apdu_rx chokepoint, so it covers every app and every
deferred-UI window without per-app changes.

Findings rebuttal

# Finding (their claim) Severity claimed Why we reject it
H1 CBOR frame passes the reject gate, is answered by u2f_message_reply, which clears the latch unconditionally → TOCTOU window reopens behind a pending review HIGH / "CRITICAL if FIDO2" The exploit needs one app that is both a signer (deferred APDU review) and a FIDO authenticator (processes CBOR). No shipping app is both — signing apps (Monero, Bitcoin, Eth, Solana…) don't process CBOR; the FIDO app has no deferred APDU review. So the unconditional clear in u2f_message_reply can never erase a genuine pending review. Documented in-code with a "make armer-aware if such an app ever ships" pointer. The proposed enum adds fragility to guard a combination that cannot occur.
H1-bis (CBOR→CBOR case) 1st CBOR arms latch + starts UI; 2nd CBOR is exempt from the reject gate → overwrites the shared G_io_apdu_buffer behind the UI (implied by H1) True at the latch level — the latch does not refuse the 2nd CBOR — but the 2nd CBOR never reaches it. The CTAPHID transport serializes one transaction per handle: while the 1st is in process, any non-CANCEL frame gets CTAP1_ERR_CHANNEL_BUSY at lib_u2f/src/u2f_transport.c:81-91 and is never reassembled into a delivered 0x24 packet. So no second memmove into the shared buffer ever happens — no overwrite, no TOCTOU.
H2 A command that arms but never replies leaves the latch stuck true → all later APDUs rejected 0x6901 until power-cycle MEDIUM Self-healing, not a lock. Every normal reply path clears the latch (io_legacy_apdu_tx for sync and async/deferred replies, u2f_message_reply for U2F) — the review's own table admits this. The "fire-and-forget / SWO_NO_RESPONSE without TX" path doesn't exist in the legacy dispatch model: an accepted command always ends in an SW, and any non-OK SW exits to dashboard (state reset). A wedge would be a self-DoS recoverable by disconnect, not a security defect.
W2 The two exemption lists are asymmetric — arm excludes only {CANCEL, RAW}, reject excludes {CANCEL, RAW, CBOR} WARNING (claimed root cause of H1) Intentional division of labor, not a bug. CANCEL/RAW are transport-only frames carrying no command → never arm, never gated. CBOR is a command (it replies like one) so it arms, but it's left exempt from the reject gate on purpose: CBOR↔CBOR concurrency is already serialized one layer down by CTAPHID (CHANNEL_BUSY, u2f_transport.c:81), which also preserves the legitimate CANCEL/keepalive flow. Having the latch also reject CBOR would be redundant and would force it to re-implement the CANCEL exception to avoid breaking FIDO. The asymmetry is correct per frame type.
W3 No negative test for CBOR interleaving during a pending review WARNING Nothing reachable to test. The interleave only exists if H1/H1-bis are reachable, and they aren't (no signer+FIDO app; CBOR→CBOR serialized by the transport). A test would have to fabricate a non-existent app config. The real surface — APDU-interleave-during-review — is covered by the app-monero V-030 ragger tests.
W1 The reject response reuses the incoming packet type WARNING (observation) Correct by design. The 0x6901 rejection must go back to the host on the transport that sent the offending frame (os_io_legacy.c:410). Standard SDK reply behavior, not a flaw.

Test run with the same fix applied to API_LEVEL_26-derived branch (reject_if_pending_26):

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

Breaking changes

It may break:

  • dishonest that sends a 2nd APDU while user is on conf
  • some "honest" uses cases if this unusual behavior is really anticipated (to be found)

Additional comments

Please post additional comments in this section if you have them, otherwise delete it.

Auto cherry-pick in API_LEVEL

If requested to port the commits from this PR on a dedicated API_LEVEL branch,
select the targeted one(s), or add new references if not listed:

[x] TARGET_API_LEVEL: API_LEVEL_26

This will only create the PR with cherry-picks, ready to be reviewed and merged.

Remember:

  • The merge will ALWAYS be a manual operation.
  • It is possible the cherry-picks don't apply correctly, mainly if previous commits have been forgotten.
  • In case of failure, there is no other solution than redo the operation manually...

@codecov-commenter

codecov-commenter commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.87%. Comparing base (719b3a0) to head (1c95fdd).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1588   +/-   ##
=======================================
  Coverage   91.87%   91.87%           
=======================================
  Files          39       39           
  Lines        4653     4653           
  Branches      596      596           
=======================================
  Hits         4275     4275           
  Misses        266      266           
  Partials      112      112           
Flag Coverage Δ
unittests 91.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@iartemov-ledger
iartemov-ledger marked this pull request as ready for review June 8, 2026 15:42
Comment thread io/include/os_io.h Outdated
@iartemov-ledger
iartemov-ledger force-pushed the reject_if_pending branch 2 times, most recently from b4941c1 to 161bf7e Compare June 9, 2026 08:45
@iartemov-ledger iartemov-ledger changed the title fix CWE-367 (TOCTOU Race Condition): reject incoming APDU if there is already a pending one fix TOCTOURC: reject incoming APDU if there is already a pending one Jun 9, 2026
@iartemov-ledger iartemov-ledger changed the title fix TOCTOURC: reject incoming APDU if there is already a pending one fix TOCTOU RC: reject incoming APDU if there is already a pending one Jun 9, 2026
@iartemov-ledger
iartemov-ledger requested a review from Copilot June 9, 2026 14:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a “reply pending” latch to mitigate a TOCTOU issue by rejecting any new incoming APDU command while a previous command is still awaiting its reply, preventing an in-flight transaction from being mutated during on-screen review.

Changes:

  • Add a global IO “reply pending” latch (os_io_set_reply_pending / os_io_reply_pending) in io.
  • Arm the latch when a command is accepted in io_legacy_apdu_rx, and reject subsequent commands with SWO_COMMAND_NOT_ACCEPTED while the latch is armed.
  • Clear the latch when replying via io_legacy_apdu_tx, and also clear it from the U2F/CTAP2 reply path (u2f_message_reply) since that bypasses io_legacy_apdu_tx.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
lib_u2f_legacy/src/u2f_legacy.c Clears the latch from the U2F/CTAP2 reply path.
io/src/os_io.c Adds storage and accessors for the “reply pending” latch.
io/include/os_io.h Exposes the latch API with documentation of intended behavior.
io_legacy/src/os_io_legacy.c Arms/clears the latch around legacy APDU RX/TX and rejects new commands while pending.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib_u2f_legacy/src/u2f_legacy.c Outdated
Comment thread io_legacy/src/os_io_legacy.c
Comment thread io/src/os_io.c Outdated
Comment thread io/include/os_io.h
Comment thread io/src/os_io.c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@bboilot-ledger bboilot-ledger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to notify the DMK/LW team in case they encounter this new status word.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants