skip header fields containing CR/LF in http serialization

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

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;
     {