Skip to content

docs: say in SECURITY.md that hostile captures are a live risk, not a closed one - #611

Merged
JarryShaw merged 2 commits into
mainfrom
docs/security-untrusted-input
Sep 22, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
docs/security-untrusted-input

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Reviewing SECURITY.md against the ask "see if any updates needed". It was rewritten from GitHub's unedited template in #550 one day ago, so the first job was establishing whether it was already current rather than assuming it needed work.

What I verified, and what held up

Everything factual in the file checks out. Recorded here so the next review does not have to redo it:

Claim in the file Checked against Result
1.5.x is the pre-release development line pcapkit/__init__.py1.5.0b4 accurate
1.4.x is current stable docs/source/changelog/ — highest non-1.5 entry is 1.4.1.rst accurate
test matrix covers CPython 3.10–3.14 unit-tests.yml and python-compatibility.yml both matrix 3.10–3.14, 3.15 as experimental: true accurate
requires-python = ">=3.6, <4" pyproject.toml:49 quoted exactly
"see the note above requires-python" pyproject.toml:41-48 the note is real and substantive
private vulnerability reporting "is enabled on this repository" GET /repos/JarryShaw/PyPCAPKit/private-vulnerability-reporting{"enabled":true} accurate, and the advisory URL form is right
email fallback via CODE_OF_CONDUCT.md resolves to jarryshaw@icloud.com a real address, not a placeholder
the six optional engines named as out of scope dpkt scapy pyshark pypcap pcap-ct pypcapfile, all real extras in pyproject.toml none invented
response expectations "best-effort rather than contractual", acknowledgement "normally within a week" does not over-commit for a single maintainer

So no correction was needed to the supported versions, the reporting channel, or the response expectations. This PR does not touch any of them.

The one gap

The threat model is written for a reporter — it says which classes of defect are in scope. It never tells a user that defects of exactly those classes are present and unfixed right now, and one sentence actively implied the opposite:

parse errors are supposed to arrive as pcapkit.utilities.exceptions types, not as an arbitrary traceback

MemoryError appears nowhere in pcapkit/ (grep -rn MemoryError pcapkit/ → no matches), so the hierarchy provably does not cover it. Stated as a bare promise, that is the same species of untrue documentation #550 set out to remove. Reworded to intent plus a known shortfall, keeping the class in scope for reports.

The new Handling untrusted captures section then gives callers the advice the threat model implied but never stated: bound memory and time out of process, expect built-in exception types the hierarchy misses, and revalidate any parsed field that carries a security decision.

That last point is not hypothetical — a short read can pad to a value rather than raise, so a caller trusting the value gets a wrong answer with no error. Note the existing out-of-scope bullet ("a capture that parses to the wrong values without any security consequence") is saved only by its qualifier; a reporter skimming it may self-triage that class away. Flagged for your call, not changed here.

What this deliberately does not publish

No amplification figures, no capture shapes, no issue numbers. The open vectors stay described where they already are — docs/source/changelog/1.5.0.rst already states the crafted shape, the ratio, the retained bytes and that the 16-bit band is deliberately untouched, in far more operational detail than a policy file would carry. Nothing here is news to an attacker, and this file only needs to stop implying the work is finished. Whether to go further and name the open vector explicitly in the policy is your decision.

Also noticed, not changed

.github/dependabot.yml covers pip only. There is no github-actions ecosystem entry, so the action versions pinned across the workflows never get update PRs. Adding it is a real if small hardening, but it would open a wave of PRs at once, so it is left for you to decide.

No changelog bullet, following #550, which rewrote this file wholesale and added none — repository policy documents are not a release note here.

Docs-only; no code touched, no tests run.

… closed one

`SECURITY.md` was rewritten from GitHub's template in #550 and its facts still
check out, so this is the one gap that review found rather than a rework.

* The threat model promised that parse errors "are supposed to arrive as"
  `pcapkit.utilities.exceptions` types. `MemoryError` appears nowhere in
  `pcapkit/`, so the hierarchy demonstrably does not cover it and the sentence
  over-claimed. Reworded to intent plus a known shortfall, keeping the class in
  scope for reports.
* Added "Handling untrusted captures", giving callers the advice the threat model
  implied but never stated: bound memory and time out of process, expect
  built-in exception types the hierarchy misses, and revalidate any parsed field
  that carries a security decision. A short read can pad to a value instead of
  raising, so the last one is not hypothetical.

Deliberately non-specific: no amplification figures, capture shapes or issue
numbers. The open vectors stay described where they already are, in
`docs/source/changelog/1.5.0.rst`, which is a record of what was measured; this
file only needs to stop implying the work is finished.

