skip header fields containing CR/LF in http serialization#3387
Open
ubeddulla wants to merge 2 commits into
Open
skip header fields containing CR/LF in http serialization#3387ubeddulla wants to merge 2 commits into
ubeddulla wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens outbound HTTP serialization in brpc to prevent HTTP request/response splitting by ensuring headers containing raw CR/LF bytes are not written onto the wire.
Changes:
- Added a helper (
HeaderHasCRLF) and used it to skip header fields whose name/value contains\ror\nduring request/response serialization. - Applied the same CR/LF check to outbound
Content-Typeserialization. - Added a unit test to validate CR/LF injection is not emitted in serialized HTTP messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/brpc_http_message_unittest.cpp | Adds a regression test ensuring CR/LF in header fields cannot inject additional serialized headers. |
| src/brpc/details/http_message.cpp | Drops outbound header fields (and Content-Type) containing CR/LF to prevent HTTP splitting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
657
to
660
| if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) { | ||
| os << "Content-Type: " << h->content_type() | ||
| << BRPC_CRLF; | ||
| } |
Comment on lines
751
to
755
| if (!is_invalid_content && !h->content_type().empty() && | ||
| !HeaderHasCRLF(h->content_type())) { | ||
| os << "Content-Type: " << h->content_type() | ||
| << BRPC_CRLF; | ||
| } |
Comment on lines
+663
to
+667
| if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { | ||
| LOG(ERROR) << "Skip header `" << it->first | ||
| << "' containing CR/LF to avoid injection"; | ||
| continue; | ||
| } |
Comment on lines
+758
to
+762
| if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { | ||
| LOG(ERROR) << "Skip header `" << it->first | ||
| << "' containing CR/LF to avoid injection"; | ||
| continue; | ||
| } |
Comment on lines
+706
to
+729
| TEST(HttpMessageTest, serialize_header_with_crlf_is_not_injected) { | ||
| // A header value carrying CR/LF must not terminate the current line and | ||
| // introduce extra header fields (HTTP request/response splitting). | ||
| butil::EndPoint ep; | ||
| ASSERT_EQ(0, butil::str2endpoint("127.0.0.1:1234", &ep)); | ||
| butil::IOBuf content; | ||
| content.append("data"); | ||
|
|
||
| brpc::HttpHeader req_header; | ||
| req_header.set_method(brpc::HTTP_METHOD_POST); | ||
| req_header.SetHeader("X-Evil", "a\r\nInjected: 1"); | ||
| butil::IOBuf request; | ||
| MakeRawHttpRequest(&request, &req_header, ep, &content); | ||
| std::string request_str = request.to_string(); | ||
| ASSERT_EQ(std::string::npos, request_str.find("Injected: 1")) << request_str; | ||
|
|
||
| brpc::HttpHeader res_header; | ||
| res_header.SetHeader("X-Evil", "a\r\nInjected: 1"); | ||
| butil::IOBuf response; | ||
| content.append("data"); | ||
| MakeRawHttpResponse(&response, &res_header, &content); | ||
| std::string response_str = response.to_string(); | ||
| ASSERT_EQ(std::string::npos, response_str.find("Injected: 1")) << response_str; | ||
| } |
wwbmmm
reviewed
Jul 15, 2026
| for (HttpHeader::HeaderIterator it = h->HeaderBegin(); | ||
| it != h->HeaderEnd(); ++it) { | ||
| if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { | ||
| LOG(ERROR) << "Skip header `" << it->first |
Contributor
There was a problem hiding this comment.
This should be warning, not error
Contributor
Author
There was a problem hiding this comment.
Done, switched both to LOG(WARNING). Also wrapped the logged name in butil::ToPrintable so a CR/LF in the name can't inject newlines into the log.
| // such fields so a value forwarded from an untrusted source can't smuggle | ||
| // headers. | ||
| static bool HeaderHasCRLF(const std::string& s) { | ||
| return s.find_first_of("\r\n") != std::string::npos; |
Contributor
There was a problem hiding this comment.
This may affect performance, since MakeRawHttpRequest/Response are hot paths. You'd better add a flag to switch this on/off.
Contributor
Author
There was a problem hiding this comment.
Good point. I put it behind -http_check_outbound_header_crlf (default on). The scan is a single find_first_of over the header name/value, so it's cheap, but anyone who never forwards untrusted header values can turn it off.
…name Signed-off-by: ubeddulla khan <ubed@bugqore.com>
Contributor
Author
|
Pushed an update covering the review:
|
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.
What problem does this PR solve?
Issue Number:
Problem Summary:
MakeRawHttpRequestandMakeRawHttpResponsewrite every header field-name and field-value straight onto the wire (os << it->first << ": " << it->second << CRLF), and emitContent-Typefromcontent_type()the same way. When one of those values carries a raw CR or LF (for example a value that an application forwards from an untrusted request), it closes the current header line and the following bytes are parsed as extra header fields or a body, i.e. HTTP request/response splitting. The inboundhttp_parseralready refuses these control bytes in header fields, but the outbound serializers trusted them.What is changed and the side effects?
Changed:
HeaderHasCRLF()and, in both serializers, skip any header whose name or value contains CR or LF (a warning is logged).Content-Typeline in both functions.Side effects:
find_first_ofscan per emitted header; negligible.