Reject CR/LF in the request target and fail the request cleanly#2501
Merged
Conversation
split.py strips `inline` and moves the definition into httplib.cc, so detail::write_request_line is not visible from the split httplib.h and test_split failed to compile. Re-declare it in test.cc, matching what the base64_encode and getaddrinfo_with_timeout tests already do.
The CR/LF guard in write_request_line returns -1, but ClientImpl::write_request ignored that return value. A rejected target therefore produced a request-line- less request (headers only) that the client silently reported as a successful send. This is the primary path reachable via a decoded redirect Location under set_path_encode(false), and it also carries the CONNECT target. Check the return value like ClientImpl::open_stream already does and fail with Error::Write. Add an end-to-end test asserting the client refuses a CR/LF target instead of putting it on the wire.
Owner
Author
|
@metsw24-max this is another PR that revised your PR #2495. I fixed an issue that the return code from |
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
2 participants
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.
Supersedes #2495 (thanks @metsw24-max — the first two commits are theirs, unchanged).
What the original PR does
The client builds each request line in
write_request_lineby concatenating the method, the target, and" HTTP/1.1\r\n", with no check on the target. A target carrying CR/LF splits the request line and injects a header or a whole request. The neighbouring writers already guard this (check_and_write_headersover header values,perform_websocket_handshakeover the path), but the ordinary request path was left out.It becomes reachable when following a redirect with path encoding disabled:
ClientImpl::redirectdecodes theLocationpath throughdecode_path_component, so a wire-safe%0D%0Aturns into a raw CR/LF, and underset_path_encode(false)that decoded target reacheswrite_request_lineverbatim. The guard rejects a target that failsis_field_value, matching the other two writers.What this PR adds on top
write_request_linenow returns-1on a rejected target, butClientImpl::write_requestignored that return value. A rejected target was silently dropped, producing a request-line-less (headers-only) request — and the client reported it as a successful send (res.error() == Success). This is the primary path (all normal requests and theCONNECTtarget flow throughwrite_request), i.e. exactly the path the original PR is about.write_requestand fail withError::Write, matchingClientImpl::open_stream, which already checks it.RequestLineInjectionTest.ClientRejectsCRLFTargetEndToEnd: withset_path_encode(false),cli.Get("/a\r\nInjected: pwned")must returnfalsewithError::Writerather than reaching the wire. Verified to fail without the call-site fix.Note: the guard blocks CR/LF (the injection vector).
is_field_valuestill permits interior SP/HTAB, consistent with the other two writers; normal encoding prevents raw spaces in targets.Tested: full suite green (734 tests) in both header-only and split builds.