No changelog bullet, following #550, which rewrote this file wholesale and added
none -- repository policy documents are not a release note here.
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — head a9de6ce7cc4af9047f87affadf31c9ac0d136786. The core premise (the changelog already publishes far more operational detail than this vague caveat) is independently confirmed with exact matching figures; every other factual claim in the PR I spot-checked (version, requires-python, changelog line-up, dependabot ecosystem, private vulnerability reporting, MemoryError absence, struct.error handling) checked out.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review appendix — PR #611

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head a9de6ce7cc4af9047f87affadf31c9ac0d136786 in an isolated worktree (/tmp/pcapkit-review/pr611, removed after this review). Docs-only, no tests to run.

CI

Rollup PENDING, CheckRun tally 14 SUCCESS, 2 SKIPPED, rest QUEUED, 0 FAILURE/CANCELLED.

The premise — checked, not assumed

The PR's argument for publishing rests on: "docs/source/changelog/1.5.0.rst already publishes #594's full specifics, so withholding a vague caveat protects nobody." Read the actual changelog text (lines 351-396) rather than taking this on faith. It is substantially more specific than anything the SECURITY.md diff adds:

  • Names the exact amplification ratio twice, matching this exact figure and matching the run-history example from an earlier stage of this same review programme: "retains 125.00 MiB of synthesised zeros for 216.88 MiB of RSS -- 1,637x its own size... amplifies by the same 1,637x."
  • States the crafted shape explicitly: "A crafted 80,048-octet PCAP-NG file of 2,000 Enhanced Packet Blocks, each carrying one option declaring 65,535 octets against four real ones."
  • States the exact history-dependence failure mode: "the same legitimate 54-octet frame declaring an IPv4 total length of 65,535 parse[d] to one result on 37 of 40 identical calls and to another on calls 26, 33 and 39."
  • States the specific threshold (65,536 octets), the exact worst-case legitimate shortfalls measured (65,495 and 64,750 octets), and explicitly says the 16-bit band "is deliberately untouched."

This is public (it is the source docs/source/changelog/1.5.0.rst renders to GitHub Pages and, via util/changelog_md.py, into the repository's own CHANGELOG.md). The SECURITY.md diff, by contrast, names no figures, no capture shapes, and no issue numbers at all — exactly as its own "What this deliberately does not publish" section claims. The premise holds: there is materially more detail already public than what this PR adds, so treating the new caveat as a disclosure risk would be inconsistent with what the project already ships.

The MemoryError claim

grep -rn MemoryError pcapkit/ produces no matches — confirmed directly, matching the PR's own stated check. MemoryError is a genuine gap in the pcapkit.utilities.exceptions hierarchy: nothing in the package catches or wraps it, so it would reach a caller as a bare built-in, contradicting the pre-existing "parse errors are supposed to arrive as pcapkit.utilities.exceptions types" line the PR reworded.

The struct.error/StructError correction

Verified this is handled correctly, and does not escape: pcapkit/protocols/protocol.py:1058 (except struct.error as error:) re-raises as StructError at :1063, and the packing-direction mirror does the same at :1155-1156. So struct.error is caught and wrapped everywhere in protocol.py, unlike MemoryError. This matches the correction attributed to this PR against an earlier, wrong claim in the coordinator's own brief.

Other factual claims — spot-checked

  • pcapkit/__init__.py:132: __version__ = '1.5.0b4' — confirms "1.5.x is the pre-release development line."
  • pyproject.toml:49: requires-python = ">=3.6, <4" — quoted exactly.
  • docs/source/changelog/: highest non-1.5 entry is 1.4.1.rst — confirms "1.4.x is current stable."
  • gh api repos/JarryShaw/PyPCAPKit/private-vulnerability-reporting{"enabled":true} — confirms the reporting-channel claim exactly, including that it resolves as stated.
  • .github/dependabot.yml: only a package-ecosystem: "pip" entry exists, no github-actions entry — confirms the "also noticed, not changed" observation.

Not independently checked

  • The CI workflow matrix claim (3.10–3.14 required, 3.15 experimental) — read the PR's own description of unit-tests.yml/python-compatibility.yml but did not open those files myself to re-derive it; no reason to doubt it given everything else checked out.
  • The CODE_OF_CONDUCT.md email-resolves-to-a-real-address claim — not independently verified (would require contacting the address, which is out of scope for a code review).
  • The six named optional engines (dpkt, scapy, pyshark, pypcap, pcap-ct, pypcapfile) as real pyproject.toml extras — not individually re-checked against the extras table.

Disagreement log

None. Every claim I checked — the central premise, the MemoryError gap, the StructError correction, and the spot-checked factual table — held up exactly.

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

Labels

docs Pull requests that change documentation only (docs: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant