Threat model chapter 4

[Dreamsorcerer]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.95%. Comparing base (93de128) to head (f2b5297).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12706   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files         131      131           
  Lines       46688    46688           
  Branches     2421     2421           
=======================================
  Hits        46200    46200           
  Misses        366      366           
  Partials      122      122           

Flag	Coverage Δ
Autobahn	22.42% <ø> (ø)
CI-GHA	98.92% <ø> (-0.01%)	⬇️
OS-Linux	98.67% <ø> (-0.01%)	⬇️
OS-Windows	97.03% <ø> (ø)
OS-macOS	97.92% <ø> (-0.01%)	⬇️
Py-3.10	98.15% <ø> (ø)
Py-3.11	98.41% <ø> (-0.01%)	⬇️
Py-3.12	98.50% <ø> (ø)
Py-3.13	98.47% <ø> (-0.01%)	⬇️
Py-3.14	98.50% <ø> (+<0.01%)	⬆️
Py-3.14t	97.55% <ø> (ø)
Py-pypy-3.11	97.42% <ø> (-0.01%)	⬇️
VM-macos	97.92% <ø> (-0.01%)	⬇️
VM-ubuntu	98.67% <ø> (-0.01%)	⬇️
VM-windows	97.03% <ø> (ø)
cython-coverage	37.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 72 skipped benchmarks¹

---

_{Comparing threat-model4 (f2b5297) with master (93de128)}

Footnotes

1. 72 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[Dreamsorcerer]

@aiolibsbot review

[aiolibsbot]

PR Review — Threat model chapter 4

Documentation-only PR extending the threat model with §5.4 Multipart parsing & encoding. The technical content is largely accurate — I spot-checked the cited commit hashes (5fe9dfb, 9cc4b91, dab9e87), the Request.post() nested-multipart rejection at web_request.py:760, the FormData.add_field CR/LF check, and the read_chunk base64 alignment loop, and they match the description. Two issues warrant attention before this lands:

1. The §5.3 WebSocket Open questions section is silently deleted by this diff, taking unresolved follow-up 3.1 (RFC 6455 mask-direction enforcement) with it — the document loses a live security follow-up.
2. Threat 4.9's Recommended action asks for an audit of base64 4-byte alignment in read_chunk, but that alignment is already implemented (multipart.py:347-370). The real residual concern is b64decode(..., validate=False) tolerance of stray bytes, not the chunk-boundary case.

The rest is style/consistency: dead anchors to §5.5–5.7 and §6.1 will need to follow shortly, and the threat 4.3 Recommended column should carry the max_nesting_depth proposal explicitly rather than parking it in the open question.

---

🟡 Important

1. §5.3 WebSocket open questions silently deleted (`THREAT_MODEL.md`, L541-548)

This diff inserts §5.4 between §5.3's Past advisories / hardening (recap) block and the existing Open questions. section, then replaces the three WebSocket open questions with the multipart ones. As a result §5.3 ends without an Open questions subsection at all, and the WebSocket follow-ups are lost from the document.

Questions 3.2 (PRNG mask source) and 3.10 (no-context-takeover default) were resolved as Documented design decision in the §5.3 mitigation table by the chapter 3 PR, so dropping them is defensible — but question 3.1 (RFC 6455 §5.1 mask-direction enforcement) is still flagged as Recommended hardening in the existing mitigation row at line 517 and has not been resolved anywhere else in the document. Deleting it drops a live follow-up off the audit trail.

Suggested fix: keep §5.3's open questions block (trimmed to question 1 if you want to retire 2 and 3 as design decisions, with a one-line rationale referencing the mitigation column) and add a fresh **Open questions.** block at the end of §5.4 for the multipart items. That preserves the per-chunk structure the document has been building.

-1. Should server-side reader reject unmasked frames (and client-side reject
-   masked ones) per RFC 6455 §5.1? (Threat 3.1 — recommended.)
-2. Is the PRNG mask source (`random.getrandbits`) sufficient, or should it be
-   migrated to `secrets`/`os.urandom`? (Threat 3.2.)
-3. For long-lived WebSocket sessions, is there a use case for *forcing*
-   `no_context_takeover` defaults to limit memory growth? (Threat 3.10.)

2. Threat 4.9 audit recommendation describes already-shipped mitigation (`THREAT_MODEL.md`, L605)

The Recommended column for 4.9 reads:

Audit whether read_chunk aligns reads on 4-byte boundaries for base64 parts — base64.b64decode accepts non-base64 bytes silently (with validate=False default), so a mid-quartet chunk split can lose data without raising. If this isn't already aligned, add the alignment or buffer the trailing bytes until the next chunk.

