Re-pin the threat model to b1de40b, absorb what merged, and trim it to what a triager needs

[pjfanning]

Motivation

Two things, plus a re-pin to current main, in separate commits so they can be reviewed separately; commits 4, 5 and 7 answer review, and commit 6 re-pins to the current main.

Content. Nineteen commits landed on main after the model was pinned to 6740cbd, ten of which touch a claim it makes. One changes a triage line: #1194 documents idle-timeout as a bidirectional inactivity timeout that a client sending bytes inside every window keeps alive by design, where §9 called such a connection an "evasion" and therefore in scope. Two others defend a property the model never stated: that a malformed request on one HTTP/2 stream is answered on that stream and leaves the connection alone (#1252, #1297).

Length. The document had grown to ~10,000 words, and the growth was narrative about how each claim came to be stated rather than the claims themselves. Its readers are triagers and the report-generating tools that cite it; they need the properties, limits, dispositions and non-findings, fast. The review history is real but belongs here, in the PR — which is where this description puts it.

Modification

Commit 1 — re-pin to 40b07a2 and absorb what merged:

Section	Change
P9 (new)	HTTP/2 stream isolation: a request that fails to parse gets a 400 on its own stream and the connection carries on (RequestErrorFlow, since #59). Two paths around it were found and closed during review — #1252 and #1297, below
P5 / §9 slow-loris	Restated around inactivity rather than evasion, citing reference.conf and timeouts.md (#1194). A trickling client is BY-DESIGN: property-disclaimed with a documented citation; the in-scope shape is a connection with no traffic outliving the timeout, or a bound whose cleanup leaks (#1284)
P1 verification	#1281 added beside #1259 as the third leaked discard path; #1266 as a bound that was missing outright
§9 smuggling	#1257 beside #1267
§5a	max-part-count = 10000, error-logging-verbosity = full
§6	Multipart row (there was none), noting #1279's header-state bleed between parts; IllegalRequestContext.rawRequestTarget row
§9 false friends	verbose-error-messages = off governs the client, not the log — what reaches the log is error-logging-verbosity
§10.9	Recommend error-logging-verbosity = simple where logs are shipped or alerted on
§11	Two misuse patterns: building Raw-Request-URI from request input (#1280 extended it to HTTP/2 :path); echoing rawRequestTarget unescaped from a custom ParsingErrorHandler
§15	Back-map rows to match

Commit 2 — trim to 6,500 words, no claim removed:

• The four "how Pn came to be stated" essays under the §8 table become one verification paragraph, one clause per defect with its PR.
• §14 retitled Maintainer rulings — nothing in it was open — and each answer cut to its ruling; reasoning kept only where it is the ruling (Q3's three tiers).
• §9's essays cut to the boundary they draw plus one PR reference per fixed case.
• Confidence paragraph → one sentence. §5b.4 tally, §12 near-miss narrative, §5 shutdown-hook caveat → one sentence each.
• Line-number citations dropped for file/symbol names: the pin fixes the commit, and the numbers rot every merge.

Commit 3 — re-pin to 478c58b and absorb what merged since 40b07a2:

Twenty commits landed after the 40b07a2 pin; two move a claim.

Section	Change
§5a / §6 / §15	max-frame-size = 512kB (#1264): an HTTP/2 frame over the limit is rejected on its frame header, before the payload is buffered; a limit on what is accepted, not advertised as SETTINGS_MAX_FRAME_SIZE
§8	#1264 added to the P1 verification paragraph; the "one P1 gap is open" paragraph removed
§11 / §15	Raw-Request-URI misuse restated (#1296): the header must be visible ASCII and a custom HttpMethod a token, checked at construction, so building either from request input no longer reaches request-line injection. What remains is client bytes choosing an unnormalized target; a value that passes construction yet corrupts the request line is VALID under §5b.4

No change from the rest: #1316's max-connection-age defaults to infinite; #1301 encodes HPACK literals as ISO-8859-1, which still substitutes ? above 0xFF, and the P2 guard checks the rendered bytes; #1305, #1298, #1318 and the dependency updates are not security-relevant.

Commit 4 — narrow claims the code does not back (from review):

Section	Change
P9 / §15	Scoped to a method, target, authority or header field that fails to parse into the model. Pseudo-header violations and connection-specific headers go through RequestParsing.protocolError → Http2ProtocolException → GOAWAY, and are named as not covered
P2	NUL claimed for headers and trailers only; RenderSupport.renderChunk drops chunk extensions on CR or LF, not NUL
P5	request-timeout starts once the entity is fully received (requestEnd), not when the request is delivered
§6	rawRequestTarget is the target as received, present whenever it was read; the failure may be a later header or the entity
§9 / §15	Logging claim cites UriParser.fail → DefaultParsingErrorHandler → logParsingError instead of ErrorInfo.scala

Commit 5 — §14 Q11 ruling: an HTTP/2 framing violation closing the violating peer's own connection with GOAWAY rather than RST_STREAM (RFC 9113 §8.1.1) is BY-DESIGN: property-disclaimed; moving to RST_STREAM is a compliance improvement, not a security fix. Matching §11a non-finding.

Commit 6 — re-pin to b1de40b: nine commits landed after 478c58b and none moves a claim. #1320 and #1323 add the client-only persistent-connection-max-age (default infinite) and reword a comment on the server's max-connection-age drain, which the model does not cite; the Http2Blueprint change only adds parentheses and keeps the stage order; the rest are syntax cleanups, release notes, build and dependency updates.

Commit 7 — P8 states the length disclosure (from review): secure_== returns early on a length mismatch, as its scaladoc documents, so verify does not hide whether the presented secret has the stored secret's length; verify(secret, hasher) over fixed-length digests removes even that. §14 Q5 adds that a length-only report is documented behaviour, not a P8 violation. The applied Client IP suggestion is tidied in the same commit.

#1344 makes renderChunk drop NUL in chunk extensions; once it merges, P2 can claim NUL for chunk extensions again.

The review history this document no longer carries

For the record — the model's claims were checked against the code as it was written, and the check kept finding defects. All are fixed at or before the pinned commit unless noted.

P1 — inbound messages are bounded. Asserted from the §5a table, then checked:

• max-chunk-count had no else branch, so failEntityStream's result was discarded and the parser trampolined into the next chunk — the limit emitted an error per remaining chunk instead of stopping (Stop parsing chunks once the chunk count limit is reached #1220).
• max-header-count was not incremented on the branch merging repeated Connection headers, so that header could be repeated without limit (fix: count repeated Connection headers towards max-header-count #1255).
• HTTP/2 totalBufferedData was never decremented when a buffered stream was discarded on RST_STREAM, downstream cancellation, or a stream-level FLOW_CONTROL_ERROR; once the leak passed half of incoming-connection-level-buffer-size the connection window drained to zero and every stream stalled (fix: release connection-level flow control accounting for discarded buffered data #1259, fix: release buffered data accounting when an incoming HTTP/2 stream is shut down #1281).
• Nothing bounded multipart part count; a body at max-content-length packed with minimal parts ran to well over a hundred thousand of them (feat: bound the number of parts in a multipart entity #1266).
• Http2FrameParsing took a frame of any declared length, up to 16 MiB − 1, before any §5a bound applied (feat: bound the size of an incoming HTTP/2 frame #1264, which adds max-frame-size). This was open at 40b07a2 and is closed at 478c58b.

P2 — response splitting is blocked. Asserted from the two illegal-response-header-*-processing-mode defaults, then checked: the guard lived in one ~~(HttpHeader) overload, so chunked-response trailers and chunk extensions rendered around it (#1256); it tested CR and LF but not NUL (#1260); the HTTP/2 header path had no equivalent check, so a mitigation relied on under HTTP/1.1 disappeared on upgrade (#1258). The rule now lives in one predicate, Rendering.isIllegalHeaderChar.

P9 — HTTP/2 stream isolation. A header that failed to parse unwound out of the HPACK decoder before the entry reached the dynamic table and before the rest of the block was read, desynchronising the table from the peer's encoder for every later HEADERS frame on the connection; an unknown method was enough (#1252, merged through #1251). A field the HTTP/1.1 header parser rejects — NUL in a value, an illegal character in a name, a value over max-header-value-length — was reported with that parser's internal exception type, which nothing on the HTTP/2 side caught, failing the connection instead of the stream; the same change closed the RFC 9113 §8.2.1 gap of a CR LF value being accepted silently truncated (#1297).

Q3 — path traversal. The draft asserted containment held. checkIsSafeDescendant compared canonical paths as strings, so a symlink resolving into /var/www-private passed for a root of /var/www (#1218). Reviewing the other directives found three tiers, not one: safeDirectoryChildPath (segment filter + containment) for the directory-serving directives; safeJoinPaths only for getFromResourceDirectory; nothing for getFromFile and getFromResource.

Q4, Q5, Q9 — corrections to the draft's facts. Q4: the draft said Pekko HTTP does not parse forwarding headers; extractClientIP does, and the point is that it never does so implicitly. Q5: the draft had constant-time comparison backwards; Credentials.verify does use secure_==. Q9: the draft said Pekko HTTP writes no files; the upload directives do. The dispositions survived in all three; the stated basis did not.

CORS enforcement. validateOrigins accepted a request if any origin matched while the response echoed every origin it was given (#1262). The document now distinguishes the permissive default (BY-DESIGN) from a restrictive setting that fails to restrict (VALID).

#1217 near-miss. Originally registered a JVM shutdown hook from Pekko HTTP itself, which would have falsified §5's "registers no shutdown hook"; reworked to a CoordinatedShutdown task before merging.

#1246 and the error path. Checking whether the default ParsingErrorHandler logs the new rawRequestTarget (it does not) surfaced that error-logging-verbosity = full has always written the failing request target into the log at warning level via UriParser.fail's error line — pre-existing, undocumented in the model, and the reason §5a, §9 and §10.9 gained their entries. #1294 escapes control characters in that line; once it lands the §9 false-friend narrows from "injection and flooding" to flooding.

Result

Every claim is verified against b1de40b, the current main. Provenance is 23 documented / 33 maintainer / 0 inferred at the content commit; the trim removes tags only on sentences that go with them, and commit 3 adds one documented tag (§11 Raw-Request-URI) commit 5 one maintainer tag (§14 Q11), and commit 7 one documented tag (§14 Q5).

#1264 has landed and is absorbed in commit 3. #1294 is not waited on: once it lands, the §9 verbose-error-messages false friend narrows from log injection and flooding to flooding alone, a one-line follow-up.

Tests

Not run - docs only

References

Refs #1194, #1246, #1251, #1252, #1257, #1264, #1266, #1279, #1280, #1281, #1284, #1296, #1297

[pjfanning]

If someone has time to review this, it would be useful to keep the security model up to date as we do get a regular stream of reports from security reporters, especially now that AI makes this much easier.

[samueleresca]

Taking a look at it this weekend

[Copilot]

🟡 Changes recommended

Several security properties overstate implementation guarantees or cite incomplete evidence.

6 open findings

• Avoid attributing all parse failures to the request target · New
• NUL filtering promise exceeds chunk extension implementation · New
• Scope request-timeout promise to fully received entities · New
• Narrow stream isolation claim to model-level parsing failures · New
• Cite the source chain proving raw target logging · New
• Scope P9 back-map to model-level field parsing failures · New

What changed in this PR

Re-pins and streamlines the security threat model while incorporating recently merged protections and maintainer rulings.

Changes:

• Updates the model to commit 478c58b.
• Adds HTTP/2, multipart, timeout, and request-target security guidance.
• Condenses historical analysis into triage-focused rules.

File	Description
THREAT_MODEL.md	Updates and shortens the project threat model.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[samueleresca]

Looks good overall. I left few nits.

[samueleresca]

small note here from AI scan: " The length leak is documented in the code but absent from P8."

It is referring to: https://github.com/apache/pekko-http/blame/main/http-core/src/main/scala/org/apache/pekko/http/impl/util/EnhancedByteArray.scala#L31-L35

Theoretically, an attacker could infer the length of a stored secret as the length checks early exit. In practice, I'm not sure how this would be feasible. Specially considering a Pekko HTTP service behind a proxy, it's a long shot.

So I'm pointing this out, but I'm also happy to leave as-is.

[pjfanning]

Good catch. The leak is real and documented on secure_==: a length mismatch returns before the XOR loop. I agree it is hard to exploit, but the model should say so rather than leave it implied. In b0ca9dc, P8 now states that whether the presented secret matches the stored secret's length is not hidden, and that verify(secret, hasher) over fixed-length digests removes even that. §14 Q5 adds that a length-only report describes documented behaviour, not a P8 violation, so a report of this shape has a disposition.