Conversation
… request target Motivation: `UriParser.fail` and `HeaderParser.parseError` build the `detail` of their `ErrorInfo` from parboiled2's `formatErrorLine`, which reproduces the failing input line verbatim under a caret. That input is the raw request target or header value as the peer sent it, and the shipped `error-logging-verbosity = full` writes the detail to the log at warning level, so a client could put a CR, ESC or NUL into a request target and have it land raw in the log: a forged log line, or a terminal control sequence for whoever tails it. The summary was already safe, because parboiled2 escapes the offending character there; only the line was not. apache#1246 handed the same raw request target to custom `ParsingErrorHandler`s through `IllegalRequestContext`. Its scaladoc said the value must be escaped, but `toString` did not, the `error-handler` comment in `reference.conf` still said the handler had no access to the request, and no documentation page mentioned the handler at all. Modification: Add `ParseErrorLine.render`, which renders the failing line the way `formatErrorLine` does but escapes control characters as `\t`, `\r`, `\n` and `\uXXXX`, keeping the caret aligned under the escaped text. The EOI sentinel U+FFFF is deliberately left alone, since `HeaderParser` appends it to every value and strips it from the error afterwards. Use it in both parsers. Escape the raw request target in `IllegalRequestContext.toString` so that logging the context is safe, and say so in the scaladoc; add the warning to the Java accessor. Bring the `error-handler` comment up to date and note at `error-logging-verbosity` what `full` logs. Add a "Requests that fail to parse" section to the low-level server docs describing the handler, the context and the escaping requirement. Result: A parse error's detail carries no raw control characters, so a client can no longer inject into the log through a request target or header value. The raw request target is documented as attacker-controlled everywhere a user meets it, and the one rendering the library provides is escaped. Tests: - New `ParseErrorLineSpec` covers caret placement, end-of-input, escaping and caret alignment past an escaped character. - `UriSpec`, `HeaderSpec` and `RequestParserSpec` each gain a case sending an ESC or NUL and asserting the escaped detail, and the request-parser case asserts the escaped `toString`. All three fail with the change to the parsers and `toString` reverted. - sbt "http-core/testOnly ...ParseErrorLineSpec ...UriSpec ...HeaderSpec ...RequestParserCRLFSpec ...RequestParserLFSpec" - 197 tests pass. - sbt "http-core/mimaReportBinaryIssues" - pass. - sbt "docs/paradox" - pass; the new apidoc links resolve. References: Refs apache#1246, apache#1245
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
UriParser.failandHeaderParser.parseErrorbuild thedetailof theirErrorInfofrom parboiled2'sformatErrorLine, which reproduces the failing input line verbatim under a caret:That input is the raw request target or header value as the peer sent it, and the shipped
error-logging-verbosity = fullwrites the detail to the log at warning level. So a client can put a CR, ESC or NUL into a request target and have it land raw in the server log — a forged log line, or a terminal control sequence for whoever tails it. The summary was already safe, because parboiled2 escapes the offending character there (Invalid input '\u001b'); only the line under it was not.#1246 handed the same raw request target to custom
ParsingErrorHandlers throughIllegalRequestContext. Its scaladoc said the value must be escaped, buttoStringdid not, theerror-handlercomment inreference.confstill said the handler had no access to the request, and no documentation page mentioned the handler at all.Modification
Code
ParseErrorLine.render, which renders the failing line the wayformatErrorLinedoes but escapes control characters as\t,\r,\nand\uXXXX, keeping the caret aligned under the escaped text. The EOI sentinel U+FFFF is deliberately left alone:HeaderParserappends it to every value and strips it from the error afterwards, which only works if it is still that character. Used in both parsers.IllegalRequestContext.toString, so that the naturallog.warning(s"... $context")is safe.Docs
IllegalRequestContextscaladoc: spell out what the raw target can carry and what goes wrong if it is logged or echoed raw; note thattoStringis escaped and the field is not. Same warning on the Java accessorgetRawRequestTargetand on the five-argumenthandle.reference.conf: theerror-handlercomment now describes the context the handler receives and the escaping requirement;error-logging-verbositynow says whatfulllogs.Result
A parse error's detail carries no raw control characters, so a client can no longer inject into the log through a request target or header value. The raw request target is documented as attacker-controlled everywhere a user meets it, and the one rendering the library provides is escaped.
Before, for a request line of
GET /<NUL>HTTP/1.1 HTTP/1.1, the logged detail was the line with a literal NUL byte in it; now it is:Tests
ParseErrorLineSpeccovers caret placement, end-of-input, escaping, caret alignment past an escaped character, and multi-line input.UriSpec,HeaderSpecandRequestParserSpeceach gain a case sending an ESC or NUL and asserting the escaped detail; the request-parser case also asserts the escapedtoString. All three fail with the parser wiring andtoStringchange reverted.sbt "http-core/testOnly ...ParseErrorLineSpec ...UriSpec ...HeaderSpec ...RequestParserCRLFSpec ...RequestParserLFSpec"— 197 tests pass.sbt "http-core/mimaReportBinaryIssues"— pass.sbt "docs/paradox"— pass; the new@apidoclinks resolve.References
Refs #1246, #1245