This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new d54ffe98 reject CR/LF in SetHttpURL and SetH2Path (#3588)
d54ffe98 is described below
commit d54ffe98f1ed956b1756800876100027894d7f80
Author: sahvx655-wq <[email protected]>
AuthorDate: Fri Oct 9 11:54:57 2026 +0530
reject CR/LF in SetHttpURL and SetH2Path (#3588)
Both parsers copied control characters into _host/_path/_query/_fragment,
which MakeRawHttpRequest and GenerateH2Path then wrote verbatim into the Host
header, the request line and the HTTP/2 :path.
---
src/brpc/policy/http2_rpc_protocol.cpp | 4 ++--
src/brpc/uri.cpp | 22 ++++++++++++++++++++++
test/brpc_uri_unittest.cpp | 32 ++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/src/brpc/policy/http2_rpc_protocol.cpp
b/src/brpc/policy/http2_rpc_protocol.cpp
index 060b25fa..004430af 100644
--- a/src/brpc/policy/http2_rpc_protocol.cpp
+++ b/src/brpc/policy/http2_rpc_protocol.cpp
@@ -1405,8 +1405,8 @@ int
H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
<< ", 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.
+ // Including path/query/fragment. Fails on a CR/LF
+ // in the value or too many query parameters.
LOG(ERROR) << h.uri().status().error_cstr()
<< ", stream_id=" << _stream_id;
_rejected_error = H2_ENHANCE_YOUR_CALM;
diff --git a/src/brpc/uri.cpp b/src/brpc/uri.cpp
index 6b10464b..90263ff2 100644
--- a/src/brpc/uri.cpp
+++ b/src/brpc/uri.cpp
@@ -17,6 +17,7 @@
#include <ctype.h> // isalnum
+#include <string.h> // strpbrk
#include <unordered_set>
#include <gflags/gflags.h>
#include "brpc/log.h"
@@ -180,10 +181,26 @@ static const char g_url_parsing_fast_action_map_raw[] = {
static const char* const g_url_parsing_fast_action_map =
g_url_parsing_fast_action_map_raw + 128;
+// CR and LF are never valid in a URL and must not survive parsing: every
+// component is written verbatim into the serialized message later on, the
+// host/port into the "Host" header and the :authority pseudo-header, and
+// path/query/fragment into the HTTP/1 request line (PrintWithoutHost) and
+// the HTTP/2 :path (GenerateH2Path). Either byte there appends headers to
+// the request or starts a second one. Only the parsers below reject it; the
+// set_*() overwriters are documented as taking already-valid input.
+static bool has_crlf(const char* s) {
+ return strpbrk(s, "\r\n") != nullptr;
+}
+
// This implementation is faster than http_parser_parse_url() and allows
// ignoring of scheme("http://";)
int URI::SetHttpURL(const char* url) {
Clear();
+
+ if (has_crlf(url)) {
+ _st.set_error(EINVAL, "Invalid CR/LF in url");
+ return -1;
+ }
const char* p = url;
// skip heading blanks
@@ -441,6 +458,11 @@ int URI::SetH2Path(const char* h2_path) {
_initialized_query_map = false;
_query_map.clear();
+ if (has_crlf(h2_path)) {
+ _st.set_error(EINVAL, "Invalid CR/LF in :path");
+ return -1;
+ }
+
const char* p = h2_path;
const char* start = p;
for (; *p && *p != '?' && *p != '#'; ++p) {}
diff --git a/test/brpc_uri_unittest.cpp b/test/brpc_uri_unittest.cpp
index b1be2c55..1fb43696 100644
--- a/test/brpc_uri_unittest.cpp
+++ b/test/brpc_uri_unittest.cpp
@@ -346,6 +346,38 @@ TEST(URITest, invalid_spaces) {
ASSERT_STREQ("Invalid space in fragment", uri.status().error_cstr());
}
+TEST(URITest, invalid_crlf) {
+ brpc::URI uri;
+ // A CR/LF that reaches _host, _path, _query or _fragment is written
+ // verbatim into the "Host" header and the request line, which appends
+ // headers to the outbound request or starts a second one. None of these
+ // contains a space, so the "Invalid space" checks cannot catch them.
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com\r\nX-Injected:1/s";));
+ ASSERT_STREQ("Invalid CR/LF in url", uri.status().error_cstr());
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s\r\nX-Injected:1";));
+ ASSERT_STREQ("Invalid CR/LF in url", uri.status().error_cstr());
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s?wd=uri\r\nX-Injected:1";));
+ ASSERT_STREQ("Invalid CR/LF in url", uri.status().error_cstr());
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s#frag\r\nX-Injected:1";));
+ ASSERT_STREQ("Invalid CR/LF in url", uri.status().error_cstr());
+ // A bare CR or LF is enough, the pair is not required.
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s\nX-Injected:1";));
+ ASSERT_EQ(-1, uri.SetHttpURL("http://a.com/s\rX-Injected:1";));
+ // Nothing of the rejected URL is left behind.
+ ASSERT_EQ("", uri.host());
+ ASSERT_EQ("", uri.path());
+
+ ASSERT_EQ(-1, uri.SetH2Path("/s\r\nX-Injected:1"));
+ ASSERT_STREQ("Invalid CR/LF in :path", uri.status().error_cstr());
+ ASSERT_EQ(-1, uri.SetH2Path("/s?wd=uri\r\nX-Injected:1"));
+ ASSERT_STREQ("Invalid CR/LF in :path", uri.status().error_cstr());
+ ASSERT_EQ(-1, uri.SetH2Path("/s#frag\r\nX-Injected:1"));
+ ASSERT_STREQ("Invalid CR/LF in :path", uri.status().error_cstr());
+ ASSERT_EQ("", uri.path());
+ // The next path clears the failure rather than inheriting it.
+ ASSERT_EQ(0, uri.SetH2Path("/s?wd=uri")) << uri.status();
+}
+
TEST(URITest, invalid_query) {
brpc::URI uri;
ASSERT_EQ(0, uri.SetHttpURL("http://a.b.c/?a-b-c:def";));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
