From 076def3c5a8f746c3d0bfeba77ce25d4d906542c Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Tue, 1 Sep 2026 20:38:28 +0100 Subject: [PATCH] fix: drop HTTP/2 header fields containing CR, LF or NUL (#1258) Motivation: Outgoing HTTP/2 header fields were HPACK-encoded without checking for CR, LF or NUL in the name or value. The HTTP/1.1 renderer scans each rendered header for CR/LF and drops it, but the HTTP/2 path had no equivalent, so an attacker-influenced value (a RawHeader, CustomHeader or a response trailer built from user data) was encoded verbatim. RFC 9113 8.2.1 forbids these characters in a field name or value. On a native HTTP/2 leg HPACK is length-prefixed so this is not direct frame splitting, but it violates the spec and enables HTTP/2 -> HTTP/1.1 downgrade smuggling when an intermediary re-serialises the message, and it means the mitigation an application relies on under HTTP/1.1 silently disappears under HTTP/2. Modification: In `HeaderCompression`, the single point every outgoing header field (regular headers, trailers and pseudo-headers all arrive here as key/value pairs) passes through before HPACK encoding, drop any field whose name or value contains CR, LF or NUL, logging at debug. Debug rather than warning because the value can be attacker-influenced, so a warning would be a log-flooding vector, and the HTTP/1.1 renderer drops silently too. Result: CR/LF/NUL in an HTTP/2 header name or value can no longer reach the wire; the offending field is dropped, matching the HTTP/1.1 renderer, whether it rides in the header block or a response trailer. Tests: - sbt "http2-tests/testOnly org.apache.pekko.http.impl.engine.http2.Http2ServerSpec" - pass (125 tests); two new tests assert a CRLF-bearing response header and a CRLF-bearing response trailer header are dropped while a valid sibling trailer survives. Verified both fail with the fix stashed (the injected set-cookie reaches the decoded headers). - sbt http-core/mimaReportBinaryIssues - pass (internal impl.engine.http2 change, no public API). References: None - aligns HTTP/2 header rendering with the HTTP/1.1 CR/LF guard (RFC 9113 8.2.1) --- .../http2/hpack/HeaderCompression.scala | 14 ++++++++- .../impl/engine/http2/Http2ServerSpec.scala | 30 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala index 2738a8f09a..4d8fac117f 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderCompression.scala @@ -35,6 +35,10 @@ private[http2] object HeaderCompression extends GraphStage[FlowShape[FrameEvent, val shape = FlowShape(eventsIn, eventsOut) + /** RFC 9113 8.2.1: NUL, CR and LF are never valid in an HTTP/2 field name or value. */ + private[http2] def hasIllegalChar(s: String): Boolean = + s.indexOf('\r') >= 0 || s.indexOf('\n') >= 0 || s.indexOf('\u0000') >= 0 + def createLogic(inheritedAttributes: Attributes): GraphStageLogic = new GraphStageLogic(shape) with StageLogging with InHandler with OutHandler { logic => setHandlers(eventsIn, eventsOut, this) @@ -55,7 +59,15 @@ private[http2] object HeaderCompression extends GraphStage[FlowShape[FrameEvent, else { kvs.foreach { case (key, value: String) => - encoder.encodeHeader(os, key, value, false) + // RFC 9113 8.2.1: a field name or value carrying a NUL, CR or LF is malformed and must not be sent. + // The value can be attacker-controlled (a RawHeader, CustomHeader or a response trailer), so drop the + // offending field instead of encoding it, mirroring the HTTP/1.1 renderer's CR/LF guard. + if (HeaderCompression.hasIllegalChar(key) || HeaderCompression.hasIllegalChar(value)) + // debug, not warning: the value can be attacker-influenced (reflected into a header), so a warning + // here would be a log-flooding vector; the HTTP/1.1 renderer drops such headers silently too + log.debug("Dropping HTTP/2 header [{}] because its name or value contains a CR, LF or NUL", key) + else + encoder.encodeHeader(os, key, value, false) case (key, value) => throw new IllegalStateException( s"Didn't expect key-value-pair [$key] -> [$value](${value.getClass}) here.") diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerSpec.scala index 95892435e0..534e0ff63a 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerSpec.scala @@ -319,6 +319,36 @@ class Http2ServerSpec extends Http2SpecWithMaterializer(""" trailingResponseHeaders.size should be(1) trailingResponseHeaders.head should be(("Status", "grpc-status 10")) }) + "drop a response header whose value contains CRLF".inAssertAllStagesStopped( + new TestSetup with RequestResponseProbes { + val streamId = 1 + network.sendHEADERS(streamId, endStream = true, network.headersForRequest(Get("/"))) + user.expectRequest() + user.emitResponse(streamId, + HttpResponse(StatusCodes.OK, headers = RawHeader("x-trace", "ok\r\nset-cookie: injected=1") :: Nil)) + + val responseHeaders = network.expectDecodedResponseHEADERSPairs(streamId) + responseHeaders.map(_._1) should not contain "x-trace" + responseHeaders.exists { case (_, v) => v.contains("injected") } should be(false) + }) + "drop a response trailer header whose value contains CRLF".inAssertAllStagesStopped( + new TestSetup with RequestResponseProbes { + val streamId = 1 + network.sendHEADERS(streamId, endStream = true, network.headersForRequest(Get("/"))) + user.expectRequest() + val response = + HttpResponse(StatusCodes.OK, + entity = HttpEntity.Strict(ContentTypes.`application/octet-stream`, ByteString("Hello"))) + .addAttribute(AttributeKeys.trailer, + Trailer(Vector(RawHeader("x-evil", "ok\r\nset-cookie: injected=1"), RawHeader("x-good", "fine")))) + user.emitResponse(streamId, response) + + network.expectHeaderBlock(streamId, endStream = false) + network.expectDATA(streamId, endStream = false, ByteString("Hello")) + val trailingResponseHeaders = network.expectDecodedResponseHEADERSPairs(streamId) + trailingResponseHeaders should contain(("x-good", "fine")) + trailingResponseHeaders.map(_._1) should not contain "x-evil" + }) "consider stream as closed after sending out strict response > WINDOW_SIZE".inAssertAllStagesStopped( new TestSetup with RequestResponseProbes { override def settings: ServerSettings =