The alignment is already in place. aiohttp/multipart.py:read_chunk (current master, lines 347–370) detects when Content-Transfer-Encoding: base64 is set, strips whitespace from the chunk, computes the remainder against the 4-byte quartet boundary, and loops to pull additional bytes (first from self._prev_chunk, then from the underlying content stream) until the chunk is quartet-aligned before returning. So the silent-truncation failure mode the recommendation worries about can't fire from a chunk split alone — it would require the wire payload to actually be unaligned, which is a peer-supplied corruption, not a parser bug.

Since the document treats the Recommended column as the action list for maintainers and the Past advisories / hardening (recap) block as the verified-in-place audit trail, this should either (a) move the alignment recap into Existing/Past advisories with a code reference, or (b) drop the audit ask and substitute the real residual concern: base64.b64decode(..., validate=False) still tolerates stray non-alphabet bytes mid-chunk and produces silently corrupted output. That is the genuine 4.9 follow-up — enable validate=True or document the trust assumption — not the alignment audit.

# aiohttp/multipart.py:347-370
# For the case of base64 data, we must read a fragment of size with a
# remainder of 0 by dividing by 4 for string without symbols \n or \r
encoding = self.headers.get(CONTENT_TRANSFER_ENCODING)
if encoding and encoding.lower() == "base64":
    stripped_chunk = b"".join(chunk.split())
    remainder = len(stripped_chunk) % 4

    while remainder != 0 and not self.at_eof():
        ...

🟢 Suggestions

1. Forward references to §5.5, §5.6, §5.7 and §6.1–§6.4 are dead links until later chapters land (`THREAT_MODEL.md`, L549)

The Scope paragraph and several mitigations link to #55-compression-codecs, #56-streams--payloads, #57-server-connection-lifecycle, and #61-highest-leverage-recommendations (§5.4 inherits the §6.1 reference style from §5.3). Until those sections exist, the rendered Sphinx anchors will 404. That's fine if chapters 5/6/7 are queued behind this one, but worth either (a) calling out in the PR description that the broken anchors land green in the next PR, or (b) softening the references to plain prose (§5.5 (Compression codecs, future chapter)) so the published threat model doesn't ship dead links if chapter 4 lands solo.

back-stop slow drips ([§5.7](#57-server-connection-lifecycle)).

2. Threat 4.3 mitigation: depth cap is a `Recommended`, not just an open question (`THREAT_MODEL.md`, L581)

Threat 4.3 (nested-multipart recursion) is rated Medium and the mitigation column explicitly proposes adding a max_nesting_depth parameter, then defers it to Tracked as a follow-up. This is the only Medium-rated threat in the table whose Recommended action is parked behind the open question. Given the document's convention (§5.3 row 3.1 is also a Medium-rated Recommended hardening), this would be more consistent if the Recommended cell said something like **Recommended hardening: add max_nesting_depth (default ~10) to MultipartReader; tracked as open question 1.** so the action item shows up in the mitigation column where readers scan for it, with the open question carrying the design discussion (default value, whether to error or yield-and-stop).

| 4.3 | Nested-multipart recursion | `Request.post()` rejects any nested multipart... | **Open question.** Direct `MultipartReader` users get unlimited recursion. Consider adding a `max_nesting_depth` parameter...

---

Checklist

• Documentation accurately reflects code — warning ＃2
• No content silently removed without justification — warning ＃1
• Cited commit hashes / CVEs verifiable
• Cross-references resolve — suggestion ＃3
• Threats vs. mitigations vs. open questions structure consistent — suggestion ＃4

---

Summary

Documentation-only PR extending the threat model with §5.4 Multipart parsing & encoding. The technical content is largely accurate — I spot-checked the cited commit hashes (5fe9dfb, 9cc4b91, dab9e87), the Request.post() nested-multipart rejection at web_request.py:760, the FormData.add_field CR/LF check, and the read_chunk base64 alignment loop, and they match the description. Two issues warrant attention before this lands:

1. The §5.3 WebSocket Open questions section is silently deleted by this diff, taking unresolved follow-up 3.1 (RFC 6455 mask-direction enforcement) with it — the document loses a live security follow-up.
2. Threat 4.9's Recommended action asks for an audit of base64 4-byte alignment in read_chunk, but that alignment is already implemented (multipart.py:347-370). The real residual concern is b64decode(..., validate=False) tolerance of stray bytes, not the chunk-boundary case.

The rest is style/consistency: dead anchors to §5.5–5.7 and §6.1 will need to follow shortly, and the threat 4.3 Recommended column should carry the max_nesting_depth proposal explicitly rather than parking it in the open question.

---

To rebase specific severity levels, mention me: @aiolibsbot rebase critical (fixes 🔴 only), @aiolibsbot rebase important (fixes 🔴 + 🟡), or just @aiolibsbot rebase for all.

---

Automated review by Kōan491bb4f
1b3f31c