Skip to content

Bound the number of http headers and query parameters per message #3524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chenBright wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bound the number of http headers and query parameters per message #3524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/brpc/details/http_message.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Comment thread
chenBright marked this conversation as resolved.
DECLARE_int64(socket_max_unwritten_bytes);
DECLARE_uint64(max_body_size);

Expand Down Expand Up @@ -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;
}
Comment thread
chenBright marked this conversation as resolved.
Comment on lines +138 to +143
Comment on lines 134 to +143

if (http_message->_cur_value && !http_message->_cur_value->empty()) {
http_message->_cur_value->append(
header.HeaderValueDelimiter(http_message->_cur_header));
Expand Down
62 changes: 49 additions & 13 deletions src/brpc/policy/http2_rpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -729,6 +730,11 @@ H2ParseResult H2StreamContext::OnHeaders(
<< ", stream_id=" << frame_head.stream_id;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
// The whole block went through the decoder, the connection is in a
// consistent state again and only this stream needs to be reset.
if (_rejected_error != H2_NO_ERROR) {
return MakeH2Error(_rejected_error, stream_id());
}
if (frame_head.flags & H2_FLAGS_END_STREAM) {
return OnEndStream();
}
Expand Down Expand Up @@ -791,6 +797,10 @@ H2ParseResult H2StreamContext::OnContinuation(
<< ", stream_id=" << frame_head.stream_id;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
// See the same check in H2StreamContext::OnHeaders().
if (_rejected_error != H2_NO_ERROR) {
return MakeH2Error(_rejected_error, stream_id());
}
if (_stream_ended) {
return OnEndStream();
}
Expand Down Expand Up @@ -1294,7 +1304,8 @@ H2StreamContext::H2StreamContext(bool read_body_progressively)
, _remote_window_left(0)
, _deferred_window_update(0)
, _correlation_id(INVALID_BTHREAD_ID.value)
, _decoded_header_list_size(0) {
, _decoded_header_list_size(0)
, _rejected_error(H2_NO_ERROR) {
header().set_version(2, 0);
#ifndef NDEBUG
get_h2_bvars()->h2_stream_context_count << 1;
Expand Down Expand Up @@ -1349,6 +1360,14 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
<< max_header_list_size << ", stream_id=" << _stream_id;
return -1;
}
if (_rejected_error != H2_NO_ERROR) {
// The stream is already refused, keep feeding the decoder so that
// the dynamic table stays in sync with the peer, but stop spending
// memory on fields nobody is going to read. A peer that keeps
// piling them up still runs into max_header_list_size above, which
// escalates to a connection error as it has to.
continue;
}
const char* const name = pair.name.c_str();
bool matched = false;
if (name[0] == ':') { // reserved names
Expand All @@ -1364,10 +1383,12 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
matched = true;
HttpMethod method;
if (!Str2HttpMethod(pair.value.c_str(), &method)) {
LOG(ERROR) << "Invalid method=" << pair.value;
return -1;
LOG(ERROR) << "Invalid method=" << pair.value
<< ", stream_id=" << _stream_id;
_rejected_error = H2_PROTOCOL_ERROR;
} else {
h.set_method(method);
}
h.set_method(method);
}
break;
case 'p':
Expand All @@ -1380,11 +1401,16 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
// would take the whole header block, since HPACK does not
// order pseudo-headers and :method may not have arrived.
if (pair.value != "*" && (pair.value.empty() || pair.value[0] != '/')) {
LOG(ERROR) << "Invalid path=" << pair.value;
return -1;
LOG(ERROR) << "Invalid path=" << pair.value
<< ", stream_id=" << _stream_id;
_rejected_error = H2_PROTOCOL_ERROR;
} else if (h.uri().SetH2Path(pair.value) != 0) {
// Including path/query/fragment. The only way this
// fails is too many query parameters.
LOG(ERROR) << h.uri().status().error_cstr()
<< ", stream_id=" << _stream_id;
_rejected_error = H2_ENHANCE_YOUR_CALM;
}
// Including path/query/fragment
h.uri().SetH2Path(pair.value);
}
break;
case 's':
Expand All @@ -1396,24 +1422,34 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
char* endptr = nullptr;
const int sc = strtol(pair.value.c_str(), &endptr, 10);
if (*endptr != '\0') {
LOG(ERROR) << "Invalid status=" << pair.value;
return -1;
LOG(ERROR) << "Invalid status=" << pair.value
<< ", stream_id=" << _stream_id;
_rejected_error = H2_PROTOCOL_ERROR;
} else {
h.set_status_code(sc);
}
h.set_status_code(sc);
}
break;
default:
break;
}
if (!matched) {
LOG(ERROR) << "Unknown name=`" << name << '\'';
return -1;
LOG(ERROR) << "Unknown pseudo-header=`" << name
<< "', stream_id=" << _stream_id;
_rejected_error = H2_PROTOCOL_ERROR;
}
} else if (name[0] == 'c' &&
strcmp(name + 1, /*c*/"ontent-type") == 0) {
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
<< ", stream_id=" << _stream_id;
Comment thread
chenBright marked this conversation as resolved.
_rejected_error = H2_ENHANCE_YOUR_CALM;
Comment on lines 1445 to +1451
}
}

