Skip to content

Commit 0ca9ef5

Browse files
masaori335maskit
andauthored
[8.1.x] Backport HTTP Validations (#9015)
* Fail fast on HTTP/2 header validation (#9009) (cherry picked from commit eaef5e8) 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 c56f872) 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 2f363d9) 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 e921228) * Ignore POST request case from a check for background fill (#9013) (cherry picked from commit 1f3e111) Co-authored-by: Masakazu Kitajo <maskit@apache.org>
1 parent 4da63a6 commit 0ca9ef5

File tree

16 files changed

+332
-31
lines changed

16 files changed

+332
-31
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ iocore/hostdb/test_RefCountCache
104104

105105
proxy/hdrs/test_mime
106106
proxy/hdrs/test_Huffmancode
107+
proxy/hdrs/test_proxy_hdrs
107108
proxy/hdrs/test_XPACK
108109
proxy/http/test_ForwardedConfig
109110
proxy/http2/test_libhttp2

include/tscore/ParseRules.h

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ class ParseRules
126126
static CTypeResult is_empty(char c); // wslfcr,#
127127
static CTypeResult is_alnum(char c); // 0-9,A-Z,a-z
128128
static CTypeResult is_space(char c); // ' ' HT,VT,NP,CR,LF
129-
static CTypeResult is_control(char c); // 0-31 127
129+
static CTypeResult is_control(char c); // 0x00-0x08, 0x0a-0x1f, 0x7f
130130
static CTypeResult is_mime_sep(char c); // ()<>,;\"/[]?{} \t
131131
static CTypeResult is_http_field_name(char c); // not : or mime_sep except for @
132132
static CTypeResult is_http_field_value(char c); // not CR, LF, comma, or "
@@ -665,14 +665,24 @@ ParseRules::is_space(char c)
665665
#endif
666666
}
667667

668+
/**
669+
Return true if @c is a control char except HTAB(0x09) and SP(0x20).
670+
If you need to check @c is HTAB or SP, use `ParseRules::is_ws`.
671+
*/
668672
inline CTypeResult
669673
ParseRules::is_control(char c)
670674
{
671675
#ifndef COMPILE_PARSE_RULES
672676
return (parseRulesCType[(unsigned char)c] & is_control_BIT);
673677
#else
674-
if (((unsigned char)c) < 32 || ((unsigned char)c) == 127)
678+
if (c == CHAR_HT || c == CHAR_SP) {
679+
return false;
680+
}
681+
682+
if (((unsigned char)c) < 0x20 || c == 0x7f) {
675683
return true;
684+
}
685+
676686
return false;
677687
#endif
678688
}

proxy/hdrs/MIME.cc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2693,6 +2693,21 @@ mime_parser_parse(MIMEParser *parser, HdrHeap *heap, MIMEHdrImpl *mh, const char
26932693

26942694
int field_name_wks_idx = hdrtoken_tokenize(field_name_first, field_name_length);
26952695

2696+
if (field_name_wks_idx < 0) {
2697+
for (int i = 0; i < field_name_length; ++i) {
2698+
if (ParseRules::is_control(field_name_first[i])) {
2699+
return PARSE_RESULT_ERROR;
2700+
}
2701+
}
2702+
}
2703+
2704+
// RFC 9110 Section 5.5. Field Values
2705+
for (int i = 0; i < field_value_length; ++i) {
2706+
if (ParseRules::is_control(field_value_first[i])) {
2707+
return PARSE_RESULT_ERROR;
2708+
}
2709+
}
2710+
26962711
///////////////////////////////////////////
26972712
// build and insert the new field object //
26982713
///////////////////////////////////////////

proxy/hdrs/MIME.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ struct MIMEField {
164164
int value_get_comma_list(StrList *list) const;
165165

166166
void name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length);
167-
bool name_is_valid() const;
167+
bool name_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
168168

169169
void value_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length);
170170
void value_set_int(HdrHeap *heap, MIMEHdrImpl *mh, int32_t value);
@@ -176,7 +176,7 @@ struct MIMEField {
176176
// Other separators (e.g. ';' in Set-cookie/Cookie) are also possible
177177
void value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int length, bool prepend_comma = false,
178178
const char separator = ',');
179-
bool value_is_valid() const;
179+
bool value_is_valid(uint32_t invalid_char_bits = is_control_BIT) const;
180180
int has_dups() const;
181181
};
182182

