You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bc...@apache.org on 2022/08/09 02:54:00 UTC
[trafficserver] branch 9.1.x updated: [9.1.x] Backport HTTP Validations (#9016)
This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 9.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/9.1.x by this push:
new 2e3a50cf7 [9.1.x] Backport HTTP Validations (#9016)
2e3a50cf7 is described below
commit 2e3a50cf7992c34bd31a944b4ca3b8ae7475bd20
Author: Masaori Koshiba <ma...@apache.org>
AuthorDate: Tue Aug 9 11:53:53 2022 +0900
[9.1.x] Backport HTTP Validations (#9016)
* Fail fast on HTTP/2 header validation (#9009)
(cherry picked from commit eaef5e8d7a3f4e2540caec516bffc59fa22d2472)
Conflicts:
proxy/http2/HTTP2.cc
* Restrict unknown scheme of HTTP/2 request (#9010)
Strictly following RFC 3986 Section 3.1
```
scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
```
(cherry picked from commit c56f8722470a2e068557006ed6750c545602a62c)
Conflicts:
proxy/http2/HTTP2.cc
* Add control char check in MIME Parser (#9011)
(cherry picked from commit 2f363d973b321e58297fa356a5aab66b6b6788c1)
Conflicts:
tests/gold_tests/headers/good_request_after_bad.test.py
tests/gold_tests/logging/gold/field-json-test.gold
tests/gold_tests/logging/log-field-json.test.py
* Add content length mismatch check on handling HEADERS frame and CONTINUATION frame (#9012)
* Add content length mismatch check on handling HEADERS frame and CONTINUATION frame
* Correct error class of HTTP/2 malformed requests
(cherry picked from commit e92122833c68ec7528dce09ff0db630825ebc348)
* Ignore POST request case from a check for background fill (#9013)
(cherry picked from commit 1f3e1111931e26b5ad483a8bc80b3ae7edc0b568)
Co-authored-by: Masakazu Kitajo <ma...@apache.org>
---
include/tscore/ParseRules.h | 14 ++++++-
proxy/hdrs/MIME.cc | 15 +++++++
proxy/hdrs/MIME.h | 12 +++---
proxy/hdrs/URL.cc | 42 ++++++++++++++-----
proxy/hdrs/URL.h | 2 +
proxy/hdrs/unit_tests/test_Hdrs.cc | 49 ++++++++++++++++++++++
proxy/hdrs/unit_tests/test_URL.cc | 21 ++++++++++
proxy/http/HttpTunnel.h | 12 +++---
proxy/http2/HTTP2.cc | 24 ++++++++---
proxy/http2/Http2ConnectionState.cc | 14 ++++++-
proxy/http2/Http2Stream.cc | 6 ++-
.../gold/invalid_character_in_te_value.gold | 23 ++++++++++
.../headers/good_request_after_bad.test.py | 6 +++
13 files changed, 208 insertions(+), 32 deletions(-)
diff --git a/include/tscore/ParseRules.h b/include/tscore/ParseRules.h
index 1eac23d01..0c9318de3 100644
--- a/include/tscore/ParseRules.h
+++ b/include/tscore/ParseRules.h
@@ -128,7 +128,7 @@ public:
static CTypeResult is_empty(char c); // wslfcr,#
static CTypeResult is_alnum(char c); // 0-9,A-Z,a-z
static CTypeResult is_space(char c); // ' ' HT,VT,NP,CR,LF
- static CTypeResult is_control(char c); // 0-31 127
+ static CTypeResult is_control(char c); // 0x00-0x08, 0x0a-0x1f, 0x7f
static CTypeResult is_mime_sep(char c); // ()<>,;\"/[]?{} \t
static CTypeResult is_http_field_name(char c); // not : or mime_sep except for @
static CTypeResult is_http_field_value(char c); // not CR, LF, comma, or "
@@ -667,14 +667,24 @@ ParseRules::is_space(char c)
#endif
}
+/**
+ Return true if @c is a control char except HTAB(0x09) and SP(0x20).
+ If you need to check @c is HTAB or SP, use `ParseRules::is_ws`.
+ */
inline CTypeResult
ParseRules::is_control(char c)
{
#ifndef COMPILE_PARSE_RULES
return (parseRulesCType[(unsigned char)c] & is_control_BIT);
#else
- if (((unsigned char)c) < 32 || ((unsigned char)c) == 127)
+ if (c == CHAR_HT || c == CHAR_SP) {
+ return false;
+ }
+
+ if (((unsigned char)c) < 0x20 || c == 0x7f) {
return true;
+ }
+
return false;
#endif
}
diff --git a/proxy/hdrs/MIME.cc b/proxy/hdrs/MIME.cc
index 729ec9da9..23f8b7abd 100644
--- a/proxy/hdrs/MIME.cc
+++ b/proxy/hdrs/MIME.cc
@@ -2611,6 +2611,21 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap, MIMEHdrImpl *mh, const char
int field_name_wks_idx = hdrtoken_tokenize(field_name.data(), field_name.size());
+ if (field_name_wks_idx < 0) {
+ for (auto i : field_name) {
+ if (ParseRules::is_control(i)) {
+ return PARSE_RESULT_ERROR;
+ }
+ }
+ }
+
+ // RFC 9110 Section 5.5. Field Values
+ for (char i : field_value) {
+ if (ParseRules::is_control(i)) {
+ return PARSE_RESULT_ERROR;
+ }
+ }
+
///////////////////////////////////////////
// build and insert the new field object //
///////////////////////////////////////////
diff --git a/proxy/hdrs/MIME.h b/proxy/hdrs/MIME.h
index bce38ae87..9ff3e85d9 100644
--- a/proxy/hdrs/MIME.h
+++ b/proxy/hdrs/MIME.h
@@ -170,7 +170,7 @@ struct MIMEField {
int value_get_comma_list(StrList *list) const;
void name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length);
- bool name_is_valid() const;
+ bool name_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
void value_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length);
void value_set_int(HdrHeap *heap, MIMEHdrImpl *mh, int32_t value);
@@ -182,7 +182,7 @@ struct MIMEField {
// Other separators (e.g. ';' in Set-cookie/Cookie) are also possible
void value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length, bool prepend_comma = false,
const char separator = ',');
- bool value_is_valid() const;
+ bool value_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
int has_dups() const;
};
@@ -835,13 +835,13 @@ MIMEField::name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length
-------------------------------------------------------------------------*/
inline bool
-MIMEField::name_is_valid() const
+MIMEField::name_is_valid(uint32_t invalid_char_bits) const
{
const char *name;
int length;
for (name = name_get(&length); length > 0; length--) {
- if (ParseRules::is_control(name[length - 1])) {
+ if (ParseRules::is_type(name[length - 1], invalid_char_bits)) {
return false;
}
}
@@ -944,13 +944,13 @@ MIMEField::value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int l
-------------------------------------------------------------------------*/
inline bool
-MIMEField::value_is_valid() const
+MIMEField::value_is_valid(uint32_t invalid_char_bits) const
{
const char *value;
int length;
for (value = value_get(&length); length > 0; length--) {
- if (ParseRules::is_control(value[length - 1])) {
+ if (ParseRules::is_type(value[length - 1], invalid_char_bits)) {
return false;
}
}
diff --git a/proxy/hdrs/URL.cc b/proxy/hdrs/URL.cc
index e7fa71222..8124bb9f2 100644
--- a/proxy/hdrs/URL.cc
+++ b/proxy/hdrs/URL.cc
@@ -114,6 +114,36 @@ validate_host_name(std::string_view addr)
return std::all_of(addr.begin(), addr.end(), &is_host_char);
}
+/**
+ Checks if the (un-well-known) scheme is valid
+
+ RFC 3986 Section 3.1
+ These are the valid characters in a scheme:
+ scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
+ return an error if there is another character in the scheme
+*/
+bool
+validate_scheme(std::string_view scheme)
+{
+ if (scheme.empty()) {
+ return false;
+ }
+
+ if (!ParseRules::is_alpha(scheme[0])) {
+ return false;
+ }
+
+ for (size_t i = 0; i < scheme.size(); ++i) {
+ const char &c = scheme[i];
+
+ if (!(ParseRules::is_alnum(c) != 0 || c == '+' || c == '-' || c == '.')) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
@@ -1147,19 +1177,9 @@ url_parse_scheme(HdrHeap *heap, URLImpl *url, const char **start, const char *en
if (!(scheme_wks_idx > 0 && hdrtoken_wks_to_token_type(scheme_wks) == HDRTOKEN_TYPE_SCHEME)) {
// Unknown scheme, validate the scheme
-
- // RFC 3986 Section 3.1
- // These are the valid characters in a scheme:
- // scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
- // return an error if there is another character in the scheme
- if (!ParseRules::is_alpha(*scheme_start)) {
+ if (!validate_scheme({scheme_start, static_cast<size_t>(scheme_end - scheme_start)})) {
return PARSE_RESULT_ERROR;
}
- for (cur = scheme_start + 1; cur < scheme_end; ++cur) {
- if (!(ParseRules::is_alnum(*cur) != 0 || *cur == '+' || *cur == '-' || *cur == '.')) {
- return PARSE_RESULT_ERROR;
- }
- }
}
url_scheme_set(heap, url, scheme_start, scheme_wks_idx, scheme_end - scheme_start, copy_strings_p);
}
diff --git a/proxy/hdrs/URL.h b/proxy/hdrs/URL.h
index baa35315d..54fced5e5 100644
--- a/proxy/hdrs/URL.h
+++ b/proxy/hdrs/URL.h
@@ -157,6 +157,8 @@ extern int URL_LEN_MMST;
/* Public */
bool validate_host_name(std::string_view addr);
+bool validate_scheme(std::string_view scheme);
+
void url_init();
URLImpl *url_create(HdrHeap *heap);
diff --git a/proxy/hdrs/unit_tests/test_Hdrs.cc b/proxy/hdrs/unit_tests/test_Hdrs.cc
index 73022bf62..e7314fee3 100644
--- a/proxy/hdrs/unit_tests/test_Hdrs.cc
+++ b/proxy/hdrs/unit_tests/test_Hdrs.cc
@@ -530,6 +530,55 @@ TEST_CASE("HdrTest", "[proxy][hdrtest]")
mime_init();
http_init();
+ SECTION("Field Char Check")
+ {
+ static struct {
+ std::string_view line;
+ ParseResult expected;
+ } test_cases[] = {
+ ////
+ // Field Name
+ {"Content-Length: 10\r\n", PARSE_RESULT_CONT},
+ {"Content-Length\x0b: 10\r\n", PARSE_RESULT_ERROR},
+ ////
+ // Field Value
+ // SP
+ {"Content-Length: 10\r\n", PARSE_RESULT_CONT},
+ {"Foo: ab cd\r\n", PARSE_RESULT_CONT},
+ // HTAB
+ {"Foo: ab\td/cd\r\n", PARSE_RESULT_CONT},
+ // VCHAR
+ {"Foo: ab\x21/cd\r\n", PARSE_RESULT_CONT},
+ {"Foo: ab\x7e/cd\r\n", PARSE_RESULT_CONT},
+ // DEL
+ {"Foo: ab\x7f/cd\r\n", PARSE_RESULT_ERROR},
+ // obs-text
+ {"Foo: ab\x80/cd\r\n", PARSE_RESULT_CONT},
+ {"Foo: ab\xff/cd\r\n", PARSE_RESULT_CONT},
+ // control char
+ {"Content-Length: 10\x0b\r\n", PARSE_RESULT_ERROR},
+ {"Content-Length:\x0b 10\r\n", PARSE_RESULT_ERROR},
+ {"Foo: ab\x1d/cd\r\n", PARSE_RESULT_ERROR},
+ };
+
+ MIMEHdr hdr;
+ MIMEParser parser;
+ mime_parser_init(&parser);
+
+ for (const auto &t : test_cases) {
+ mime_parser_clear(&parser);
+
+ const char *start = t.line.data();
+ const char *end = start + t.line.size();
+
+ int r = hdr.parse(&parser, &start, end, false, false, false);
+ if (r != t.expected) {
+ std::printf("Expected %s is %s, but not", t.line.data(), t.expected == PARSE_RESULT_ERROR ? "invalid" : "valid");
+ CHECK(false);
+ }
+ }
+ }
+
SECTION("Test parse date")
{
static struct {
diff --git a/proxy/hdrs/unit_tests/test_URL.cc b/proxy/hdrs/unit_tests/test_URL.cc
index a39f7c01b..115aeee5d 100644
--- a/proxy/hdrs/unit_tests/test_URL.cc
+++ b/proxy/hdrs/unit_tests/test_URL.cc
@@ -53,6 +53,27 @@ TEST_CASE("ValidateURL", "[proxy][validurl]")
}
}
+TEST_CASE("Validate Scheme", "[proxy][validscheme]")
+{
+ static const struct {
+ std::string_view text;
+ bool valid;
+ } scheme_test_cases[] = {{"http", true}, {"https", true}, {"example", true}, {"example.", true},
+ {"example++", true}, {"example--.", true}, {"++example", false}, {"--example", false},
+ {".example", false}, {"example://", false}};
+
+ for (auto i : scheme_test_cases) {
+ // it's pretty hard to debug with
+ // CHECK(validate_scheme(i.text) == i.valid);
+
+ std::string_view text = i.text;
+ if (validate_scheme(text) != i.valid) {
+ std::printf("Validation of scheme: \"%s\", expected %s, but not\n", text.data(), (i.valid ? "true" : "false"));
+ CHECK(false);
+ }
+ }
+}
+
namespace UrlImpl
{
bool url_is_strictly_compliant(const char *start, const char *end);
diff --git a/proxy/http/HttpTunnel.h b/proxy/http/HttpTunnel.h
index 73842b1e0..37d0d0870 100644
--- a/proxy/http/HttpTunnel.h
+++ b/proxy/http/HttpTunnel.h
@@ -528,12 +528,14 @@ HttpTunnel::has_consumer_besides_client() const
continue;
}
- if (consumer.vc_type == HT_HTTP_CLIENT) {
- res = false;
+ switch (consumer.vc_type) {
+ case HT_HTTP_CLIENT:
continue;
- } else {
- res = true;
- break;
+ case HT_HTTP_SERVER:
+ // ignore uploading data to servers
+ continue;
+ default:
+ return true;
}
}
diff --git a/proxy/http2/HTTP2.cc b/proxy/http2/HTTP2.cc
index 50a5d2364..d26904f1a 100644
--- a/proxy/http2/HTTP2.cc
+++ b/proxy/http2/HTTP2.cc
@@ -426,11 +426,21 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
if (http_hdr_type_get(headers->m_http) == HTTP_TYPE_REQUEST) {
// :scheme
- if (MIMEField *field = headers->field_find(HTTP2_VALUE_SCHEME, HTTP2_LEN_SCHEME); field != nullptr && field->value_is_valid()) {
+ if (MIMEField *field = headers->field_find(HTTP2_VALUE_SCHEME, HTTP2_LEN_SCHEME);
+ field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
int scheme_len;
const char *scheme = field->value_get(&scheme_len);
+ const char *scheme_wks;
+
+ int scheme_wks_idx = hdrtoken_tokenize(scheme, scheme_len, &scheme_wks);
+
+ if (!(scheme_wks_idx > 0 && hdrtoken_wks_to_token_type(scheme_wks) == HDRTOKEN_TYPE_SCHEME)) {
+ // unkown scheme, validate the scheme
+ if (!validate_scheme({scheme, static_cast<size_t>(scheme_len)})) {
+ return PARSE_RESULT_ERROR;
+ }
+ }
- int scheme_wks_idx = hdrtoken_tokenize(scheme, scheme_len);
url_scheme_set(headers->m_heap, headers->m_http->u.req.m_url_impl, scheme, scheme_wks_idx, scheme_len, true);
headers->field_delete(field);
@@ -440,7 +450,7 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
// :authority
if (MIMEField *field = headers->field_find(HTTP2_VALUE_AUTHORITY, HTTP2_LEN_AUTHORITY);
- field != nullptr && field->value_is_valid()) {
+ field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
int authority_len;
const char *authority = field->value_get(&authority_len);
@@ -452,7 +462,8 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
}
// :path
- if (MIMEField *field = headers->field_find(HTTP2_VALUE_PATH, HTTP2_LEN_PATH); field != nullptr && field->value_is_valid()) {
+ if (MIMEField *field = headers->field_find(HTTP2_VALUE_PATH, HTTP2_LEN_PATH);
+ field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
int path_len;
const char *path = field->value_get(&path_len);
@@ -470,7 +481,8 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
}
// :method
- if (MIMEField *field = headers->field_find(HTTP2_VALUE_METHOD, HTTP2_LEN_METHOD); field != nullptr && field->value_is_valid()) {
+ if (MIMEField *field = headers->field_find(HTTP2_VALUE_METHOD, HTTP2_LEN_METHOD);
+ field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
int method_len;
const char *method = field->value_get(&method_len);
@@ -500,7 +512,7 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
// Check validity of all names and values
MIMEFieldIter iter;
for (auto *mf = headers->iter_get_first(&iter); mf != nullptr; mf = headers->iter_get_next(&iter)) {
- if (!mf->name_is_valid() || !mf->value_is_valid()) {
+ if (!mf->name_is_valid(is_control_BIT | is_ws_BIT) || !mf->value_is_valid()) {
return PARSE_RESULT_ERROR;
}
}
diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc
index a486d4567..1a924493e 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -140,7 +140,7 @@ rcv_data_frame(Http2ConnectionState &cstate, const Http2Frame &frame)
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
}
if (!stream->payload_length_is_valid()) {
- return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
"recv data bad payload length");
}
@@ -385,6 +385,12 @@ rcv_headers_frame(Http2ConnectionState &cstate, const Http2Frame &frame)
}
}
+ // Check Content-Length & payload length when END_STREAM flag is true
+ if (stream->recv_end_stream && !stream->payload_length_is_valid()) {
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
+ "recv data bad payload length");
+ }
+
// Set up the State Machine
if (!empty_request) {
SCOPED_MUTEX_LOCK(stream_lock, stream->mutex, this_ethread());
@@ -944,6 +950,12 @@ rcv_continuation_frame(Http2ConnectionState &cstate, const Http2Frame &frame)
}
}
+ // Check Content-Length & payload length when END_STREAM flag is true
+ if (stream->recv_end_stream && !stream->payload_length_is_valid()) {
+ return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR,
+ "recv data bad payload length");
+ }
+
// Set up the State Machine
SCOPED_MUTEX_LOCK(stream_lock, stream->mutex, this_ethread());
stream->mark_milestone(Http2StreamMilestone::START_TXN);
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index 5c7957734..b06c822df 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -233,7 +233,11 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
this->_http_sm_id = this->_sm->sm_id;
// Convert header to HTTP/1.1 format
- http2_convert_header_from_2_to_1_1(&_req_header);
+ if (http2_convert_header_from_2_to_1_1(&_req_header) == PARSE_RESULT_ERROR) {
+ // There's no way to cause Bad Request directly at this time.
+ // Set an invalid method so it causes an error later.
+ _req_header.method_set("\xffVOID", 1);
+ }
// Write header to a buffer. Borrowing logic from HttpSM::write_header_into_buffer.
// Seems like a function like this ought to be in HTTPHdr directly
diff --git a/tests/gold_tests/headers/gold/invalid_character_in_te_value.gold b/tests/gold_tests/headers/gold/invalid_character_in_te_value.gold
new file mode 100644
index 000000000..30a27d819
--- /dev/null
+++ b/tests/gold_tests/headers/gold/invalid_character_in_te_value.gold
@@ -0,0 +1,23 @@
+HTTP/1.1 400 Invalid HTTP Request
+Date:``
+Connection: close
+Server:``
+Cache-Control: no-store
+Content-Type: text/html
+Content-Language: en
+Content-Length:``
+
+<HTML>
+<HEAD>
+<TITLE>Bad Request</TITLE>
+</HEAD>
+
+<BODY BGCOLOR="white" FGCOLOR="black">
+<H1>Bad Request</H1>
+<HR>
+
+<FONT FACE="Helvetica,Arial"><B>
+Description: Could not process this request.
+</B></FONT>
+<HR>
+</BODY>
diff --git a/tests/gold_tests/headers/good_request_after_bad.test.py b/tests/gold_tests/headers/good_request_after_bad.test.py
index f65d54284..7502a75f5 100644
--- a/tests/gold_tests/headers/good_request_after_bad.test.py
+++ b/tests/gold_tests/headers/good_request_after_bad.test.py
@@ -92,6 +92,12 @@ tr.Processes.Default.Command = 'printf "GET / HTTP/1.1\r\nhost: bob\r\ntransfer-
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = 'gold/bad_te_value.gold'
+tr = Test.AddTestRun("Another unsupported Transfer Encoding value")
+tr.Processes.Default.Command = 'printf "GET / HTTP/1.1\r\nhost: bob\r\ntransfer-encoding: \x08chunked\r\n\r\nGET / HTTP/1.1\r\nHost: boa\r\n\r\n" | nc 127.0.0.1 {}'.format(
+ ts.Variables.port)
+tr.Processes.Default.ReturnCode = 0
+tr.Processes.Default.Streams.stdout = 'gold/invalid_character_in_te_value.gold'
+
# TRACE request with a body
tr = Test.AddTestRun("Trace request with a body")
tr.Processes.Default.Command = 'printf "TRACE /foo HTTP/1.1\r\nHost: bob\r\nContent-length:2\r\n\r\nokGET / HTTP/1.1\r\nHost: boa\r\n\r\n" | nc 127.0.0.1 {}'.format(