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