if (FLAGS_http_verbose) {
Expand Down
17 changes: 16 additions & 1 deletion src/brpc/policy/http2_rpc_protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,9 @@ class H2StreamContext : public HttpContext {

// Decode headers in HPACK from *it and set into this->header(). The input
// does not need to complete.
// Returns 0 on success, -1 otherwise.
// Returns 0 on success, -1 on a connection-level error. A message that is
// merely malformed or unacceptable does not fail here, it sets
// `_rejected_error` instead, see the comment on that field.
int ConsumeHeaders(butil::IOBufBytesIterator& it);
H2ParseResult OnEndStream();

Expand Down Expand Up @@ -281,6 +283,19 @@ friend class H2Context;
// (name + value + 32 per field, RFC 7540 section 10.5.1), checked
// against the local max_header_list_size in ConsumeHeaders().
uint64_t _decoded_header_list_size;
// Set when this message must be refused although the connection itself is
// still healthy: it is malformed (invalid or unknown pseudo-header, RFC
// 9113 section 8.1.1 mandates a stream error of type PROTOCOL_ERROR) or it
// violates a local limit (too many headers, too many query parameters in
// :path). Only the stream is reset so that the other streams keep working,
// but the error cannot be raised where it is detected: HPACK keeps a
// dynamic table per connection, so leaving the rest of the block undecoded
// would desynchronize it from the encoding table of the peer and corrupt
// every header block that follows. RFC 9113 section 10.5.1: "The field
// block MUST be processed to ensure a consistent connection state, unless
// the connection is closed." Hence the rejection is remembered here and
// turned into a RST_STREAM once END_HEADERS is reached.
H2Error _rejected_error;
butil::IOBuf _remaining_header_fragment;
// Request body which cannot be sent yet due to remote flow control.
// Accessed under H2Context::_stream_mutex.
Expand Down
1 change: 1 addition & 0 deletions src/brpc/socket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment thread
chenBright marked this conversation as resolved.
CHECK(nullptr == _write_head.load(butil::memory_order_relaxed));
_is_write_shutdown = false;
int fd = options.fd;
Expand Down
40 changes: 34 additions & 6 deletions src/brpc/uri.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,25 @@


#include <ctype.h> // isalnum

#include <unordered_set>

#include <gflags/gflags.h>
#include "brpc/log.h"
#include "brpc/details/http_parser.h" // http_parser_parse_url
#include "brpc/uri.h" // URI


namespace brpc {

DEFINE_uint32(http_max_query_count, 1000,
"Reject an URL carrying more than so many query parameters. "
Comment thread
chenBright marked this conversation as resolved.
"0 lifts the limit.");
Comment thread
chenBright marked this conversation as resolved.

URI::URI()
: _port(-1)
, _query_was_modified(false)
, _initialized_query_map(false)
{}

URI::~URI() {
}

void URI::Clear() {
_st.reset();
_port = -1;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions src/brpc/uri.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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()); }
Comment thread
chenBright marked this conversation as resolved.
Comment thread
chenBright marked this conversation as resolved.
Comment on lines +102 to +104

// Get the value of a CASE-SENSITIVE key.
// Returns pointer to the value, nullptr when the key does not exist.
Expand Down
60 changes: 60 additions & 0 deletions test/brpc_http_message_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(),
Expand Down
Loading
Loading