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 <masa...@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 <mas...@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(