Re: [PR] Re-pin the threat model to b1de40b, absorb what merged, and trim it to what a triager needs [pekko-http]

["samueleresca (via GitHub)" <gi...@apache.org>]

samueleresca commented on code in PR #1295:
URL: https://github.com/apache/pekko-http/pull/1295#discussion_r4237646032


##########
THREAT_MODEL.md:
##########
@@ -47,65 +42,63 @@ Caller roles:
 | Benchmarks | `http-bench-jmh` | — | **no** — §3 |
 | Lint / build / docs | `http-scalafix`, `docs`, `project`, `scripts`, `legal` | — | **no** — §3 |
 
-*(maintainer — the in/out split is confirmed by the Pekko maintainers; see §14 Q6)*
+*(maintainer — §14 Q6)*
 
 ---
 
 ## §3 Out of scope (explicit non-goals)
 
 - **Test kits, benchmarks, scalafix rules, build tooling and documentation sources.** A finding in `http-bench-jmh` or any `*-tests` module is `OUT-OF-MODEL: unsupported-component`. *(maintainer — §14 Q6)*
-- **Pekko HTTP is not a WAF, and not an edge-hardened server.** The documentation says so plainly: applications *"should not be exposed to the public internet directly"* and an *"enterprise grade routing solution"* or a load balancer such as Apache HTTP Server or Nginx *"would be safer"* *(documented — `security.md`)*. See §4.
-- **Pekko HTTP is not an authentication or authorization system.** It ships `authenticateBasic`, `authenticateOAuth2` and `authorize` directives, but these are *plumbing*: the credential check is a function the application supplies. Pekko HTTP has no user store, no session model, and no policy engine. *(maintainer — §14 Q7)*
-- **The actor, stream, remoting and cluster layers** are out of scope here and covered by `apache/pekko`'s threat model.
-- **Attackers who already control the embedding process** are out of scope. *(maintainer — §14 Q7)*
+- **Pekko HTTP is not a WAF, and not an edge-hardened server.** Applications *"should not be exposed to the public internet directly"*; an *"enterprise grade routing solution"* or a load balancer such as Apache HTTP Server or Nginx *"would be safer"* *(documented — `security.md`)*. See §4.
+- **Pekko HTTP is not an authentication or authorization system.** `authenticateBasic`, `authenticateOAuth2` and `authorize` are plumbing: the credential check is a function the application supplies. There is no user store, session model or policy engine. *(maintainer — §14 Q7)*
+- **The actor, stream, remoting and cluster layers** are covered by `apache/pekko`'s threat model.
+- **Attackers who already control the embedding process.** *(maintainer — §14 Q7)*
 
 ---
 
 ## §4 Trust boundaries and data flow
 
 **The trust boundary is the inbound HTTP request.** Everything derived from wire bytes — request line, headers, cookies, entity, HTTP/2 frames — is attacker-controlled until the application validates it.
 
-Pekko HTTP's documented posture is unusual and important enough to quote in full:
+Pekko HTTP's documented posture:
 
 > "Pekko HTTP-based applications should not be exposed to the public internet directly. We believe Pekko HTTP behaves pretty well under most known Denial of Service attacks, but if you want the best security, you should use an enterprise grade routing solution. Even using a load balancing solution like an up-to-date version of Apache HTTP Server or Nginx would be safer than exposing Pekko HTTP-based applications directly to the public internet."
 > — *(documented — `security.md`, "Security model")*
 
-Read carefully, this makes a **graded** claim rather than a binary one: Pekko HTTP asserts it *"behaves pretty well under most known"* DoS attacks — not that it is DoS-proof, and not that DoS is out of scope. That hedge was the single most consequential ambiguity in this model for triage; §14 Q1 has now resolved it into the content-vs-volume line, which is the rule triage actually applies.
+This is a graded claim, not a disclaimer of DoS as a class. §14 Q1 resolves it into the content-vs-volume line that triage applies.
 
 ### Reachability preconditions per family
 
-- **Core protocol / HTTP/2** — reachable from raw wire bytes. The strongest in-model surface: a parser defect here needs no application cooperation.
-- **Routing DSL** — reachable from a request that the application's own routes expose. A finding must name the directive and show a route shape a reasonable application would write.
+- **Core protocol / HTTP/2** — reachable from raw wire bytes. A parser defect here needs no application cooperation.
+- **Routing DSL** — reachable from a request the application's own routes expose. A finding must name the directive and show a route shape a reasonable application would write.
 - **Marshalling** — reachable from an entity body **only where the application has bound that marshaller to a route**. Findings in the underlying JSON/XML library belong to that library.
