From 72f8395ccdc847471cf77679f91a760e9827158b Mon Sep 17 00:00:00 2001 From: Mladen Milankovic Date: Tue, 29 Sep 2026 15:44:13 +0200 Subject: [PATCH] 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, 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) --- CHANGELOG.md | 8 ++++ src/cryptnox_id_cli/secrets/redaction.py | 43 ++++++++++++++---- tests/unit/test_redaction.py | 56 ++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 538a1b7..2eed358 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The APDU transcript no longer renders the data field of a secret-bearing + command whose length byte disagrees with the bytes present. An `Lc` larger + than the remaining bytes left the data field unsplit, and the command then + took the path that masks only values registered with the redactor, so an + unregistered PIN could reach the log in the clear. Commands that can carry a + secret now withhold the whole body when the length does not parse; commands + that cannot still render in full, and the case-1 retry-counter probe is + unaffected. - `doctor` no longer advises "use a DESFire-capable contactless reader" to someone already on a contactless one. It now tells the detector which reader the session is on, so the DESFire diagnosis uses the reader name as well as diff --git a/src/cryptnox_id_cli/secrets/redaction.py b/src/cryptnox_id_cli/secrets/redaction.py index ac00fc0..45e8163 100644 --- a/src/cryptnox_id_cli/secrets/redaction.py +++ b/src/cryptnox_id_cli/secrets/redaction.py @@ -63,25 +63,33 @@ def _replace_aligned(text: str, needle: str, marker: str) -> str: return "".join(out) -def _parse_command(apdu: bytes) -> tuple[int, int, int, int, bytes, bytes] | None: - """Best-effort split into (cla, ins, p1, p2, data, trailer). Short + extended.""" +def _parse_command(apdu: bytes) -> tuple[int, int, int, int, bytes, bytes, bool] | None: + """Best-effort split into (cla, ins, p1, p2, data, trailer, data_located). + + ``data_located`` is False when the length field disagrees with the bytes present, + so the data field could not be isolated and everything after the header sits in + ``trailer``. Callers must not read that as "carries no data": an Lc larger than + the remaining bytes is exactly how a truncated VERIFY looks. + """ if len(apdu) < 4: return None cla, ins, p1, p2 = apdu[0], apdu[1], apdu[2], apdu[3] body = apdu[4:] if not body: - return (cla, ins, p1, p2, b"", b"") + return (cla, ins, p1, p2, b"", b"", True) if len(body) == 1: # Le only (case 2) - return (cla, ins, p1, p2, b"", body) + return (cla, ins, p1, p2, b"", body, True) if body[0] == 0x00 and len(body) >= 3: # extended length lc = (body[1] << 8) | body[2] if lc and len(body) >= 3 + lc: - return (cla, ins, p1, p2, body[3 : 3 + lc], body[3 + lc :]) - return (cla, ins, p1, p2, b"", body) # extended Le, no data + return (cla, ins, p1, p2, body[3 : 3 + lc], body[3 + lc :], True) + # lc == 0 is the extended Le form and genuinely carries no data; any other + # value is an Lc that overruns what is here. + return (cla, ins, p1, p2, b"", body, lc == 0) lc = body[0] # short length if lc and len(body) >= 1 + lc: - return (cla, ins, p1, p2, body[1 : 1 + lc], body[1 + lc :]) - return (cla, ins, p1, p2, b"", body) + return (cla, ins, p1, p2, body[1 : 1 + lc], body[1 + lc :], True) + return (cla, ins, p1, p2, b"", body, False) def _is_sensitive(cla: int, ins: int, data: bytes) -> bool: @@ -91,6 +99,17 @@ def _is_sensitive(cla: int, ins: int, data: bytes) -> bool: return (cla & 0xF0) == 0x80 and ins == 0x10 and data[:1] == b"\x06" +def _may_carry_secret(cla: int, ins: int) -> bool: + """Sensitivity decided from the header alone, for a body that would not split. + + Used only when the length field is inconsistent, so the data bytes cannot be + inspected. CTAP is treated as sensitive as a family here: ``clientPIN`` is + identifiable only from the first data byte, which is precisely what an + unparseable length hides. + """ + return ins in SENSITIVE_INS or ((cla & 0xF0) == 0x80 and ins == 0x10) + + class Redactor: """Masks secrets in hex transcripts. Stateless except for the secret registry.""" @@ -121,12 +140,18 @@ def redact_command(self, apdu: bytes | bytearray) -> str: parsed = _parse_command(apdu) if parsed is None: return self.mask(apdu.hex().upper()) - cla, ins, p1, p2, data, trailer = parsed + cla, ins, p1, p2, data, trailer, data_located = parsed if data and _is_sensitive(cla, ins, data): header = apdu[:4].hex().upper() lc = f"{len(data):02X}" if len(data) <= 0xFF else f"00{len(data):04X}" tail = trailer.hex().upper() return f"{header}{lc}{_MARK.format(n=len(data))}{tail}" + if not data_located and trailer and _may_carry_secret(cla, ins): + # Fail closed. The length field did not agree with the bytes present, so + # the data field was never isolated and the registered-secret path below + # would mask nothing unless the value happened to be registered. Emit the + # header and the length of what was withheld, without inventing an Lc. + return f"{apdu[:4].hex().upper()}{_MARK.format(n=len(trailer))}" return self.mask(apdu.hex().upper()) def redact_response_data(self, data: bytes | bytearray, *, ins: int) -> str: diff --git a/tests/unit/test_redaction.py b/tests/unit/test_redaction.py index ab55752..7d34526 100644 --- a/tests/unit/test_redaction.py +++ b/tests/unit/test_redaction.py @@ -105,3 +105,59 @@ def test_chained_change_reference_data_masked(): out = Redactor().redact_command(apdu) assert component not in out assert "REDACTED" in out + + +# ---------------------------------------------------------------- short Lc --- # +# An Lc larger than the bytes actually present cannot be split into a data field. +# The parser then reports no data, and a `if data and ...` guard would wave the +# command through to the registered-secret path - which masks nothing when the PIN +# was never registered. The bytes land in a file whose own header says the secrets +# in it are redacted, so this fails closed instead. + + +def test_verify_with_overlong_lc_does_not_leak_the_pin(): + # Lc claims 6 bytes, only 3 follow: ASCII "123". + out = Redactor().redact_command(bytes.fromhex("0020008006313233")) + assert "313233" not in out + assert out.startswith("00200080") + assert "REDACTED" in out + + +def test_change_reference_data_with_overlong_lc_does_not_leak(): + out = Redactor().redact_command(bytes.fromhex("00240080FF313233")) + assert "313233" not in out + assert "REDACTED" in out + + +def test_ctap_clientpin_with_overlong_lc_does_not_leak(): + # clientPIN is identifiable only from the first data byte, which is exactly what + # an unparseable length hides, so CTAP messages fail closed as a family. + out = Redactor().redact_command(bytes.fromhex("80100000060601")) + assert out.startswith("80100000") + assert "REDACTED" in out + + +def test_extended_length_with_overlong_lc_does_not_leak(): + # 00 = 0x0010 claimed, 3 bytes present. + out = Redactor().redact_command(bytes.fromhex("00200080" + "000010" + "313233")) + assert "313233" not in out + assert "REDACTED" in out + + +def test_pin_status_probe_stays_readable(): + # Control: the case-1 retry-counter probe carries no data and must keep rendering, + # otherwise the transcript loses the command it is most often read for. + assert Redactor().redact_command(bytes.fromhex("00200080")) == "00200080" + + +def test_non_sensitive_command_with_overlong_lc_still_renders(): + # Control: failing closed applies to commands that can carry a secret, not to + # every malformed APDU - raw `apdu send` debugging depends on seeing the bytes. + out = Redactor().redact_command(bytes.fromhex("00CB3FFF05AABB")) + assert out == "00CB3FFF05AABB" + + +def test_well_formed_sensitive_command_is_unchanged_by_the_guard(): + # Control: the ordinary path keeps its Lc and trailing bytes. + out = Redactor().redact_command(bytes.fromhex("00200080043132333435")) + assert out == "002000800435"