Repository navigation
fix(pcapng): cap a Decryption Secrets Block's secrets at its block (#1980) - #1983
Conversation
…1980) - dsb_secrets_selector sized the secrets from secrets_length alone, so a length past the block read the blocks after it, and past the zero-pad ceiling raised FieldValueError inside the schema, aborting the extract. The new secrets_area bounds it at the Block Total Length less the framing, sharing _held_octets with captured_area; padding_data and the option area are sized from the bounded length. - DecryptionSecretsBlock.post_process keeps the declared secrets_length and warns with a ProtocolWarning worded like the EPB one, on the unpacking path only. - A key log the block ends inside drops its cut last line, as a short read already does, so the TLS and WireGuard parsers keep the entries before it instead of raising. - Document secrets_area; the selector unit test now passes the block length. New test module fails on origin/main and passes here; tests/protocols/misc, tests/protocols/schema, the PCAP-NG engine and tier-guard tests pass. Closes #1980
JarryShaw
added
fix
|
NEEDS CHANGES at |
JarryShaw
added
review: needs-changes
…#1980) - _make_block_dsb recomputed secrets_length from the secrets it wrote, so a block whose declared length ran past it rebuilt with the held length. It now keeps the data model's declared secrets_length when that exceeds the secrets held, as the EPB build keeps captured_len; a block built fresh from user data still computes it. - The secrets are measured in the context of the block they were parsed from, so a key log the block cut is still read as cut instead of being refused while the block is rebuilt. The new rebuild assertions fail on 11f3f27 and pass here. Closes #1980
|
Pushed |
JarryShaw
added
review: running
|
NEEDS CHANGES at
The declared length should be kept only on a true overrun, where it runs past the block body; otherwise the length of the secrets held is used. The 19 failures the review saw in |
JarryShaw
added
review: needs-changes
|
CodeQL fails on this PR with 9 "clear-text logging/storage of sensitive data" alerts, all false positives. The SARIF traces every alert back to one source: the new warning's There are three ways to clear it:
I recommend option 1. Dismissing alerts is a security-tab action, though, so I'll leave it to you unless you say otherwise. |
JarryShaw
added
the
needs: decision
…lock (#1980) - _make_block_dsb kept a data model's secrets_length whenever it exceeded the secrets written, so a length that fits the block's padding, or one an edit to secrets_data had outdated, was written over the new secrets and the block came out shorter than its Block Total Length. It is now kept only on a true overrun, past what the block body holds as secrets_area measures it, the condition the parse side warns on. - Test a ZigBee key declared within its padding and an edited unknown DSB for a block as long as its Block Total Length says. The new cases fail on aed6dfe and pass here; tests/protocols/misc and tests/protocols/schema pass. Closes #1980
|
Pushed |
JarryShaw
added
review: running
|
GOOD TO GO at |
JarryShaw
added
review: good-to-go
|
Option 1 |
|
Done. I dismissed the CodeQL alerts on this PR as false positives, with the reason recorded on each. CodeQL now reports success. |
JarryShaw
removed
the
needs: decision
|
Coverage: 91.52% (unit tier, Python 3.15,
Per-file detail: the |
JarryShaw
removed
the
review: good-to-go
…#1985) (#1989) * fix(pcapng): warn on a malformed TLS key-log line instead of aborting (#1985) A TLS or WireGuard key log line that did not parse raised FieldValueError from post_process, which aborted the whole extraction, wherever the line sat in the log. It now raises a ProtocolWarning ("...; line kept as captured"), is left out of the entries, and the well-formed entries around it are kept. The log text is already kept verbatim on the data model, so the block still rebuilds octet for octet. A last line the capture or the block cut is still left out silently, as #1983 does. Tests that pinned the old refusal now pin the warning. Closes #1985 * fix(pcapng): keep a malformed key-log line out of entries entirely (#1985) TLSKeyLog.post_process indexed entries[label] before bytes.fromhex ran, so a line with a known label but fields that do not decode left an empty bucket for that label. Both values are decoded first and entries is touched only once they are. WireGuardKeyLog decodes before it adds, so it needs no change. Also drops timed wording from the test docstrings and rewraps the post_process docstring. Refs #1985 * fix(pcapng): elide key-log values from the malformed-line warning (#1985) The malformed-line warning quoted the whole key-log line, which carries key material. _malformed_line_message names only the label, when the key log knows it, and elides the values. An unrecognised label is not quoted either, since it may be key material too. The WireGuard parser's internal ValueError no longer carries the line. Tests assert the new text and that no warning quotes a hex secret or base64 key from their input. Refs #1985
make pylint,make mypy,make isort)make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix— a bug fixDescription of your pull request and other information
A Decryption Secrets Block whose
secrets_lengthruns past its block no longer aborts extraction. A newsecrets_areacaps the secrets at what the block holds; it shares_held_octetswithcaptured_area. The DSB's padding and option area are sized from the capped length.DecryptionSecretsBlock.post_processkeeps the declaredsecrets_lengthand warns, the way the EPB does ("secrets length N runs past the block … kept as captured"). If a key log is cut off by the block, its partial last line is dropped, as a short read already does, so the TLS and WireGuard parsers keep the complete entries. The frames before and after the bad block survive.An audit of the module's other length-driven fields found them already capped at the block, or computed from the block's own length.
The new
test_pcapng_dsb_secrets_overrun_1980_unit.pyfails onmain(8 failed) and passes here. Well-formed DSBs still rebuild byte-exact.Closes #1980