-- **CORS** — reachable only where the application has installed the `cors()` directive. It is opt-in, not on by default. See §5a.
+- **CORS** — reachable only where the application has installed `cors()`. Opt-in, not on by default.
 - **Caching** — reachable only where the application has installed a caching directive; cache-key correctness is the sharp edge.
 
 ---
 
 ## §5 Assumptions about the environment
 
 - **Runtime.** A conformant JVM. Pekko HTTP does not defend against a hostile JVM or in-process attacker. *(maintainer — §14 Q7)*
-- **Fronting infrastructure.** The documented expectation is that something sits in front in production *(documented — `security.md`)*. Per §14 Q1 this is load-bearing for *volume* only: the proxy is relied on for flood and slow-loris defence, not for bounding a single request, which is P1's job.
-- **TLS.** Pekko HTTP can terminate TLS itself (`HttpsConnectionContext`) and this is supported, though the documented recommendation to front the service (§4) means termination is commonly the proxy's job in production. Cipher and protocol selection come from the JSSE context the application supplies — Pekko HTTP pins nothing and overrides no JDK default, so cipher strength is a deployment property. *(maintainer — §14 Q8)*
-- **Client IP.** `remote-address-attribute` ships `off` *(documented — `reference.conf`)*. When on, the attribute reflects the **socket** peer, which behind a proxy is the proxy; forwarding headers never feed it. Header-derived client IP is opt-in at the call site: `extractClientIP` reads `X-Forwarded-For` / `X-Real-Ip` and is therefore client-controllable, while `extractDirectClientIP` reads the attribute alone and is not. Choosing between them is the application's decision (§14 Q4). *(documented — `MiscDirectives.scala`)*
+- **Fronting infrastructure.** Something sits in front in production *(documented — `security.md`)*. Per §14 Q1 this is load-bearing for *volume* only: the proxy is relied on for flood and slow-loris defence, not for bounding a single request, which is P1's job.
+- **TLS.** Pekko HTTP can terminate TLS itself (`HttpsConnectionContext`); in a fronted deployment termination is commonly the proxy's job. Cipher and protocol selection come from the JSSE context the application supplies — Pekko HTTP pins nothing and overrides no JDK default. *(maintainer — §14 Q8)*
+- **Client IP.** `remote-address-attribute` ships `off` *(documented — `reference.conf`)*. When on, the attribute is the **socket** peer — behind a proxy, the proxy. `extractClientIP` reads `X-Forwarded-For` / `X-Real-Ip` and is client-controllable; `extractDirectClientIP` reads the attribute alone and is not (§14 Q4). *(documented — `MiscDirectives.scala`)*

Review Comment:
   nit
   ```suggestion
   - **Client IP.** `remote-address-attribute` ships `off` *(documented — `reference.conf`)*. When on, the attribute holds the address of whoever opened the TCP connection to Pekko HTTP. With a load balancer or reverse proxy in front, that's the proxy's address. 
     - `extractClientIP` reads `X-Forwarded-For` / `X-Real-Ip` and is **client-controllable**;  
     - `extractDirectClientIP` reads the attribute alone and is not (§14 Q4). *(documented —`MiscDirectives.scala`)*
   ```



##########
THREAT_MODEL.md:
##########
@@ -199,65 +195,63 @@ Pekko HTTP therefore takes the following position *(maintainer)*:
 
 **In scope:**
 
-- **The remote HTTP client.** Can send arbitrary bytes, malformed framing, oversized or deeply-nested input, many concurrent connections, and abusive HTTP/2 frame sequences. The primary adversary — though per §14 Q1 what this adversary achieves through sheer *volume* is the proxy's problem, not the library's. *(maintainer — §14 Q7)*
+- **The remote HTTP client.** Arbitrary bytes, malformed framing, oversized or deeply-nested input, many concurrent connections, abusive HTTP/2 frame sequences. The primary adversary — though what it achieves through sheer *volume* is the proxy's problem (§14 Q1). *(maintainer — §14 Q7)*
 - **A malicious upstream server**, where the application uses the client API against an untrusted endpoint. *(maintainer — §14 Q7)*
 - **A cross-origin web attacker**, where the application enables CORS. *(maintainer — §14 Q2)*
 
 **Explicitly out of scope:**
 
