From 2a594a0f6810ced86e178333970eedd4f34dc119 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Fri, 11 Sep 2026 12:31:31 +0100 Subject: [PATCH] http/2: reject a header field carrying CR, LF or NUL, and answer a malformed field with a 400 Motivation: Two things go wrong when an HTTP/2 peer sends a malformed header field. A regular field whose value contains CR LF is silently accepted with the value truncated. `RequestParsing.parseHeaderPair` reuses the HTTP/1.1 line parser by building `name + ": " + value + "\r\nx"`, so the parser stops at the first CRLF it meets, which is now the peer's, and the header that comes back is whatever preceded it. RFC 9113 section 8.2.1 says a field name or value carrying NUL, CR or LF must be treated as malformed. A field the HTTP/1.1 parser does reject -- a NUL in the value, an illegal character in the name, a value over `max-header-value-length` -- is reported with that parser's own, internal `ParsingException`, which is not the model `ParsingException` that `HeaderDecompression` catches. It escapes the decompression stage and fails it, and with it the whole connection and every stream on it, where the HTTP/2 engine answers other malformed fields with a 400 on the one stream (#59). On a connection a proxy multiplexes for many users, one bad header from one of them takes the connection down for all of them. Modification: Check the name and the value for CR, LF and NUL at the top of the HPACK listener, before the field is dispatched on its name, reusing the predicate the rendering side uses. Neither is echoed in the error, since either may be what is malformed. Widen the internal `ParsingException` from `private[parsing]` to `private[http]` and rethrow it from `parseHeaderPair` as the model exception, so that every failure the HTTP/1.1 parser reports for an HTTP/2 field takes the 400 path. Result: A header field carrying CR, LF or NUL is answered with a 400 instead of being accepted truncated, and a field the HTTP/1.1 parser rejects is answered with a 400 on its own stream instead of failing the connection. Tests: - `RequestParsingSpec`: CR LF, bare LF and NUL in a value, CR LF in a name, and a value over `max-header-value-length` each produce a `BadRequest` with the expected summary. - `Http2ServerSpec`: the CR LF, NUL and over-long cases each get a 400 on their stream and the next stream on the same connection is served. - With both source changes reverted all eight fail; with only the CR/LF/ NUL check in place the two over-long cases still fail, so the exception translation is covered on its own. - sbt "http2-tests/test" - 374 pass. - sbt "http-core/mimaReportBinaryIssues" - pass. - native scalafmt clean. References: Refs #59 --- .../impl/engine/http2/RequestParsing.scala | 11 +++++- .../http2/hpack/HeaderDecompression.scala | 10 +++++ .../http/impl/engine/parsing/package.scala | 2 +- .../impl/engine/http2/Http2ServerSpec.scala | 39 +++++++++++++++++++ .../engine/http2/RequestParsingSpec.scala | 33 ++++++++++++++++ 5 files changed, 92 insertions(+), 3 deletions(-) diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala index 386873947..af019fbfd 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala @@ -218,8 +218,15 @@ private[http2] object RequestParsing { // The odd-looking 'x' below is a by-product of how current parser and HTTP/1.1 work. // Without '\r\n\x' (x being any additional byte) parsing will fail. See HttpHeaderParserSpec for examples. val concHeaderLine = name + ": " + value + "\r\nx" - httpHeaderParser.parseHeaderLine(ByteString(concHeaderLine))() - httpHeaderParser.resultHeader + try { + httpHeaderParser.parseHeaderLine(ByteString(concHeaderLine))() + httpHeaderParser.resultHeader + } catch { + // the HTTP/1.1 parser reports a malformed field with its own, internal exception type, which nothing on the + // HTTP/2 side catches: left alone it fails the decompression stage and with it the whole connection. Rethrow + // it as the model exception `HeaderDecompression` turns into a 400 for the one stream. + case e: pekko.http.impl.engine.parsing.ParsingException => throw new ParsingException(e.info) + } } private[http2] def checkRequiredPseudoHeader(name: String, value: AnyRef): Unit = diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala index d51dd85e8..4cd22a15a 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala @@ -81,6 +81,16 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar var parsingError: Option[ErrorInfo] = None object Receiver extends HeaderListener { def addHeader(name: String, value: String, parsed: AnyRef, sensitive: Boolean): AnyRef = try { + // RFC 9113 8.2.1: a field name or value carrying a NUL, CR or LF makes the message malformed. Check it + // here, before the field is dispatched on its name: a regular field goes through the HTTP/1.1 line + // parser, which reads up to the first CRLF it finds and would silently accept the value truncated + // there. Neither the name nor the value is echoed, since either may be what is malformed. + if (HeaderCompression.hasIllegalChar(name)) + throw new ParsingException( + ErrorInfo("Malformed request: header field name must not contain CR, LF or NUL")) + if (HeaderCompression.hasIllegalChar(value)) + throw new ParsingException( + ErrorInfo("Malformed request: header field value must not contain CR, LF or NUL")) if (parsed ne null) { headers += name -> parsed parsed diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/package.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/package.scala index 92b2036fb..9ac295cf2 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/package.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/package.scala @@ -70,7 +70,7 @@ package parsing { * INTERNAL API */ @InternalApi - private[parsing] class ParsingException( + private[http] class ParsingException( val status: StatusCode, val info: ErrorInfo) extends RuntimeException(info.formatPretty) { def this(status: StatusCode, summary: String) = 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 662078d44..9ced005fb 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 @@ -57,6 +57,45 @@ class Http2ServerSpec extends Http2SpecWithMaterializer(""" override def failOnSevereMessages: Boolean = true "The Http/2 server implementation" should { + "answer a malformed header field with a 400 on its stream and keep the connection" should { + abstract class MalformedHeaderSetup extends TestSetup with RequestResponseProbes { + def badRequestThenStillUsable(streamId: Int, headerPairs: Seq[(String, String)]): Unit = { + // the 400 is produced where the parsed request would otherwise be handed to the handler, so the handler + // has to be asking for one + user.requestIn.request(1) + network.sendHEADERS(streamId, endStream = true, endHeaders = true, network.encodeHeaderPairs(headerPairs)) + network.expectDecodedResponseHEADERSPairs(streamId, endStream = false).toMap should contain( + ":status" -> "400") + network.expectDATAFrame(streamId) + + // the connection is still open and serving: the next stream gets through to the handler + val nextStreamId = streamId + 2 + network.sendRequest(nextStreamId, + HttpRequest(HttpMethods.GET, "https://www.example.com/", protocol = HttpProtocols.`HTTP/2.0`)) + user.expectRequest() + user.emitResponse(nextStreamId, HttpResponse()) + network.expectDecodedResponseHEADERSPairs(nextStreamId).toMap should contain(":status" -> "200") + } + def request(extra: (String, String)*): Seq[(String, String)] = + Seq(":method" -> "GET", ":scheme" -> "https", ":path" -> "/", ":authority" -> "www.example.com") ++ extra + } + + "for a value containing CR LF".inAssertAllStagesStopped(new MalformedHeaderSetup { + badRequestThenStillUsable(1, request("x-a" -> "foo\r\nx-b: bar")) + }) + "for a value containing NUL".inAssertAllStagesStopped(new MalformedHeaderSetup { + // before the fix this failed the decompression stage and took the whole connection down + badRequestThenStillUsable(1, request("x-a" -> "foo\u0000bar")) + }) + "for a value longer than max-header-value-length".inAssertAllStagesStopped(new MalformedHeaderSetup { + override def settings: ServerSettings = { + val s = super.settings + s.withParserSettings(s.parserSettings.withMaxHeaderValueLength(16)) + } + badRequestThenStillUsable(1, request("x-a" -> ("v" * 17))) + }) + } + "support simple round-trips" should { abstract class SimpleRequestResponseRoundtripSetup extends TestSetup with RequestResponseProbes { def requestResponseRoundtrip( diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala index e972a87e1..65067d8c6 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/RequestParsingSpec.scala @@ -109,6 +109,39 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer with Inside with Insp futureValueEx.getCause.asInstanceOf[Http2ProtocolException] } + "reject a malformed header field with a bad request rather than accepting or failing the connection" should { + // RFC 9113 8.2.1: a field name or value carrying NUL, CR or LF makes the message malformed + def request(extra: (String, String)*): Vector[(String, String)] = + Vector(":method" -> "GET", ":scheme" -> "https", ":path" -> "/") ++ extra + + "a header value containing CR LF" in { + // the HTTP/1.1 line parser this is handed to stops at the first CRLF it finds, so without the check the + // request was accepted with the value silently truncated to `foo` + val info = parseExpectError(request("x-a" -> "foo\r\nx-b: bar")) + info.summary should include("header field value must not contain CR, LF or NUL") + } + "a header value containing a bare LF" in { + val info = parseExpectError(request("x-a" -> "foo\nbar")) + info.summary should include("header field value must not contain CR, LF or NUL") + } + "a header value containing NUL" in { + val info = parseExpectError(request("x-a" -> "foo\u0000bar")) + info.summary should include("header field value must not contain CR, LF or NUL") + } + "a header name containing CR LF" in { + val info = parseExpectError(request("x-a\r\nx-b" -> "v")) + info.summary should include("header field name must not contain CR, LF or NUL") + } + "a header value longer than max-header-value-length" in { + // the HTTP/1.1 parser reports this with its own, internal exception type, which used to escape the + // decompression stage and fail the whole connection instead of answering the one stream + val settings = ServerSettings(system) + val small = settings.withParserSettings(settings.parserSettings.withMaxHeaderValueLength(16)) + val info = parseExpectError(request("x-a" -> ("v" * 17)), settings = small) + info.summary should include("HTTP header value exceeds the configured limit of 16 characters") + } + } + "follow RFC7540" should { // 8.1.2.1. Pseudo-Header Fields