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 2738a8f09..4d8fac117 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 95892435e..534e0ff63 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 =