Skip to content

reject crlf in the request target in write_request_line#2495

Closed
metsw24-max wants to merge 2 commits into
yhirose:masterfrom
metsw24-max:request-line-crlf-guard
Closed

reject crlf in the request target in write_request_line#2495
metsw24-max wants to merge 2 commits into
yhirose:masterfrom
metsw24-max:request-line-crlf-guard

Conversation

@metsw24-max

Copy link
Copy Markdown
Contributor

the client builds each request line in write_request_line by concatenating the method, the target and " HTTP/1.1\r\n" and writing it to the socket with no check on the target, so 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_headers runs is_field_value over every header value, and perform_websocket_handshake runs it over the path before the identical "GET " + path build, but the ordinary request path was left out. it becomes reachable when following a redirect with path encoding disabled: ClientImpl::redirect decodes the Location path through decode_path_component, so a wire-safe %0D%0A turns into a raw CR/LF, and under set_path_encode(false) that decoded target reaches write_request_line verbatim (the default encode_path re-encodes it, so only the disabled case leaks). the same writer also emits the CONNECT target, so guarding it once covers every request line the client sends. the change rejects a target that fails is_field_value there, matching the other two writers; a valid target is always field-value-clean, so i don't think anything legitimate is refused. added RequestLineInjectionTest.RejectsCRLFInTarget.

@yhirose

yhirose commented Jul 13, 2026

Copy link
Copy Markdown
Owner

@metsw24-max could you please take a look at CI errors?

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.
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Sorry about that. The failure was test_split, not the guard itself: split.py strips inline and moves the definition into httplib.cc, so detail::write_request_line isn't visible from the split httplib.h and test.cc failed to compile against it.

Pushed a fix that re-declares it in test.cc, the same way the base64_encode and getaddrinfo_with_timeout tests already handle it. httplib.h is unchanged. make test_split builds clean now and the offline suite is 684/684.

@yhirose

yhirose commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Thanks for this @metsw24-max — the fix is correct and I've carried it forward in #2501 with your two commits unchanged, so your authorship is preserved.

While reviewing I found the guard needs one more thing to fully take effect: write_request_line now returns -1, but ClientImpl::write_request (the primary path for all normal requests and the CONNECT target) ignored that return value. A rejected target was silently dropped, producing a headers-only request that the client reported as a successful send. #2501 checks the return value there (matching ClientImpl::open_stream) and adds an end-to-end test.

Closing in favor of #2501. Thank you again for finding and reporting this.

@yhirose yhirose closed this Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants