From 307d21d36073efb2e0a9064c8fb4b8ee8230bdf9 Mon Sep 17 00:00:00 2001 From: chenBright Date: Sat, 5 Sep 2026 22:19:05 +0800 Subject: [PATCH] Bound the number of http headers and query parameters per message --- src/brpc/details/http_message.cpp | 11 +++ src/brpc/policy/http2_rpc_protocol.cpp | 12 ++- src/brpc/socket.cpp | 1 + src/brpc/uri.cpp | 40 ++++++++-- src/brpc/uri.h | 7 +- test/brpc_http_message_unittest.cpp | 60 +++++++++++++++ test/brpc_http_rpc_protocol_unittest.cpp | 97 ++++++++++++++++++++++-- test/brpc_uri_unittest.cpp | 32 ++++++++ 8 files changed, 242 insertions(+), 18 deletions(-) diff --git a/src/brpc/details/http_message.cpp b/src/brpc/details/http_message.cpp index 0cb4f78378..14a81e553c 100644 --- a/src/brpc/details/http_message.cpp +++ b/src/brpc/details/http_message.cpp @@ -49,6 +49,9 @@ DEFINE_int32(http_verbose_max_body_length, 512, 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."); +DEFINE_uint32(http_max_header_count, 100, + "Reject a message carrying more than so many header fields. " + "0 lifts the limit."); DECLARE_int64(socket_max_unwritten_bytes); DECLARE_uint64(max_body_size); @@ -131,6 +134,14 @@ int HttpMessage::on_header_value(http_parser *parser, http_message->_cur_value = &header.AddHeader(http_message->_cur_header); } + + if (FLAGS_http_max_header_count > 0 && + header.HeaderCount() > FLAGS_http_max_header_count) { + LOG(ERROR) << "Too many headers, max=" + << FLAGS_http_max_header_count; + return -1; + } + if (http_message->_cur_value && !http_message->_cur_value->empty()) { http_message->_cur_value->append( header.HeaderValueDelimiter(http_message->_cur_header)); diff --git a/src/brpc/policy/http2_rpc_protocol.cpp b/src/brpc/policy/http2_rpc_protocol.cpp index b20a05727c..00e4a74de9 100644 --- a/src/brpc/policy/http2_rpc_protocol.cpp +++ b/src/brpc/policy/http2_rpc_protocol.cpp @@ -29,6 +29,7 @@ DECLARE_int32(http_verbose_max_body_length); DECLARE_int32(health_check_interval); DECLARE_bool(usercode_in_pthread); DECLARE_int64(socket_max_unwritten_bytes); +DECLARE_uint32(http_max_header_count); namespace policy { @@ -1384,7 +1385,10 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) { return -1; } // Including path/query/fragment - h.uri().SetH2Path(pair.value); + if (h.uri().SetH2Path(pair.value) != 0) { + LOG(ERROR) << h.uri().status().error_cstr(); + return -1; + } } break; case 's': @@ -1414,6 +1418,12 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) { h.set_content_type(pair.value); } else { h.AppendHeader(pair.name, pair.value); + if (FLAGS_http_max_header_count > 0 && + h.HeaderCount() > FLAGS_http_max_header_count) { + LOG(ERROR) << "Too many headers, max=" + << FLAGS_http_max_header_count; + return -1; + } } if (FLAGS_http_verbose) { diff --git a/src/brpc/socket.cpp b/src/brpc/socket.cpp index 283473df9e..a0b49662fe 100644 --- a/src/brpc/socket.cpp +++ b/src/brpc/socket.cpp @@ -794,6 +794,7 @@ int Socket::OnCreated(const SocketOptions& options) { _unwritten_bytes.store(0, butil::memory_order_relaxed); _keepalive_options = options.keepalive_options; _tcp_user_timeout_ms = options.tcp_user_timeout_ms; + _http_request_method = HTTP_METHOD_GET; CHECK(nullptr == _write_head.load(butil::memory_order_relaxed)); _is_write_shutdown = false; int fd = options.fd; diff --git a/src/brpc/uri.cpp b/src/brpc/uri.cpp index 2881a8e54a..326bdb0039 100644 --- a/src/brpc/uri.cpp +++ b/src/brpc/uri.cpp @@ -17,9 +17,8 @@ #include // isalnum - #include - +#include #include "brpc/log.h" #include "brpc/details/http_parser.h" // http_parser_parse_url #include "brpc/uri.h" // URI @@ -27,15 +26,16 @@ namespace brpc { +DEFINE_uint32(http_max_query_count, 1000, + "Reject an URL carrying more than so many query parameters. " + "0 lifts the limit."); + URI::URI() : _port(-1) , _query_was_modified(false) , _initialized_query_map(false) {} -URI::~URI() { -} - void URI::Clear() { _st.reset(); _port = -1; @@ -64,6 +64,22 @@ void URI::Swap(URI &rhs) { _query_map.swap(rhs._query_map); } +// Counting separators rather than map entries deliberately overestimates: the +// splitter walks every segment even when the keys repeat, and it is that walk, +// not the final map size, that the limit is meant to bound. +static bool TooManyQueries(const std::string& query) { + if (FLAGS_http_max_query_count == 0 || query.empty()) { + return false; + } + uint32_t count = 1; + for (char i : query) { + if (i == '&' && ++count > FLAGS_http_max_query_count) { + return true; + } + } + return false; +} + // Parse queries, which is case-sensitive static void ParseQueries(URI::QueryMap& query_map, const std::string &query) { query_map.clear(); @@ -238,6 +254,11 @@ int URI::SetHttpURL(const char* url) { } } _query.assign(start, p - start); + if (TooManyQueries(_query)) { + _st.set_error(EINVAL, "More than %u query parameters in url", + FLAGS_http_max_query_count); + return -1; + } } if (*p == '#') { start = ++p; @@ -411,7 +432,8 @@ void URI::SetHostAndPort(const std::string& host) { _host.assign(host_begin, host_end - host_begin); } -void URI::SetH2Path(const char* h2_path) { +int URI::SetH2Path(const char* h2_path) { + _st.reset(); _path.clear(); _query.clear(); _fragment.clear(); @@ -427,12 +449,18 @@ void URI::SetH2Path(const char* h2_path) { start = ++p; for (; *p && *p != '#'; ++p) {} _query.assign(start, p - start); + if (TooManyQueries(_query)) { + _st.set_error(EINVAL, "More than %u query parameters in :path", + FLAGS_http_max_query_count); + return -1; + } } if (*p == '#') { start = ++p; for (; *p; ++p) {} _fragment.assign(start, p - start); } + return 0; } QueryRemover::QueryRemover(const std::string* str) diff --git a/src/brpc/uri.h b/src/brpc/uri.h index 7edac4002d..a42cf88f12 100644 --- a/src/brpc/uri.h +++ b/src/brpc/uri.h @@ -56,7 +56,7 @@ class URI { // You can copy a URI. URI(); - ~URI(); + ~URI() = default; // Exchange internal fields with another URI. void Swap(URI &rhs); @@ -99,8 +99,9 @@ class URI { void set_port(int port) { _port = port; } void SetHostAndPort(const std::string& host_and_optional_port); // Set path/query/fragment with the input in form of "path?query#fragment" - void SetH2Path(const char* h2_path); - void SetH2Path(const std::string& path) { SetH2Path(path.c_str()); } + // Returns 0 on success, -1 otherwise and status() is set. + int SetH2Path(const char* h2_path); + int SetH2Path(const std::string& path) { return SetH2Path(path.c_str()); } // Get the value of a CASE-SENSITIVE key. // Returns pointer to the value, nullptr when the key does not exist. diff --git a/test/brpc_http_message_unittest.cpp b/test/brpc_http_message_unittest.cpp index 57e98ccab0..90c9dbddae 100644 --- a/test/brpc_http_message_unittest.cpp +++ b/test/brpc_http_message_unittest.cpp @@ -32,6 +32,7 @@ DECLARE_bool(allow_chunked_length); DECLARE_bool(allow_http_1_1_request_without_host); DECLARE_bool(http_allow_obs_fold); DECLARE_bool(http_strict_header_token); +DECLARE_uint32(http_max_header_count); int main(int argc, char* argv[]) { testing::InitGoogleTest(&argc, argv); @@ -643,6 +644,65 @@ TEST(HttpMessageTest, htab_is_ows_in_header_values) { } } +TEST(HttpMessageTest, too_many_headers) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + brpc::FLAGS_http_max_header_count = 8; + + // Host counts as well, so 8 distinct names in total are accepted. + std::string at_limit = "GET / HTTP/1.1\r\nHost: a.com\r\n"; + for (int i = 1; i < 8; ++i) { + at_limit.append("h" + std::to_string(i) + ": v\r\n"); + } + std::string over_limit = at_limit + "last: v\r\n\r\n"; + at_limit.append("\r\n"); + { + brpc::HttpMessage http_message; + ASSERT_EQ((ssize_t)at_limit.size(), + http_message.ParseFromArray(at_limit.data(), at_limit.size())) + << http_message._parser; + ASSERT_EQ(8u, http_message.header().HeaderCount()); + } + { + brpc::HttpMessage http_message; + ASSERT_EQ(-1, http_message.ParseFromArray(over_limit.data(), + over_limit.size())); + } + + // Repeated names fold into one entry, so they occupy one bucket and are not + // what the limit is aimed at. + std::string folded = "GET / HTTP/1.1\r\nHost: a.com\r\n"; + for (int i = 0; i < 100; ++i) { + folded.append("dup: v\r\n"); + } + folded.append("\r\n"); + { + brpc::HttpMessage http_message; + ASSERT_EQ((ssize_t)folded.size(), + http_message.ParseFromArray(folded.data(), folded.size())) + << http_message._parser; + ASSERT_EQ(2u, http_message.header().HeaderCount()); + } + // Set-Cookie is the one name that does not fold, so each occurrence is its + // own entry and does count. + std::string cookies = "GET / HTTP/1.1\r\nHost: a.com\r\n"; + for (int i = 0; i < 100; ++i) { + cookies.append("Set-Cookie: a=b\r\n"); + } + cookies.append("\r\n"); + { + brpc::HttpMessage http_message; + ASSERT_EQ(-1, http_message.ParseFromArray(cookies.data(), cookies.size())); + } + + brpc::FLAGS_http_max_header_count = 0; + { + brpc::HttpMessage http_message; + ASSERT_EQ((ssize_t)over_limit.size(), + http_message.ParseFromArray(over_limit.data(), over_limit.size())) + << http_message._parser; + } +} + TEST(HttpMessageTest, find_method_property_by_uri) { brpc::Server server; ASSERT_EQ(0, server.AddService(new test::EchoService(), diff --git a/test/brpc_http_rpc_protocol_unittest.cpp b/test/brpc_http_rpc_protocol_unittest.cpp index e0c7b741dd..126be1b9b2 100644 --- a/test/brpc_http_rpc_protocol_unittest.cpp +++ b/test/brpc_http_rpc_protocol_unittest.cpp @@ -63,6 +63,8 @@ DECLARE_bool(allow_chunked_length); DECLARE_int32(max_connection_pool_size); DECLARE_uint64(max_body_size); DECLARE_int64(socket_max_unwritten_bytes); +DECLARE_uint32(http_max_header_count); +DECLARE_uint32(http_max_query_count); extern bvar::CollectorSpeedLimit g_rpc_dump_sl; } @@ -1940,6 +1942,85 @@ TEST_F(HttpTest, h2_header_list_budget_resets_per_block) { delete sctx; } +// Literal header field without indexing, new name (RFC 7541 6.2.2), with both +// lengths in a single 7-bit prefix octet. 0x80 of that octet is the Huffman +// flag and a length of 128 or more needs the multi-octet form, so refuse what +// does not fit instead of emitting a corrupt header block. +void AppendLiteralHeader(butil::IOBuf* out, const std::string& name, + const std::string& value) { + ASSERT_LT(name.size(), 0x80u); + ASSERT_LT(value.size(), 0x80u); + uint8_t prefix[] = { 0x00, (uint8_t)name.size() }; + out->append(prefix, sizeof(prefix)); + out->append(name); + uint8_t value_len = (uint8_t)value.size(); + out->append(&value_len, 1); + out->append(value); +} + +TEST_F(HttpTest, h2_too_many_headers) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + brpc::FLAGS_http_max_header_count = 8; + + brpc::policy::H2Context* ctx = + new brpc::policy::H2Context(_socket.get(), nullptr); + CHECK_EQ(ctx->Init(), 0); + _socket->initialize_parsing_context(&ctx); + + { + std::unique_ptr sctx( + new brpc::policy::H2StreamContext(false)); + sctx->Init(ctx, 1); + butil::IOBuf payload; + for (int i = 0; i < 8; ++i) { + AppendLiteralHeader(&payload, "h" + std::to_string(i), "v"); + } + butil::IOBufBytesIterator it(payload); + ASSERT_EQ(0, sctx->ConsumeHeaders(it)); + ASSERT_EQ(8u, sctx->header().HeaderCount()); + } + { + std::unique_ptr sctx( + new brpc::policy::H2StreamContext(false)); + sctx->Init(ctx, 3); + butil::IOBuf payload; + for (int i = 0; i < 9; ++i) { + AppendLiteralHeader(&payload, "h" + std::to_string(i), "v"); + } + butil::IOBufBytesIterator it(payload); + ASSERT_EQ(-1, sctx->ConsumeHeaders(it)); + } +} + +TEST_F(HttpTest, h2_too_many_queries_in_path) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + brpc::FLAGS_http_max_query_count = 4; + + brpc::policy::H2Context* ctx = + new brpc::policy::H2Context(_socket.get(), nullptr); + CHECK_EQ(ctx->Init(), 0); + _socket->initialize_parsing_context(&ctx); + + { + std::unique_ptr sctx( + new brpc::policy::H2StreamContext(false)); + sctx->Init(ctx, 1); + butil::IOBuf payload; + AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4"); + butil::IOBufBytesIterator it(payload); + ASSERT_EQ(0, sctx->ConsumeHeaders(it)); + } + { + std::unique_ptr sctx( + new brpc::policy::H2StreamContext(false)); + sctx->Init(ctx, 3); + butil::IOBuf payload; + AppendLiteralHeader(&payload, ":path", "/s?a=1&b=2&c=3&d=4&e=5"); + butil::IOBufBytesIterator it(payload); + ASSERT_EQ(-1, sctx->ConsumeHeaders(it)); + } +} + TEST_F(HttpTest, h2_oversized_single_headers_block_rejected) { // A single HEADERS frame whose decoded header list exceeds // max_header_list_size must be rejected at the block boundary (before @@ -2644,8 +2725,8 @@ TEST_F(HttpTest, http_head) { const int port = 8923; brpc::Server server; HttpServiceImpl svc; - EXPECT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE)); - EXPECT_EQ(0, server.Start(port, nullptr)); + ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE)); + ASSERT_EQ(0, server.Start(port, nullptr)); brpc::Channel channel; brpc::ChannelOptions options; @@ -2770,11 +2851,11 @@ TEST_F(HttpTest, http_expect) { const int port = 8923; brpc::Server server; HttpServiceImpl svc; - EXPECT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE)); - EXPECT_EQ(0, server.Start(port, nullptr)); + ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE)); + ASSERT_EQ(0, server.Start(port, nullptr)); butil::EndPoint ep; - ASSERT_EQ(0, butil::str2endpoint("127.0.0.1:8923", &ep)); + ASSERT_EQ(0, butil::str2endpoint("127.0.0.1", port, &ep)); brpc::SocketOptions options; options.remote_side = ep; brpc::SocketId id; @@ -2803,17 +2884,17 @@ TEST_F(HttpTest, http_expect) { } // 100 Continue brpc::DestroyingPtr imsg_guard; - ReadOneResponse(sock, imsg_guard); + ASSERT_NO_FATAL_FAILURE(ReadOneResponse(sock, imsg_guard)); ASSERT_EQ(imsg_guard->header().status_code(), brpc::HTTP_STATUS_CONTINUE); ASSERT_EQ(0, sock->Write(&content)); // 200 Ok - ReadOneResponse(sock, imsg_guard); + ASSERT_NO_FATAL_FAILURE(ReadOneResponse(sock, imsg_guard)); ASSERT_EQ(imsg_guard->header().status_code(), brpc::HTTP_STATUS_OK); ASSERT_EQ(0, sock->Write(&request_buf)); // 200 Ok - ReadOneResponse(sock, imsg_guard); + ASSERT_NO_FATAL_FAILURE(ReadOneResponse(sock, imsg_guard)); ASSERT_EQ(imsg_guard->header().status_code(), brpc::HTTP_STATUS_OK); } diff --git a/test/brpc_uri_unittest.cpp b/test/brpc_uri_unittest.cpp index b9d6b6508e..b1be2c55b3 100644 --- a/test/brpc_uri_unittest.cpp +++ b/test/brpc_uri_unittest.cpp @@ -15,10 +15,15 @@ // specific language governing permissions and limitations // under the License. +#include #include #include "brpc/uri.h" +namespace brpc { +DECLARE_uint32(http_max_query_count); +} + TEST(URITest, everything) { brpc::URI uri; std::string uri_str = " foobar://user:passwd@www.baidu.com:80/s?wd=uri#frag "; @@ -347,6 +352,33 @@ TEST(URITest, invalid_query) { ASSERT_EQ("a-b-c:def", uri.query()); } +TEST(URITest, too_many_queries) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + brpc::FLAGS_http_max_query_count = 4; + + brpc::URI uri; + ASSERT_EQ(0, uri.SetHttpURL("http://a.com/s?a=1&b=2&c=3&d=4")) << uri.status(); + ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s?a=1&b=2&c=3&d=4&e=5")); + ASSERT_STREQ("More than 4 query parameters in url", uri.status().error_cstr()); + // Repeated keys collapse into one map entry, but the splitter still walks + // every segment, so they count. + ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s?a=1&a=2&a=3&a=4&a=5")); + // An empty query is not one parameter. + brpc::FLAGS_http_max_query_count = 1; + ASSERT_EQ(0, uri.SetHttpURL("http://a.com/s?")) << uri.status(); + + brpc::FLAGS_http_max_query_count = 4; + ASSERT_EQ(0, uri.SetH2Path("/s?a=1&b=2&c=3&d=4")) << uri.status(); + ASSERT_EQ(-1, uri.SetH2Path("/s?a=1&b=2&c=3&d=4&e=5")); + ASSERT_STREQ("More than 4 query parameters in :path", uri.status().error_cstr()); + // The next path clears the failure rather than inheriting it. + ASSERT_EQ(0, uri.SetH2Path("/s?a=1")) << uri.status(); + + brpc::FLAGS_http_max_query_count = 0; + ASSERT_EQ(0, uri.SetHttpURL("http://a.com/s?a=1&b=2&c=3&d=4&e=5")) << uri.status(); + ASSERT_EQ(0, uri.SetH2Path("/s?a=1&b=2&c=3&d=4&e=5")) << uri.status(); +} + TEST(URITest, high_bit_bytes) { // Bytes >= 0x80 (e.g. UTF-8 in the host/path) index the +128-biased // action table. On unsigned-char platforms they would read past the