@@ -771,13 +771,13 @@ MIMEField::name_set(HdrHeap *heap, MIMEHdrImpl *mh, const char *name, int length
771771
-------------------------------------------------------------------------*/
772772

773773
inline bool
774-
MIMEField::name_is_valid() const
774+
MIMEField::name_is_valid(uint32_t invalid_char_bits) const
775775
{
776776
const char *name;
777777
int length;
778778

779779
for (name = name_get(&length); length > 0; length--) {
780-
if (ParseRules::is_control(name[length - 1])) {
780+
if (ParseRules::is_type(name[length - 1], invalid_char_bits)) {
781781
return false;
782782
}
783783
}
@@ -878,13 +878,13 @@ MIMEField::value_append(HdrHeap *heap, MIMEHdrImpl *mh, const char *value, int l
878878
-------------------------------------------------------------------------*/
879879

880880
inline bool
881-
MIMEField::value_is_valid() const
881+
MIMEField::value_is_valid(uint32_t invalid_char_bits) const
882882
{
883883
const char *value;
884884
int length;
885885

886886
for (value = value_get(&length); length > 0; length--) {
887-
if (ParseRules::is_control(value[length - 1])) {
887+
if (ParseRules::is_type(value[length - 1], invalid_char_bits)) {
888888
return false;
889889
}
890890
}

proxy/hdrs/Makefile.am

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,33 @@ load_http_hdr_LDADD = -L. -lhdrs \
6767
@LIBTCL@
6868

6969
check_PROGRAMS = \
70+
test_proxy_hdrs \
7071
test_mime \
7172
test_Huffmancode \
7273
test_XPACK
7374

7475
TESTS = $(check_PROGRAMS)
7576

77+
test_proxy_hdrs_CPPFLAGS = $(AM_CPPFLAGS) \
78+
-I$(abs_top_srcdir)/tests/include
79+
80+
test_proxy_hdrs_SOURCES = \
81+
unit_tests/unit_test_main.cc \
82+
unit_tests/test_URL.cc \
83+
unit_tests/test_Hdrs.cc
84+
85+
test_proxy_hdrs_LDADD = \
86+
$(top_builddir)/src/tscore/libtscore.la \
87+
-L. -lhdrs \
88+
$(top_builddir)/src/tscore/libtscore.la \
89+
$(top_builddir)/src/tscpp/util/libtscpputil.la \
90+
$(top_builddir)/iocore/eventsystem/libinkevent.a \
91+
$(top_builddir)/lib/records/librecords_p.a \
92+
$(top_builddir)/mgmt/libmgmt_p.la \
93+
$(top_builddir)/proxy/shared/libUglyLogStubs.a \
94+
@HWLOC_LIBS@ \
95+
@LIBCAP@
96+
7697
test_mime_LDADD = -L. -lhdrs \
7798
$(top_builddir)/src/tscore/libtscore.la \
7899
$(top_builddir)/src/tscpp/util/libtscpputil.la \

proxy/hdrs/URL.cc

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,36 @@ validate_host_name(std::string_view addr)
114114
return std::all_of(addr.begin(), addr.end(), &is_host_char);
115115
}
116116

117+
/**
118+
Checks if the (un-well-known) scheme is valid
119+
120+
RFC 3986 Section 3.1
121+
These are the valid characters in a scheme:
122+
scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
123+
return an error if there is another character in the scheme
124+
*/
125+
bool
126+
validate_scheme(std::string_view scheme)
127+
{
128+
if (scheme.empty()) {
129+
return false;
130+
}
131+
132+
if (!ParseRules::is_alpha(scheme[0])) {
133+
return false;
134+
}
135+
136+
for (size_t i = 0; i < scheme.size(); ++i) {
137+
const char &c = scheme[i];
138+
139+
if (!(ParseRules::is_alnum(c) != 0 || c == '+' || c == '-' || c == '.')) {
140+
return false;
141+
}
142+
}
143+
144+
return true;
145+
}
146+
117147
/*-------------------------------------------------------------------------
118148
-------------------------------------------------------------------------*/
119149

@@ -1140,19 +1170,9 @@ url_parse_scheme(HdrHeap *heap, URLImpl *url, const char **start, const char *en
11401170

11411171
if (!(scheme_wks_idx > 0 && hdrtoken_wks_to_token_type(scheme_wks) == HDRTOKEN_TYPE_SCHEME)) {
11421172
// Unknown scheme, validate the scheme
1143-
1144-
// RFC 3986 Section 3.1
1145-
// These are the valid characters in a scheme:
1146-
// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
1147-
// return an error if there is another character in the scheme
1148-
if (!ParseRules::is_alpha(*scheme_start)) {
1173+
if (!validate_scheme({scheme_start, static_cast<size_t>(scheme_end - scheme_start)})) {
11491174
return PARSE_RESULT_ERROR;
11501175
}
1151-
for (cur = scheme_start + 1; cur < scheme_end; ++cur) {
1152-
if (!(ParseRules::is_alnum(*cur) != 0 || *cur == '+' || *cur == '-' || *cur == '.')) {
1153-
return PARSE_RESULT_ERROR;
1154-
}
1155-
}
11561176
}
11571177
url_scheme_set(heap, url, scheme_start, scheme_wks_idx, scheme_end - scheme_start, copy_strings_p);
11581178
}

proxy/hdrs/URL.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,8 @@ void url_adjust(MarshalXlate *str_xlate, int num_xlate);
156156

157157
/* Public */
158158
bool validate_host_name(std::string_view addr);
159+
bool validate_scheme(std::string_view scheme);
160+
159161
void url_init();
160162

161163
URLImpl *url_create(HdrHeap *heap);

proxy/hdrs/unit_tests/test_Hdrs.cc

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/** @file
2+
3+
Catch-based unit tests for various header logic.
4+
This replaces the old regression tests in HdrTest.cc.
5+
6+
@section license License
7+
8+
Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements.
9+
See the NOTICE file distributed with this work for additional information regarding copyright
10+
ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
11+
"License"); you may not use this file except in compliance with the License. You may obtain a
12+
copy of the License at
13+
14+
http://www.apache.org/licenses/LICENSE-2.0
15+
16+
Unless required by applicable law or agreed to in writing, software distributed under the License
17+
is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
18+
or implied. See the License for the specific language governing permissions and limitations under
19+
the License.
20+
*/
21+
22+
#include <cstdio>
23+
24+
#include "catch.hpp"
25+
26+
#include "MIME.h"
27+
28+
TEST_CASE("HdrTest", "[proxy][hdrtest]")
29+
{
30+
mime_init();
31+
32+
SECTION("Field Char Check")
33+
{
34+
static struct {
35+
std::string_view line;
36+
ParseResult expected;
37+
} test_cases[] = {
38+
////
39+
// Field Name
40+
{"Content-Length: 10\r\n", PARSE_RESULT_CONT},
41+
{"Content-Length\x0b: 10\r\n", PARSE_RESULT_ERROR},
42+
////
43+
// Field Value
44+
// SP
45+
{"Content-Length: 10\r\n", PARSE_RESULT_CONT},
46+
{"Foo: ab cd\r\n", PARSE_RESULT_CONT},
47+
// HTAB
48+
{"Foo: ab\td/cd\r\n", PARSE_RESULT_CONT},
49+
// VCHAR
50+
{"Foo: ab\x21/cd\r\n", PARSE_RESULT_CONT},
51+
{"Foo: ab\x7e/cd\r\n", PARSE_RESULT_CONT},
52+
// DEL
53+
{"Foo: ab\x7f/cd\r\n", PARSE_RESULT_ERROR},
54+
// obs-text
55+
{"Foo: ab\x80/cd\r\n", PARSE_RESULT_CONT},
56+
{"Foo: ab\xff/cd\r\n", PARSE_RESULT_CONT},
57+
// control char
58+
{"Content-Length: 10\x0b\r\n", PARSE_RESULT_ERROR},
59+
{"Content-Length:\x0b 10\r\n", PARSE_RESULT_ERROR},
60+
{"Foo: ab\x1d/cd\r\n", PARSE_RESULT_ERROR},
61+
};
62+
63+
MIMEHdr hdr;
64+
MIMEParser parser;
65+
mime_parser_init(&parser);
66+
67+
for (const auto &t : test_cases) {
68+
mime_parser_clear(&parser);
69+
70+
const char *start = t.line.data();
71+
const char *end = start + t.line.size();
72+
73+
int r = hdr.parse(&parser, &start, end, false, false);
74+
if (r != t.expected) {
75+
std::printf("Expected %s is %s, but not", t.line.data(), t.expected == PARSE_RESULT_ERROR ? "invalid" : "valid");
76+
CHECK(false);
77+
}
78+
}
79+
}
80+
}

proxy/hdrs/unit_tests/test_URL.cc

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/** @file
2+
3+
Catch-based unit tests for URL
4+
5+
@section license License
6+
7+
Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements.
8+
See the NOTICE file distributed with this work for additional information regarding copyright
9+
ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
10+
"License"); you may not use this file except in compliance with the License. You may obtain a
11+
copy of the License at
12+
13+
http://www.apache.org/licenses/LICENSE-2.0
14+
15+
Unless required by applicable law or agreed to in writing, software distributed under the License
16+
is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
17+
or implied. See the License for the specific language governing permissions and limitations under
18+
the License.
19+
*/
20+
21+
#include <cstdio>
22+
23+
#include "catch.hpp"
24+
25+
#include "URL.h"
26+
27+
TEST_CASE("Validate Scheme", "[proxy][validscheme]")
28+
{
29+
static const struct {
30+
std::string_view text;
31+
bool valid;
32+
} scheme_test_cases[] = {{"http", true}, {"https", true}, {"example", true}, {"example.", true},
33+
{"example++", true}, {"example--.", true}, {"++example", false}, {"--example", false},
34+
{".example", false}, {"example://", false}};
35+
36+
for (auto i : scheme_test_cases) {
37+
// it's pretty hard to debug with
38+
// CHECK(validate_scheme(i.text) == i.valid);
39+
40+
std::string_view text = i.text;
41+
if (validate_scheme(text) != i.valid) {
42+
std::printf("Validation of scheme: \"%s\", expected %s, but not\n", text.data(), (i.valid ? "true" : "false"));
43+
CHECK(false);
44+
}
45+
}
46+
}
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/** @file
2+
3+
This file used for catch based tests. It is the main() stub.
4+
5+
@section license License
6+
7+
Licensed to the Apache Software Foundation (ASF) under one
8+
or more contributor license agreements. See the NOTICE file
9+
distributed with this work for additional information
10+
regarding copyright ownership. The ASF licenses this file
11+
to you under the Apache License, Version 2.0 (the
12+
"License"); you may not use this file except in compliance
13+
with the License. You may obtain a copy of the License at
14+
15+
http://www.apache.org/licenses/LICENSE-2.0
16+
17+
Unless required by applicable law or agreed to in writing, software
18+
distributed under the License is distributed on an "AS IS" BASIS,
19+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
20+
See the License for the specific language governing permissions and
21+
limitations under the License.
22+
*/
23+
24+
#include "HTTP.h"
25+
26+
#define CATCH_CONFIG_RUNNER
27+
#include "catch.hpp"
28+
29+
extern int cmd_disable_pfreelist;
30+
31+
int
32+
main(int argc, char *argv[])
33+
{
34+
// No thread setup, forbid use of thread local allocators.
35+
cmd_disable_pfreelist = true;
36+
// Get all of the HTTP WKS items populated.
37+
http_init();
38+
39+
int result = Catch::Session().run(argc, argv);
40+
41+
// global clean-up...
42+
43+
return result;
44+
}

0 commit comments

Comments
 (0)