This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pekko-http.git
The following commit(s) were added to refs/heads/main by this push:
new 265a6ceba fix: reject control bytes in the request line and malformed
HTTP/2 :scheme and field names (#1341)
265a6ceba is described below
commit 265a6cebae5d0b4409f257ed72723539e1c2da71
Author: PJ Fanning <[email protected]>
AuthorDate: Tue Oct 13 19:26:32 2026 +0100
fix: reject control bytes in the request line and malformed HTTP/2 :scheme
and field names (#1341)
* test: cover control characters around the request-line version token
Motivation:
netty/netty#16970 was a request-smuggling bug in which control bytes around
the HTTP
version token were stripped before matching. #1088 covered a NUL byte in
the request
target, but nothing covered control bytes next to the version, in the
method, or the
other control characters #1087 lists.
Modification:
Add RequestParserSpec cases, run in both the CRLF and LF variants, for:
- NUL, VT, FF and bare CR immediately before and after HTTP/1.1 (505)
- NUL before and inside the method (501)
- VT and FF in the request target (400)
Inputs bypass prep so the CR cases keep their exact bytes.
Result:
A future parser change that trims control bytes around the version token
fails these
tests.
Tests:
- sbt "http-core/testOnly *RequestParserCRLFSpec *RequestParserLFSpec": 138
passed
- With HttpRequestParser temporarily changed to skip NUL/VT/FF/bare CR
around the
version, "a control character before or after the HTTP version" fails
- sbt http-core/Test/scalafmtCheck: pass
References:
Fixes #1087
* fix: reject request-line and HTTP/2 field bytes that re-parse differently
Motivation:
Probing the request-line parser for #1087 found that a CR or LF ending the
request
target was skipped as if it were the SP separator, so `GET /<LF>HTTP/1.1`
was accepted
as one request line where another hop may see a line without a version. The
HTTP/2
equivalents - control bytes in pseudo-header and field values - turned up
two more:
- an invalid ':scheme' (a control character or whitespace) made Uri.apply
throw an
IllegalUriException out of the request parsing stage, failing the whole
connection
instead of the one stream
- a regular field name containing ':' was handed to the HTTP/1.1 line
parser as
`name: value` and split at the colon, so `x:a: v` became a field `x`
carrying `a: v`
Modification:
- HttpRequestParser: a CR or LF ending the request target is answered with
505, as a
request line without a version already was.
- Http2HeaderParsing.Scheme: validate the value during decompression, as
':path' and
':authority' are, and report a malformed one as a ParsingException. An
empty value
is still accepted because Pekko HTTP/2 clients before #1342 sent one for a
relative URI.
- HeaderDecompression: reject a non-pseudo field name containing ':' (RFC
9113 8.2.1).
Result:
These requests are rejected with a 400/505 for the affected request or
stream; an
invalid ':scheme' no longer tears down every stream on the connection.
Tests:
- Without the main changes the new RequestParserSpec CR/LF test and the
RequestParsingSpec
':' name and ':scheme' tests fail
- sbt http-core/test: 1331 passed
- sbt http2-tests/test: 390 passed
- sbt http-core/mimaReportBinaryIssues and scalafmt checks: pass
References:
Refs #1087
---
.../engine/http2/hpack/HeaderDecompression.scala | 4 +++
.../engine/http2/hpack/Http2HeaderParsing.scala | 12 ++++++-
.../impl/engine/parsing/HttpRequestParser.scala | 4 +++
.../impl/engine/parsing/RequestParserSpec.scala | 40 ++++++++++++++++++++++
.../impl/engine/http2/RequestParsingSpec.scala | 20 +++++++++++
5 files changed, 79 insertions(+), 1 deletion(-)
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 408ae35a8..91df746eb 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
@@ -121,6 +121,10 @@ private[http2] final class
HeaderDecompression(masterHeaderParser: HttpHeaderPar
case "cookie" => handle(Cookie.parse(name, value,
parserSettings))
case x if x(0) == ':' => handle(value)
case _ =>
+ // RFC 9113 8.2.1: only a pseudo-header name may contain a
colon. The HTTP/1.1 line parser below would
+ // otherwise split `x:a` at it and read a field named `x`
carrying `a: <value>`.
+ if (name.indexOf(':') >= 0)
+ throw new ParsingException(ErrorInfo("Malformed request:
header field name must not contain ':'"))
// cannot use OtherHeader.parse because that doesn't has
access to header parser
val header = parseHeaderPair(httpHeaderParser, name, value)
RequestParsing.validateHeader(header)
diff --git
a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala
b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala
index 7b2066065..7294216a7 100644
---
a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala
+++
b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/Http2HeaderParsing.scala
@@ -32,7 +32,17 @@ private[pekko] object Http2HeaderParsing {
override def parse(name: String, value: String, parserSettings:
ParserSettings): String = value
}
- object Scheme extends Verbatim(":scheme")
+ object Scheme extends HeaderParser[String](":scheme") {
+ // validated here, like ':path' and ':authority', so that a malformed
value fails only its own stream: left to
+ // `Uri.apply` in RequestParsing, the IllegalUriException escapes the
request parsing stage and with it the
+ // whole connection fails. An empty value is let through, since Pekko HTTP
clients before #1342 sent one for a
+ // request with a relative URI.
+ override def parse(name: String, value: String, parserSettings:
ParserSettings): String =
+ try Uri.normalizeScheme(value)
+ catch {
+ case IllegalUriException(info) => throw new ParsingException(info)
+ }
+ }
object Method extends HeaderParser[HttpMethod](":method") {
override def parse(name: String, value: String, parserSettings:
ParserSettings): HttpMethod =
HttpMethods.getForKey(value)
diff --git
a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpRequestParser.scala
b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpRequestParser.scala
index e43fe3954..2b30dc13e 100644
---
a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpRequestParser.scala
+++
b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpRequestParser.scala
@@ -184,6 +184,10 @@ private[http] final class HttpRequestParser(
s"URI length exceeds the configured limit of $maxUriLength
characters$remoteAddressStr")
val uriEnd = findUriEnd()
+ // A CR or LF here ends the request line before any HTTP version.
Skipping it as if it were the SP separator
+ // would read `GET /<LF>HTTP/1.1` as one request line where another
hop may see a line without a version.
+ val separator = input(uriEnd)
+ if (separator == CR_BYTE || separator == LF_BYTE)
onBadProtocol(input.drop(uriEnd))
try {
uriBytes = input.slice(uriStart, uriEnd)
uriParser.reset(new ByteStringParserInput(uriBytes))
diff --git
a/http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala
b/http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala
index 9e85e2342..478fe399c 100644
---
a/http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala
+++
b/http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala
@@ -691,6 +691,34 @@ abstract class RequestParserSpec(mode: String, newLine:
String) extends AnyFreeS
}
}
+ // A parser that strips control bytes around the version token before
matching it can be made to agree with
+ // a request line that another hop rejects
(https://github.com/netty/netty/issues/16970). The inputs below are
+ // not passed through `prep`, so that the CR cases keep their exact
bytes in both line-ending variants.
+ "a control character before or after the HTTP version" in new Test {
+ for {
+ control <- Seq("\u0000", "\u000b", "\u000c", "\r")
+ requestLine <- Seq(s"GET / ${control}HTTP/1.1", s"GET /
HTTP/1.1$control")
+ } rejectRawRequestLine(s"$requestLine\r\nHost: x\r\n\r\n",
HttpVersionNotSupported,
+ "The server does not support the HTTP protocol version")
+ }
+
+ "a CR or LF between the request target and the HTTP version" in new Test
{
+ // either ends the request line before the version, so it must not be
skipped as if it were the SP separator
+ for (separator <- Seq("\r", "\n"))
+ rejectRawRequestLine(s"GET /${separator}HTTP/1.1\r\nHost:
x\r\n\r\n", HttpVersionNotSupported,
+ "The server does not support the HTTP protocol version")
+ }
+
+ "a NUL byte before or in the HTTP method" in new Test {
+ for (requestLine <- Seq("\u0000GET / HTTP/1.1", "G\u0000ET /
HTTP/1.1"))
+ rejectRawRequestLine(s"$requestLine\r\nHost: x\r\n\r\n",
NotImplemented, "Unsupported HTTP method")
+ }
+
+ "a VT or FF byte in the request target" in new Test {
+ for (control <- Seq("\u000b", "\u000c"))
+ rejectRawRequestLine(s"GET /$control HTTP/1.1\r\nHost: x\r\n\r\n",
BadRequest, "Illegal request-target")
+ }
+
"with an illegal char in a header name" in new Test {
"""GET / HTTP/1.1
|User@Agent: curl/7.19.7""" should parseToError(BadRequest,
ErrorInfo("Illegal character '@' in header name"))
@@ -930,6 +958,18 @@ abstract class RequestParserSpec(mode: String, newLine:
String) extends AnyFreeS
def rawMultiParseTo(parser: HttpRequestParser, expected: HttpRequest*):
Matcher[Seq[String]] =
generalRawMultiParseTo(parser, expected.map(Right(_)): _*)
+ def rejectRawRequestLine(input: String, status: StatusCode, summaryPrefix:
String): Unit = {
+ val result = multiParse(newParser)(Seq(input))
+ result.length shouldEqual 1
+ result.head match {
+ case Left(MessageStartError(`status`, info, _)) => info.summary should
startWith(summaryPrefix)
+ case other =>
+ fail(s"Expected a $status MessageStartError for ${input.flatMap(c =>
+ if (c < 0x20) f"\\u${c.toInt}%04x" else c.toString)}" +
+ s" but got $other")
+ }
+ }
+
def parseToError(status: StatusCode, info: ErrorInfo): Matcher[String] =
generalMultiParseTo(Left(MessageStartError(status, info))).compose(_ ::
Nil)
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 1af04d359..63fc77c6d 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
@@ -158,6 +158,26 @@ class RequestParsingSpec extends PekkoSpecWithMaterializer
with Inside with Insp
parseExpectError(request("x-a" -> value)).summary should ===(
"header field value must not start or end with whitespace")
}
+ "a header value containing another control character" in {
+ for (control <- Seq("\u0001", "\u000b", "\u000c")) {
+ val info = parseExpectError(request("x-a" -> s"foo${control}bar"))
+ info.summary should include("Illegal character")
+ }
+ }
+ "a header name containing ':'" in {
+ // the HTTP/1.1 line parser this is handed to splits at the colon, so
without the check the request was
+ // accepted with a field named `x` carrying `a: v`
+ val info = parseExpectError(request("x:a" -> "v"))
+ info.summary should include("header field name must not contain ':'")
+ }
+ "a ':scheme' containing a control character or whitespace" in {
+ // without validating it during decompression, `Uri.apply` threw an
IllegalUriException that escaped the
+ // request parsing stage and failed the whole connection
+ for (scheme <- Seq("ht\u000btp", "ht\u000ctp", "ht\ttp", "ht tp",
"http\u0001")) {
+ val info = parseExpectError(Vector(":method" -> "GET", ":scheme" ->
scheme, ":path" -> "/"))
+ info.summary should include("Invalid URI scheme")
+ }
+ }
"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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