-- **Attackers with code execution in the embedding JVM.** Already inside.
+- **Attackers with code execution in the embedding JVM.**
 - **The embedding application itself.** A route that deliberately leaks data is an application bug.
-- **Side-channel observers.** No general timing guarantees are made about routing or parsing. Credential comparison is the exception: `Credentials.Provided.verify` compares in constant time (see §8 P8), so a timing finding there is in scope, while one against an application-supplied `provideVerify` is not. *(documented — `SecurityDirectives.scala`, `EnhancedByteArray.scala`)*
+- **Side-channel observers.** No general timing guarantees are made about routing or parsing. Credential comparison is the exception (P8): a timing finding against `Credentials.verify` is in scope; one against an application-supplied `provideVerify` is not. *(documented — `SecurityDirectives.scala`, `EnhancedByteArray.scala`)*
 
 ---
 
 ## §8 Security properties Pekko HTTP provides
 
 | # | Property & conditions | Violation symptom | Severity | Provenance |
 | --- | --- | --- | --- | --- |
-| P1 | **Inbound messages are bounded** by the §5a limits: reaching a limit stops the parse rather than merely reporting it, every header a message carries counts towards the count limits, and buffered bytes are released from the accounting when the buffer is discarded | OOM or unbounded buffering from input *within* documented limits; a limit reached without the parse stopping | **Critical** | *(documented — `reference.conf`, `HttpMessageParser.scala`, `Http2StreamHandling.scala`)* |
-| P2 | **Response splitting is blocked**: illegal response header names and values are `error` by default, and the renderers drop any header whose rendered bytes contain CR, LF or NUL — on the HTTP/1.1 main header block, on chunked-response trailers and chunk extensions, and on the HTTP/2 HPACK path alike | CRLF or NUL in an application-supplied header, trailer or chunk extension reaching the wire | **Critical** | *(documented — `reference.conf`, `Rendering.scala:270`)* |
+| P1 | **Inbound messages are bounded** by the §5a limits: reaching a limit stops the parse, every header a message carries counts towards the count limits, and buffered bytes are released from the accounting when the buffer is discarded | OOM or unbounded buffering from input *within* documented limits; a limit reached without the parse stopping | **Critical** | *(documented — `reference.conf`, `HttpMessageParser.scala`, `Http2FrameParsing.scala`, `Http2StreamHandling.scala`)* |
+| P2 | **Response splitting is blocked**: illegal response header names and values are `error` by default, and the renderers drop any header whose rendered bytes contain CR, LF or NUL — on the HTTP/1.1 header block, chunked-response trailers and the HTTP/2 HPACK path alike — and any chunk extension containing CR or LF | CR or LF in an application-supplied header, trailer or chunk extension, or NUL in a header or trailer, reaching the wire | **Critical** | *(documented — `reference.conf`, `Rendering.isIllegalHeaderChar`, `RenderSupport.renderChunk`)* |
 | P3 | **Parse errors do not leak detail to the client** — `verbose-error-messages = off` | Internal parse state or stack detail in a 400 response under defaults | High | *(documented — `reference.conf`)* |
 | P4 | **Strict URI and RFC6265 cookie parsing by default**, rather than lenient normalization that invites smuggling | Two components disagreeing on a URI or cookie under `strict` | High | *(documented — `reference.conf`)* |
