fix: escape control characters in parse-error details and document the raw request target

docs/src/main/paradox/server-side/low-level-api.md

@@ -230,6 +230,35 @@ Note that this is when the TCP connection is closed correctly, if the client jus
 a network failure, it will not be seen as this kind of stream failure. It will instead be detected through the
 @ref[idle timeout](../common/timeouts.md#timeouts)).
 
+#### Requests that fail to parse
+
+A request can be so malformed that no @apidoc[HttpRequest] is ever created from it: an unknown method, a request target
+that is not a URI, a header the parser cannot read. Such a request never reaches your handler or any
+@ref[exception handler](../routing-dsl/exception-handling.md), because there is no request to hand over. Instead the
+server responds through a @apidoc[ParsingErrorHandler], selected by the `pekko.http.server.parsing.error-handler`
+setting. The default implementation logs the parse error and answers with the status code the parser chose, carrying
+only a short summary of the error unless `pekko.http.server.verbose-error-messages` is on.
+
+A custom handler can override the five-argument `handle` to receive an @apidoc[IllegalRequestContext] alongside the
+error: what is known about the request at the point where parsing gave up — its method, its raw request target and its
+protocol, each optional because a request can be rejected before that part of it has been read.
+
+@@@ warning
+
+`rawRequestTarget` is unvalidated, attacker-controlled input. It is by definition malformed whenever the request target
+is what failed to parse, and it can carry any byte the client chose to send — CR, LF, NUL, terminal control sequences.
+Written raw into a log it lets a client forge log lines or drive the terminal that displays them; written raw into a
+response body it is reflected input. A handler that logs or echoes it has to escape it first. The context's `toString`
+is escaped and safe to log as it is; the field itself is not.
+
+@@@
+
+The default handler never reads the raw request target, so nothing is echoed to the client under the default
+configuration. It does log the parse error, and with the default `pekko.http.server.parsing.error-logging-verbosity = full`
+the logged details include the input that failed to parse, with control characters escaped. Consider `simple` where logs
+are shipped or alerted on, since the input is attacker-chosen text of up to the configured length limit written at
+warning level.
+
 
 These failures can be described more or less infrastructure related, they are failing bindings or connections.
 Most of the time you won't need to dive into those very deeply, as Apache Pekko will simply log errors of this kind

http-core/src/main/resources/reference.conf

@@ -208,9 +208,15 @@ pekko.http {
 
       # When a request is so malformed we cannot create a RequestContext out of it,
       # the regular exception handling does not apply, and a default error handling
-      # is applied that only has access to the parse error and not the actual request.
+      # is applied that has access to the parse error and to what little is known
+      # about the request (its method, raw request target and protocol, each of which
+      # may be absent), but not to a parsed request.
       # To customize this error response, set error-handler to the FQCN of an
-      # implementation of org.apache.pekko.http.ParsingErrorHandler
+      # implementation of org.apache.pekko.http.ParsingErrorHandler.
+      #
+      # A custom handler that logs or echoes the raw request target is handling
+      # unvalidated, attacker-controlled input - it is by definition malformed whenever
+      # the request target is what failed to parse - and has to escape it first.
       error-handler = "org.apache.pekko.http.DefaultParsingErrorHandler$"
     }
 
@@ -851,6 +857,13 @@ pekko.http {
     # `off`   : no log messages are produced
     # `simple`: a condensed single-line message is logged
     # `full`  : the full error details (potentially spanning several lines) are logged
+    #
+    # With `full`, the details include the input that failed to parse - the raw request
+    # target or header value, as the peer sent it, with a marker under the offending
+    # position. Control characters in that input are escaped before it is logged, so a
+    # peer cannot forge log lines with it, but it is still attacker-chosen text of up to
+    # the configured length limit written at warning level; consider `simple` where logs
+    # are shipped or alerted on.
     error-logging-verbosity = full
 
     # Configures the processing mode when encountering illegal characters in

http-core/src/main/scala/org/apache/pekko/http/ParsingErrorHandler.scala

@@ -17,6 +17,8 @@ import java.util.Optional
 
 import scala.jdk.OptionConverters._
 
+import org.parboiled2.CharUtils
+
 import org.apache.pekko
 import pekko.event.LoggingAdapter
 import pekko.http.javadsl.{ model => jm }
@@ -31,8 +33,11 @@ import pekko.http.scaladsl.settings.ServerSettings
  * unparsable request target fails before the protocol is seen.
  *
  * Note that `rawRequestTarget` is unvalidated, attacker-controlled input, by definition malformed
- * whenever the rejection was caused by the request target itself. Anything that logs or echoes it
- * has to escape it.
+ * whenever the rejection was caused by the request target itself. It can carry any byte the client
+ * chose to send, including CR, LF, NUL and terminal control sequences, so anything that logs or
+ * echoes it has to escape it first: written raw into a log it lets a client forge log lines or drive
+ * the terminal that displays them, and written raw into a response body it is reflected input.
+ * `toString` escapes it and is safe to log as it is; the field itself is not.
  *
  * @since 2.0.0
  */
@@ -51,6 +56,9 @@ final class IllegalRequestContext private[http] (
   /**
    * Java API
    *
+   * The value is unvalidated, attacker-controlled input and has to be escaped before it is logged
+   * or echoed; see the class documentation.
+   *
    * @since 2.0.0
    */
   def getRawRequestTarget: Optional[String] = rawRequestTarget.toJava
@@ -62,9 +70,11 @@ final class IllegalRequestContext private[http] (
    */
   def getProtocol: Optional[jm.HttpProtocol] = protocol.map(p => p: jm.HttpProtocol).toJava
 
+  // the raw request target is escaped so that logging the context does not write client-chosen control
+  // characters into the log; see the class documentation
   override def toString: String =
     s"IllegalRequestContext(${method.map(_.value).getOrElse("-")}," +
-    s"${rawRequestTarget.getOrElse("-")},${protocol.map(_.value).getOrElse("-")})"
+    s"${rawRequestTarget.map(t => CharUtils.escape(t)).getOrElse("-")},${protocol.map(_.value).getOrElse("-")})"
 }
 
 object IllegalRequestContext {
@@ -111,6 +121,10 @@ abstract class ParsingErrorHandler {
    * Note that `DefaultParsingErrorHandler` deliberately keeps implementing the four-argument method
    * rather than this one, so that advice matching that signature keeps firing.
    *
+   * A handler that reads `context.rawRequestTarget` is handling unvalidated, attacker-controlled
+   * input, and must escape it before writing it to a log or into the response; see
+   * [[IllegalRequestContext]].
+   *
    * @since 2.0.0
    */
   def handle(status: StatusCode, error: ErrorInfo, log: LoggingAdapter, settings: ServerSettings,

http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/HeaderParser.scala

@@ -81,7 +81,7 @@ private[http] class HeaderParser(
   def success(result: HttpHeader :: HNil): Result = HeaderParser.Success(result.head)
   def parseError(error: ParseError): HeaderParser.Failure = {
     val formatter = new ErrorFormatter(showLine = false)
-    HeaderParser.Failure(ErrorInfo(formatter.format(error, input), formatter.formatErrorLine(error, input)))
+    HeaderParser.Failure(ErrorInfo(formatter.format(error, input), ParseErrorLine.render(error, input)))
   }
   def failure(error: Throwable): HeaderParser.Failure =
     HeaderParser.Failure {

http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/ParseErrorLine.scala

@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.http.impl.model.parser
+
+import org.apache.pekko
+import pekko.annotation.InternalApi
+import org.parboiled2.{ ParseError, ParserInput }
+
+/**
+ * INTERNAL API
+ *
+ * Renders the input line a parse error occurred on with a caret under the failing position, the way parboiled2's
+ * `ErrorFormatter.formatErrorLine` does, but with every control character escaped the way `ErrorFormatter.format`
+ * already escapes the offending character in its summary.
+ *
+ * The line is attacker-controlled -- a request target, a header value -- and the default
+ * `error-logging-verbosity = full` writes it to the log, so a raw CR, ESC or NUL in it would let a client inject
+ * line breaks or terminal control sequences into log output.
+ */
+@InternalApi
+private[http] object ParseErrorLine {
+
+  def render(error: ParseError, input: ParserInput): String = {
+    val line = input.getLine(error.position.line)
+    val column = error.position.column // 1-based; one past the end of the line for an error at end of input
+    val sb = new java.lang.StringBuilder(line.length + 16)
+    var caret = 0
+    var i = 0
+    while (i < line.length) {
+      val escaped = escape(line.charAt(i))
+      if (i < column - 1) caret += escaped.length
+      sb.append(escaped)
+      i += 1
+    }
+    sb.append('\n')
+    var j = 0
+    while (j < caret) {
+      sb.append(' ')
+      j += 1
+    }
+    sb.append('^').toString
+  }
+
+  // the same escapes `org.parboiled2.CharUtils.escape` applies to the offending character in the summary, except that
+  // the EOI sentinel (U+FFFF, which is not a control character) is left alone: `HeaderParser` appends it to every
+  // header value it parses and strips it from the error afterwards, which only works if it is still that character
+  private def escape(c: Char): String = c match {
+    case '\t'                           => "\\t"
+    case '\r'                           => "\\r"
+    case '\n'                           => "\\n"
+    case c if Character.isISOControl(c) => "\\u%04x".format(c.toInt)
+    case c                              => c.toString
+  }
+}

http-core/src/main/scala/org/apache/pekko/http/impl/model/parser/UriParser.scala

@@ -116,7 +116,7 @@ private[http] final class UriParser(
 
   def fail(error: ParseError, target: String): Nothing = {
     val formatter = new ErrorFormatter(showLine = false)
-    Uri.fail(s"Illegal $target: " + formatter.format(error, input), formatter.formatErrorLine(error, input))
+    Uri.fail(s"Illegal $target: " + formatter.format(error, input), ParseErrorLine.render(error, input))
   }
 
   private val `path-segment-char` = uriParsingMode match {

http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala

@@ -685,8 +685,13 @@ abstract class RequestParserSpec(mode: String, newLine: String) extends AnyFreeS
         val result = multiParse(newParser)(Seq("GET /\u0000HTTP/1.1 HTTP/1.1\r\n"))
         result.length shouldEqual 1
         result.head match {
-          case Left(MessageStartError(BadRequest, info, _)) =>
+          case Left(MessageStartError(BadRequest, info, context)) =>
             info.summary should startWith("Illegal request-target")
+            // the error details are logged under the default `error-logging-verbosity = full`, and the context's
+            // toString is documented as safe to log: neither may carry the client's NUL byte through raw
+            info.detail shouldEqual "/\\u0000HTTP/1.1\n ^"
+            context.rawRequestTarget shouldEqual Some("/\u0000HTTP/1.1")
+            context.toString shouldEqual "IllegalRequestContext(GET,/\\u0000HTTP/1.1,-)"
           case other => fail(s"Expected BadRequest MessageStartError but got $other")
         }
       }

http-core/src/test/scala/org/apache/pekko/http/impl/model/parser/HttpHeaderSpec.scala

@@ -907,7 +907,8 @@ class HttpHeaderSpec extends AnyFreeSpec with Matchers {
     "not accept illegal header values" in {
       parse("Foo", "ba\u0000r") shouldEqual ParsingResult.Error(ErrorInfo(
         "Illegal HTTP header value: Invalid input '\\u0000', expected field-value-char, FWS or 'EOI' (line 1, column 3)",
-        "ba\u0000r\n  ^"))
+        // the detail is logged under the default `error-logging-verbosity = full`, so the NUL is escaped in it too
+        "ba\\u0000r\n  ^"))
     }
     "allow UTF8 characters in RawHeaders" in {
       parse("Flood-Resistant-Hammerdrill", "árvíztűrő ütvefúrógép") shouldEqual

http-core/src/test/scala/org/apache/pekko/http/impl/model/parser/ParseErrorLineSpec.scala

@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.http.impl.model.parser
+
+import org.parboiled2.{ ParseError, ParserInput, Position }
+import org.scalatest.matchers.should.Matchers
+import org.scalatest.wordspec.AnyWordSpec
+
+class ParseErrorLineSpec extends AnyWordSpec with Matchers {
+
+  def errorAt(input: String, index: Int): (ParseError, ParserInput) = {
+    val in = ParserInput(input)
+    val pos = Position(index, in)
+    (ParseError(pos, pos, Vector.empty), in)
+  }
+
+  "ParseErrorLine" should {
+    "render the failing line with a caret under the failing position" in {
+      val (error, input) = errorAt("a^=b", 1)
+      ParseErrorLine.render(error, input) shouldEqual "a^=b\n ^"
+    }
+
+    "put the caret after the last character for an error at end of input" in {
+      val (error, input) = errorAt("abc", 3)
+      ParseErrorLine.render(error, input) shouldEqual "abc\n   ^"
+    }
+
+    "escape control characters instead of rendering them raw" in {
+      val (error, input) = errorAt("/a\u001b[31m", 2)
+      ParseErrorLine.render(error, input) shouldEqual "/a\\u001b[31m\n  ^"
+    }
+
+    "escape CR, tab and NUL with their short forms" in {
+      val (error, input) = errorAt("a\r\t\u0000", 1)
+      ParseErrorLine.render(error, input) shouldEqual "a\\r\\t\\u0000\n ^"
+    }
+
+    "keep the caret aligned when an escaped character precedes the failing position" in {
+      // the tab renders as two characters, so the caret moves one column right of where it would sit on the raw line
+      val (error, input) = errorAt("a\tb^c", 3)
+      ParseErrorLine.render(error, input) shouldEqual "a\\tb^c\n    ^"
+    }
+
+    "render only the line the error is on" in {
+      val (error, input) = errorAt("first\nsec^ond", 9)
+      ParseErrorLine.render(error, input) shouldEqual "sec^ond\n   ^"
+    }
+  }
+}

http-core/src/test/scala/org/apache/pekko/http/scaladsl/model/UriSpec.scala

@@ -211,6 +211,15 @@ class UriSpec extends AnyWordSpec with Matchers {
       // Nonhex
       a[IllegalUriException] should be thrownBy Host("[g:0:0:0:0:0:0]")
     }
+
+    "escape control characters in the error line of a parse failure" in {
+      // the error line is logged under the default `error-logging-verbosity = full`, so a raw ESC or NUL in it would
+      // let a client inject terminal control sequences or line breaks into log output
+      val error = the[IllegalUriException] thrownBy Uri("/a\u001b[31mb")
+      error.info.summary should startWith("Illegal URI reference: Invalid input '\\u001b'")
+      error.info.detail shouldEqual "/a\\u001b[31mb\n  ^"
+      (error.info.detail should not).include("\u001b")
+    }
   }
 
   "Uri.Path instances" should {

http-core/src/test/scala/org/apache/pekko/http/scaladsl/model/headers/HeaderSpec.scala

@@ -99,6 +99,12 @@ class HeaderSpec extends AnyFreeSpec with Matchers {
         summary3 shouldEqual
         "Illegal HTTP header 'Retry-After': Invalid input '-', expected DIGIT, OWS or 'EOI' (line 1, column 5)"
       }
+      "escape control characters in the error line" in {
+        // the error line is logged under the default `error-logging-verbosity = full`
+        val Left(List(ErrorInfo(summary, detail))) = `Retry-After`.parseFromValueString("12\u001b[31m")
+        summary should startWith("Illegal HTTP header value: Invalid input '\\u001b'")
+        detail shouldEqual "12\\u001b[31m\n  ^"
+      }
     }
   }