From 32fa1006a502dee9bed9ed2dc122ce402791c6ba Mon Sep 17 00:00:00 2001 From: ubeddulla khan Date: Tue, 14 Jul 2026 13:25:00 +0530 Subject: [PATCH 1/2] skip header fields containing CR/LF in http serialization --- src/brpc/details/http_message.cpp | 25 +++++++++++++++++++++++-- test/brpc_http_message_unittest.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/brpc/details/http_message.cpp b/src/brpc/details/http_message.cpp index 003bafa074..8211975a31 100644 --- a/src/brpc/details/http_message.cpp +++ b/src/brpc/details/http_message.cpp @@ -578,6 +578,16 @@ std::ostream& operator<<(std::ostream& os, const http_parser& parser) { #define BRPC_CRLF "\r\n" +// A header field-name or field-value carrying a raw CR or LF lets whoever +// controls it close the current line and inject extra header fields (or a +// body) into the serialized message, i.e. HTTP request/response splitting. +// The inbound parser already refuses these bytes; the outbound path drops +// 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; +} + // Request format // Request = Request-Line ; Section 5.1 // *(( general-header ; Section 4.5 @@ -644,12 +654,17 @@ void MakeRawHttpRequest(butil::IOBuf* request, } os << BRPC_CRLF; } - if (!h->content_type().empty()) { + if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) { os << "Content-Type: " << h->content_type() << BRPC_CRLF; } for (HttpHeader::HeaderIterator it = h->HeaderBegin(); it != h->HeaderEnd(); ++it) { + if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { + LOG(ERROR) << "Skip header `" << it->first + << "' containing CR/LF to avoid injection"; + continue; + } os << it->first << ": " << it->second << BRPC_CRLF; } if (h->GetHeader("Accept") == NULL) { @@ -733,12 +748,18 @@ void MakeRawHttpResponse(butil::IOBuf* response, } } } - if (!is_invalid_content && !h->content_type().empty()) { + if (!is_invalid_content && !h->content_type().empty() && + !HeaderHasCRLF(h->content_type())) { os << "Content-Type: " << h->content_type() << BRPC_CRLF; } for (HttpHeader::HeaderIterator it = h->HeaderBegin(); it != h->HeaderEnd(); ++it) { + if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { + LOG(ERROR) << "Skip header `" << it->first + << "' containing CR/LF to avoid injection"; + continue; + } os << it->first << ": " << it->second << BRPC_CRLF; } os << BRPC_CRLF; // CRLF before content diff --git a/test/brpc_http_message_unittest.cpp b/test/brpc_http_message_unittest.cpp index 9933a8d1bb..b9d1fd5070 100644 --- a/test/brpc_http_message_unittest.cpp +++ b/test/brpc_http_message_unittest.cpp @@ -703,6 +703,31 @@ TEST(HttpMessageTest, serialize_http_response) { << butil::ToPrintable(response); } +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; +} + TEST(HttpMessageTest, http_1_1_request_without_host) { brpc::FLAGS_allow_http_1_1_request_without_host = false; { From b6c27654954966d1ac989e6019785c25f2087f5f Mon Sep 17 00:00:00 2001 From: ubeddulla khan Date: Wed, 15 Jul 2026 15:17:27 +0530 Subject: [PATCH 2/2] Gate CR/LF header check behind a flag; log at WARNING with sanitized name Signed-off-by: ubeddulla khan --- src/brpc/details/http_message.cpp | 43 ++++++++++++++++++++--------- test/brpc_http_message_unittest.cpp | 26 +++++++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/brpc/details/http_message.cpp b/src/brpc/details/http_message.cpp index 8211975a31..e6e4dc0b8e 100644 --- a/src/brpc/details/http_message.cpp +++ b/src/brpc/details/http_message.cpp @@ -23,6 +23,7 @@ #include "butil/scoped_lock.h" #include "butil/endpoint.h" #include "butil/base64.h" +#include "butil/binary_printer.h" // ToPrintable #include "bthread/bthread.h" // bthread_usleep #include "brpc/log.h" #include "brpc/reloadable_flags.h" @@ -38,6 +39,9 @@ DEFINE_bool(http_verbose, false, "[DEBUG] Print EVERY http request/response"); DEFINE_int32(http_verbose_max_body_length, 512, "[DEBUG] Max body length printed when -http_verbose is on"); +DEFINE_bool(http_check_outbound_header_crlf, true, + "Skip outbound http header fields whose name or value contains " + "CR/LF to prevent request/response splitting."); DECLARE_int64(socket_max_unwritten_bytes); DECLARE_uint64(max_body_size); @@ -583,9 +587,11 @@ std::ostream& operator<<(std::ostream& os, const http_parser& parser) { // body) into the serialized message, i.e. HTTP request/response splitting. // The inbound parser already refuses these bytes; the outbound path drops // such fields so a value forwarded from an untrusted source can't smuggle -// headers. +// headers. Gated by -http_check_outbound_header_crlf so it can be turned +// off on hot paths that never forward untrusted header values. static bool HeaderHasCRLF(const std::string& s) { - return s.find_first_of("\r\n") != std::string::npos; + return FLAGS_http_check_outbound_header_crlf && + s.find_first_of("\r\n") != std::string::npos; } // Request format @@ -654,15 +660,21 @@ void MakeRawHttpRequest(butil::IOBuf* request, } os << BRPC_CRLF; } - if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) { - os << "Content-Type: " << h->content_type() - << BRPC_CRLF; + if (!h->content_type().empty()) { + if (HeaderHasCRLF(h->content_type())) { + LOG(WARNING) << "Skip Content-Type `" + << butil::ToPrintable(h->content_type()) + << "' containing CR/LF to avoid injection"; + } else { + os << "Content-Type: " << h->content_type() + << BRPC_CRLF; + } } for (HttpHeader::HeaderIterator it = h->HeaderBegin(); it != h->HeaderEnd(); ++it) { if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { - LOG(ERROR) << "Skip header `" << it->first - << "' containing CR/LF to avoid injection"; + LOG(WARNING) << "Skip header `" << butil::ToPrintable(it->first) + << "' containing CR/LF to avoid injection"; continue; } os << it->first << ": " << it->second << BRPC_CRLF; @@ -748,16 +760,21 @@ void MakeRawHttpResponse(butil::IOBuf* response, } } } - if (!is_invalid_content && !h->content_type().empty() && - !HeaderHasCRLF(h->content_type())) { - os << "Content-Type: " << h->content_type() - << BRPC_CRLF; + if (!is_invalid_content && !h->content_type().empty()) { + if (HeaderHasCRLF(h->content_type())) { + LOG(WARNING) << "Skip Content-Type `" + << butil::ToPrintable(h->content_type()) + << "' containing CR/LF to avoid injection"; + } else { + os << "Content-Type: " << h->content_type() + << BRPC_CRLF; + } } for (HttpHeader::HeaderIterator it = h->HeaderBegin(); it != h->HeaderEnd(); ++it) { if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) { - LOG(ERROR) << "Skip header `" << it->first - << "' containing CR/LF to avoid injection"; + LOG(WARNING) << "Skip header `" << butil::ToPrintable(it->first) + << "' containing CR/LF to avoid injection"; continue; } os << it->first << ": " << it->second << BRPC_CRLF; diff --git a/test/brpc_http_message_unittest.cpp b/test/brpc_http_message_unittest.cpp index b9d1fd5070..3ee05ffaf7 100644 --- a/test/brpc_http_message_unittest.cpp +++ b/test/brpc_http_message_unittest.cpp @@ -728,6 +728,32 @@ TEST(HttpMessageTest, serialize_header_with_crlf_is_not_injected) { ASSERT_EQ(std::string::npos, response_str.find("Injected: 1")) << response_str; } +TEST(HttpMessageTest, serialize_content_type_with_crlf_is_not_injected) { + // Content-Type goes through the same emission path and must be dropped + // (not written) when it carries CR/LF. + butil::EndPoint ep; + ASSERT_EQ(0, butil::str2endpoint("127.0.0.1:1234", &ep)); + + brpc::HttpHeader req_header; + req_header.set_method(brpc::HTTP_METHOD_POST); + req_header.set_content_type("text/plain\r\nInjected: 1"); + butil::IOBuf req_content; + req_content.append("data"); + butil::IOBuf request; + MakeRawHttpRequest(&request, &req_header, ep, &req_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.set_content_type("text/plain\r\nInjected: 1"); + butil::IOBuf res_content; + res_content.append("data"); + butil::IOBuf response; + MakeRawHttpResponse(&response, &res_header, &res_content); + std::string response_str = response.to_string(); + ASSERT_EQ(std::string::npos, response_str.find("Injected: 1")) << response_str; +} + TEST(HttpMessageTest, http_1_1_request_without_host) { brpc::FLAGS_allow_http_1_1_request_without_host = false; {