diff --git a/src/brpc/details/http_message.cpp b/src/brpc/details/http_message.cpp index 003bafa074..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); @@ -578,6 +582,18 @@ 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. 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 FLAGS_http_check_outbound_header_crlf && + s.find_first_of("\r\n") != std::string::npos; +} + // Request format // Request = Request-Line ; Section 5.1 // *(( general-header ; Section 4.5 @@ -645,11 +661,22 @@ void MakeRawHttpRequest(butil::IOBuf* request, os << BRPC_CRLF; } if (!h->content_type().empty()) { - os << "Content-Type: " << h->content_type() - << BRPC_CRLF; + 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(WARNING) << "Skip header `" << butil::ToPrintable(it->first) + << "' containing CR/LF to avoid injection"; + continue; + } os << it->first << ": " << it->second << BRPC_CRLF; } if (h->GetHeader("Accept") == NULL) { @@ -734,11 +761,22 @@ void MakeRawHttpResponse(butil::IOBuf* response, } } if (!is_invalid_content && !h->content_type().empty()) { - os << "Content-Type: " << h->content_type() - << BRPC_CRLF; + 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(WARNING) << "Skip header `" << butil::ToPrintable(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..3ee05ffaf7 100644 --- a/test/brpc_http_message_unittest.cpp +++ b/test/brpc_http_message_unittest.cpp @@ -703,6 +703,57 @@ 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, 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; {