This is an automated email from the ASF dual-hosted git repository. eze pushed a commit to branch 8.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.1.x by this push: new 0ca9ef5ab [8.1.x] Backport HTTP Validations (#9015) 0ca9ef5ab is described below commit 0ca9ef5abc8a535d05150ebc7c16bbfa4e982d16 Author: Masaori Koshiba <masa...@apache.org> AuthorDate: Tue Aug 9 12:19:13 2022 +0900 [8.1.x] Backport HTTP Validations (#9015) * 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/hdrs/unit_tests/test_URL.cc proxy/http2/HTTP2.cc To compile unit tests, Makefile.am is changed too. * Add control char check in MIME Parser (#9011) (cherry picked from commit 2f363d973b321e58297fa356a5aab66b6b6788c1) Conflicts: proxy/hdrs/Makefile.am proxy/hdrs/unit_tests/test_Hdrs.cc 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 to run unit test: proxy/hdrs/unit_tests/unit_test_main.cc * 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> --- .gitignore | 1 + include/tscore/ParseRules.h | 14 +++- proxy/hdrs/MIME.cc | 15 ++++ proxy/hdrs/MIME.h | 12 ++-- proxy/hdrs/Makefile.am | 21 ++++++ proxy/hdrs/URL.cc | 42 +++++++++--- proxy/hdrs/URL.h | 2 + proxy/hdrs/unit_tests/test_Hdrs.cc | 80 ++++++++++++++++++++++ proxy/hdrs/unit_tests/test_URL.cc | 46 +++++++++++++ proxy/hdrs/unit_tests/unit_test_main.cc | 44 ++++++++++++ proxy/http/HttpTunnel.h | 12 ++-- proxy/http2/HTTP2.cc | 25 +++++-- 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 ++ 16 files changed, 332 insertions(+), 31 deletions(-) diff --git a/.gitignore b/.gitignore index 6df4c6302..8fda6f1cb 100644 --- a/.gitignore +++ b/.gitignore @@ -104,6 +104,7 @@ iocore/hostdb/test_RefCountCache proxy/hdrs/test_mime proxy/hdrs/test_Huffmancode +proxy/hdrs/test_proxy_hdrs proxy/hdrs/test_XPACK proxy/http/test_ForwardedConfig proxy/http2/test_libhttp2 diff --git a/include/tscore/ParseRules.h b/include/tscore/ParseRules.h index d8fe31e51..eff74a16e 100644 --- a/include/tscore/ParseRules.h +++ b/include/tscore/ParseRules.h @@ -126,7 +126,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 " @@ -665,14 +665,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 926564388..f010b6ad5 100644 --- a/proxy/hdrs/MIME.cc +++ b/proxy/hdrs/MIME.cc @@ -2693,6 +2693,21 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap, MIMEHdrImpl *mh, const char int field_name_wks_idx = hdrtoken_tokenize(field_name_first, field_name_length); + if (field_name_wks_idx < 0) { + for (int i = 0; i < field_name_length; ++i) { + if (ParseRules::is_control(field_name_first[i])) { + return PARSE_RESULT_ERROR; + } + } + } + + // RFC 9110 Section 5.5. Field Values + for (int i = 0; i < field_value_length; ++i) { + if (ParseRules::is_control(field_value_first[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 b0e9ac17b..14271ded9 100644 --- a/proxy/hdrs/MIME.h +++ b/proxy/hdrs/MIME.h @@ -164,7 +164,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); @@ -176,7 +176,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; }; @@ -771,13 +771,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; } } @@ -878,13 +878,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/Makefile.am b/proxy/hdrs/Makefile.am index bc060e527..a84429b67 100644 --- a/proxy/hdrs/Makefile.am +++ b/proxy/hdrs/Makefile.am @@ -67,12 +67,33 @@ load_http_hdr_LDADD = -L. -lhdrs \ @LIBTCL@ check_PROGRAMS = \ + test_proxy_hdrs \ test_mime \ test_Huffmancode \ test_XPACK TESTS = $(check_PROGRAMS) +test_proxy_hdrs_CPPFLAGS = $(AM_CPPFLAGS) \ + -I$(abs_top_srcdir)/tests/include + +test_proxy_hdrs_SOURCES = \ + unit_tests/unit_test_main.cc \ + unit_tests/test_URL.cc \ + unit_tests/test_Hdrs.cc + +test_proxy_hdrs_LDADD = \ + $(top_builddir)/src/tscore/libtscore.la \ + -L. -lhdrs \ + $(top_builddir)/src/tscore/libtscore.la \ + $(top_builddir)/src/tscpp/util/libtscpputil.la \ + $(top_builddir)/iocore/eventsystem/libinkevent.a \ + $(top_builddir)/lib/records/librecords_p.a \ + $(top_builddir)/mgmt/libmgmt_p.la \ + $(top_builddir)/proxy/shared/libUglyLogStubs.a \ + @HWLOC_LIBS@ \ + @LIBCAP@ + test_mime_LDADD = -L. -lhdrs \ $(top_builddir)/src/tscore/libtscore.la \ $(top_builddir)/src/tscpp/util/libtscpputil.la \ diff --git a/proxy/hdrs/URL.cc b/proxy/hdrs/URL.cc index 48bac50be..9edf1a43c 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; +} + /*------------------------------------------------------------------------- -------------------------------------------------------------------------*/ @@ -1140,19 +1170,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 4ab99e36d..78d679e67 100644 --- a/proxy/hdrs/URL.h +++ b/proxy/hdrs/URL.h @@ -156,6 +156,8 @@ void url_adjust(MarshalXlate *str_xlate, int num_xlate); /* 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 new file mode 100644 index 000000000..5e63dea2a --- /dev/null +++ b/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -0,0 +1,80 @@ +/** @file + + Catch-based unit tests for various header logic. + This replaces the old regression tests in HdrTest.cc. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. + See the NOTICE file distributed with this work for additional information regarding copyright + ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance with the License. You may obtain a + copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software distributed under the License + is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + or implied. See the License for the specific language governing permissions and limitations under + the License. + */ + +#include <cstdio> + +#include "catch.hpp" + +#include "MIME.h" + +TEST_CASE("HdrTest", "[proxy][hdrtest]") +{ + mime_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); + if (r != t.expected) { + std::printf("Expected %s is %s, but not", t.line.data(), t.expected == PARSE_RESULT_ERROR ? "invalid" : "valid"); + CHECK(false); + } + } + } +} diff --git a/proxy/hdrs/unit_tests/test_URL.cc b/proxy/hdrs/unit_tests/test_URL.cc new file mode 100644 index 000000000..b022d068e --- /dev/null +++ b/proxy/hdrs/unit_tests/test_URL.cc @@ -0,0 +1,46 @@ +/** @file + + Catch-based unit tests for URL + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. + See the NOTICE file distributed with this work for additional information regarding copyright + ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance with the License. You may obtain a + copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software distributed under the License + is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + or implied. See the License for the specific language governing permissions and limitations under + the License. + */ + +#include <cstdio> + +#include "catch.hpp" + +#include "URL.h" + +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); + } + } +} diff --git a/proxy/hdrs/unit_tests/unit_test_main.cc b/proxy/hdrs/unit_tests/unit_test_main.cc new file mode 100644 index 000000000..636c5ccbe --- /dev/null +++ b/proxy/hdrs/unit_tests/unit_test_main.cc @@ -0,0 +1,44 @@ +/** @file + + This file used for catch based tests. It is the main() stub. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "HTTP.h" + +#define CATCH_CONFIG_RUNNER +#include "catch.hpp" + +extern int cmd_disable_pfreelist; + +int +main(int argc, char *argv[]) +{ + // No thread setup, forbid use of thread local allocators. + cmd_disable_pfreelist = true; + // Get all of the HTTP WKS items populated. + http_init(); + + int result = Catch::Session().run(argc, argv); + + // global clean-up... + + return result; +} diff --git a/proxy/http/HttpTunnel.h b/proxy/http/HttpTunnel.h index 9cc515530..4d2f6f0c1 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 84f66b25c..f928ea33a 100644 --- a/proxy/http2/HTTP2.cc +++ b/proxy/http2/HTTP2.cc @@ -413,19 +413,33 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers) int scheme_len, authority_len, path_len, method_len; // Get values of :scheme, :authority and :path to assemble requested URL - if ((field = headers->field_find(HTTP2_VALUE_SCHEME, HTTP2_LEN_SCHEME)) != nullptr && field->value_is_valid()) { + if ((field = headers->field_find(HTTP2_VALUE_SCHEME, HTTP2_LEN_SCHEME)) != nullptr && + field->value_is_valid(is_control_BIT | is_ws_BIT)) { 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; + } + } } else { return PARSE_RESULT_ERROR; } - if ((field = headers->field_find(HTTP2_VALUE_AUTHORITY, HTTP2_LEN_AUTHORITY)) != nullptr && field->value_is_valid()) { + if ((field = headers->field_find(HTTP2_VALUE_AUTHORITY, HTTP2_LEN_AUTHORITY)) != nullptr && + field->value_is_valid(is_control_BIT | is_ws_BIT)) { authority = field->value_get(&authority_len); } else { return PARSE_RESULT_ERROR; } - if ((field = headers->field_find(HTTP2_VALUE_PATH, HTTP2_LEN_PATH)) != nullptr && field->value_is_valid()) { + if ((field = headers->field_find(HTTP2_VALUE_PATH, HTTP2_LEN_PATH)) != nullptr && + field->value_is_valid(is_control_BIT | is_ws_BIT)) { path = field->value_get(&path_len); } else { return PARSE_RESULT_ERROR; @@ -445,7 +459,8 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers) arena.str_free(url); // Get value of :method - if ((field = headers->field_find(HTTP2_VALUE_METHOD, HTTP2_LEN_METHOD)) != nullptr && field->value_is_valid()) { + if ((field = headers->field_find(HTTP2_VALUE_METHOD, HTTP2_LEN_METHOD)) != nullptr && + field->value_is_valid(is_control_BIT | is_ws_BIT)) { method = field->value_get(&method_len); int method_wks_idx = hdrtoken_tokenize(method, method_len); @@ -487,7 +502,7 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers) // Check validity of all names and values MIMEFieldIter iter; for (const MIMEField *field = headers->iter_get_first(&iter); field != nullptr; field = headers->iter_get_next(&iter)) { - if (!field->name_is_valid() || !field->value_is_valid()) { + if (!field->name_is_valid(is_control_BIT | is_ws_BIT) || !field->value_is_valid()) { return PARSE_RESULT_ERROR; } } diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 308c5d5d5..c2a7cfbcd 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/proxy/http2/Http2ConnectionState.cc @@ -136,7 +136,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"); } @@ -370,6 +370,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()); @@ -930,6 +936,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 46486bc1e..8a4940bd2 100644 --- a/proxy/http2/Http2Stream.cc +++ b/proxy/http2/Http2Stream.cc @@ -165,7 +165,11 @@ Http2Stream::send_request(Http2ConnectionState &cstate) this->_http_sm_id = this->current_reader->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 3a99d412a..cf5c25b8c 100644 --- a/tests/gold_tests/headers/good_request_after_bad.test.py +++ b/tests/gold_tests/headers/good_request_after_bad.test.py @@ -80,6 +80,12 @@ tr.Processes.Default.Command = 'printf "GET / HTTP/1.1\r\nHost : bob\r\n\r\nGET tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = 'gold/bad_good_request.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' + # Commenting out a bunch of tests on master whose fixes are not in 8.1.x. #tr = Test.AddTestRun("Bad protocol number") #tr.Processes.Default.Command = 'printf "GET / HTTP/11.1\r\nhost: bob\r\n\r\nGET / HTTP/1.1\r\nHost: boa\r\n\r\n" | nc 127.0.0.1 {}'.format(