Skip to content

Withhold the body of a secret-bearing command when its length will not parse - #19

Merged
mmlado merged 1 commit into
mainfrom
fix/redaction-short-lc
Sep 29, 2026
Merged

mmlado merged 1 commit into
mainfrom
fix/redaction-short-lc

Conversation

@mmlado

@mmlado mmlado commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

Withhold the body of a secret-bearing command when its length will not parse

An Lc larger than the bytes present left the data field unsplit, and the command then
took the path that masks only registered values. An unregistered PIN reached the transcript
in the clear.

Changes

  • _parse_command reports a seventh value, data_located, distinguishing "carries no data"
    from "the length field disagreed with the bytes present".
  • redact_command fails closed on the second case for commands that can carry a secret:
    header plus the size of what was withheld, without inventing an Lc.
  • _may_carry_secret decides that from the header alone.
  • Seven tests in tests/unit/test_redaction.py, four of them regressions.
  • CHANGELOG entry under Fixed.

What leaked

Redactor().redact_command(bytes.fromhex("0020008006313233"))

Lc claims six bytes and three follow. Before: 0020008006313233, where 313233 is the
ASCII PIN 123. After: 00200080<REDACTED:4B>. The same held for CHANGE REFERENCE DATA,
for the extended-length form, and for CTAP clientPIN.

These land in the file written by --apdu-log, whose own header reads
APDU transcript (secrets redacted).

Constraints

if data and _is_sensitive(...) was the whole bug. With the length unparsed, data is
empty, so the guard is false and control falls to mask(), which only masks values passed to
Redactor.register. A PIN typed at the prompt is registered; one arriving another way is not.
Command-aware redaction is the layer that is supposed to cover that gap, and this is the hole
in it.

CTAP fails closed as a family. clientPIN is identified by the first data byte, which is
exactly what an unparseable length hides, so a CTAP message with a bad length is withheld even
if it was really getInfo. Well-formed CTAP is unaffected and still distinguishes the two.

Failing closed is scoped to commands that can carry a secret. A malformed GET DATA still
renders in full, so raw apdu send debugging keeps working. Two control tests hold that line,
along with one for the case-1 retry-counter probe, which carries no data and must stay
readable since it is what the transcript is most often read for.

No Lc is fabricated. The withheld form is header plus marker, deliberately different
from the well-formed header + Lc + marker + trailer, because the real length is unknown.

Verification

ruff check, ruff format --check, mypy, sphinx-build -W --keep-going pass. The suite
goes 350 to 357. Before the fix the four regression tests fail and the three controls pass.
Coverage of secrets/redaction.py is 93%.

🤖 Generated with Claude Code

…t parse

An Lc larger than the bytes present left the data field unsplit, so
redact_command's `if data and _is_sensitive(...)` guard was false and the
command fell through to mask(), which only masks values passed to register().
An unregistered PIN therefore reached the transcript in the clear - in the
file whose own header reads "APDU transcript (secrets redacted)".

_parse_command now reports whether it located the data field, and
redact_command fails closed for commands that can carry a secret: header plus
the size of what was withheld, with no fabricated Lc. CTAP is withheld as a
family because clientPIN is identified by the first data byte, which is what
the bad length hides. Commands that cannot carry a secret still render in
full so raw `apdu send` debugging keeps working, and the case-1 retry-counter
probe is untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmlado
mmlado merged commit ae29122 into main Sep 29, 2026
12 checks passed
@mmlado
mmlado deleted the fix/redaction-short-lc branch September 29, 2026 14:53
@mmlado
mmlado requested a review from embarquech September 29, 2026 14:53
@mmlado mmlado self-assigned this Sep 29, 2026
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.

1 participant