http/2: reject a header field carrying CR, LF or NUL, and answer a malformed field with a 400

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 =

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

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) =

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(

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