You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by ez...@apache.org on 2022/08/09 03:19:18 UTC
[trafficserver] branch 8.1.x updated: [8.1.x] Backport HTTP Validations (#9015)
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 <ma...@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 <ma...@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(