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

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

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:

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 <Lc hi> <Lc lo> = 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 == "0020008004<REDACTED:4B>35"