-| P5 | **Connection and request lifetimes are bounded** — `idle-timeout 60s`, `request-timeout 20s`, `max-connections 1024`, `pipelining-limit 1` | A client holding resources indefinitely under defaults | High | *(documented — `reference.conf`)* |
-| P6 | **HTTP/2 concurrency and stream churn are bounded** — `max-concurrent-streams = 256` caps streams open at once, and `frame-type-throttle` rate-limits the frames that let a peer cycle through that cap: `RST_STREAM` is charged by default at 100 per second with a burst of 100. Concurrency alone is not a bound on *work*, because a reset frees the slot immediately — this is the HTTP/2 Rapid Reset shape (CVE-2023-44487) | Unbounded stream/state growth on one connection, or unbounded stream churn through a bounded concurrency limit | High | *(documented — `reference.conf`)* |
-| P7 | **HTTP/2 header blocks are bounded** — `max-header-list-size = 64 KiB` caps the decompressed header list *and* the accumulated HEADERS + CONTINUATION fragments, so a header block the peer never terminates with `END_HEADERS` cannot grow without bound; over-limit blocks get `GOAWAY(ENHANCE_YOUR_CALM)` rather than being buffered | Unbounded buffering from a CONTINUATION flood or an oversized header list | **Critical** | *(documented — `reference.conf`)* |
-| P8 | **Credential comparison is constant-time** where the verifier calls `Credentials.verify` — it compares via `secure_==`, which XOR-accumulates over the full length after a length check, rather than short-circuiting on the first differing byte | Secret recoverable byte-by-byte from response timing against a `verify`-based verifier | High | *(documented — `SecurityDirectives.scala`, `EnhancedByteArray.scala:37`)* |
-
-**P1-P7 are default-on properties** — a notable contrast with `apache/pekko`, where the strongest controls must be switched on. P8 is the exception: it holds only for a verifier that calls `Credentials.verify`, which is why §10.4 states it as a downstream responsibility. The boundary of the DoS claim, once this model's largest ambiguity, is now fixed by the §14 Q1 content-vs-volume line.
-
-**A note on how P1 came to be stated this precisely.** P1 has the same history as P2, and for the same reason: the draft asserted it from the §5a table, which records what each limit is *supposed* to bound. Three of those bounds did not hold as written, all fixed at the pinned commit. `max-chunk-count` was tested without an `else` branch, so the `StateResult` of `failEntityStream` was discarded and the parser trampolined into the next chunk — reaching the limit emitted an error per remaining chunk in the buffer instead of stopping the parse ([#1220](https://github.com/apache/pekko-http/pull/1220)). `max-header-count` was not incremented on the branch that merges repeated `Connection` headers into one accumulated header, so a message could repeat that header without ever reaching the limit ([#1255](https://github.com/apache/pekko-http/pull/1255)). And on the HTTP/2 side the connection-level buffer accounting only ever went up: `totalBufferedData` was not decremented when a buffere
 d stream was discarded on `RST_STREAM` or on downstream cancellation, so a peer that sent data it never let the handler read ratcheted the counter up permanently — once the leak passed half of `incoming-connection-level-buffer-size` the server stopped replenishing the connection window, it drained to zero, and every stream on that connection stalled ([#1259](https://github.com/apache/pekko-http/pull/1259)). P1 as stated above is a claim about what the parser and the frame handler do when a bound is reached, not about what the table says the bound is for. *(maintainer)*
+| P5 | **Connection and request lifetimes are bounded** — `idle-timeout 60s` closes a connection with no traffic in *either* direction for that long; `request-timeout 20s` bounds handling once a request's entity has been fully received (a body still arriving is not covered; §9); `max-connections 1024`; `pipelining-limit 1`. `idle-timeout` is an inactivity timeout, not a receive deadline (§9) | A connection with no traffic outliving `idle-timeout`; a fully received request outliving `request-timeout` with no response; resources held past either bound after the connection is gone | High | *(documented — `reference.conf`, `timeouts.md`)* |
+| P6 | **HTTP/2 concurrency and stream churn are bounded** — `max-concurrent-streams = 256` caps open streams, and `frame-type-throttle` rate-limits `RST_STREAM` (100/s, burst 100) so a peer cannot cycle through that cap for free (Rapid Reset, CVE-2023-44487) | Unbounded stream/state growth on one connection, or unbounded churn through a bounded concurrency limit | High | *(documented — `reference.conf`)* |
+| P7 | **HTTP/2 header blocks are bounded** — `max-header-list-size = 64 KiB` caps the decompressed header list *and* the accumulated HEADERS + CONTINUATION fragments; over-limit blocks get `GOAWAY(ENHANCE_YOUR_CALM)` rather than being buffered | Unbounded buffering from a CONTINUATION flood or an oversized header list | **Critical** | *(documented — `reference.conf`)* |
+| P8 | **Credential comparison is constant-time** where the verifier calls `Credentials.verify` — `secure_==` XOR-accumulates over the full length after a length check | Secret recoverable byte-by-byte from response timing against a `verify`-based verifier | High | *(documented — `SecurityDirectives.scala`, `EnhancedByteArray.scala`)* |

Review Comment:
   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.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org