Skip to content

Commit

Permalink
Fail fast on HTTP/2 header validation (#9009)
Browse files Browse the repository at this point in the history
Co-authored-by: Masakazu Kitajo <maskit@apache.org>
(cherry picked from commit eaef5e8)
  • Loading branch information
masaori335 authored and zwoop committed Aug 10, 2022
1 parent 20fb586 commit 07de4d3
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 12 deletions.
12 changes: 6 additions & 6 deletions proxy/hdrs/MIME.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
};

Expand Down Expand Up @@ -836,13 +836,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;
}
}
Expand Down Expand Up @@ -945,13 +945,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;
}
}
Expand Down
13 changes: 8 additions & 5 deletions proxy/http2/HTTP2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,8 @@ 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;
Expand All @@ -453,7 +454,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);

Expand All @@ -465,7 +466,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);

Expand All @@ -483,7 +485,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);

Expand Down Expand Up @@ -512,7 +515,7 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)

// Check validity of all names and values
for (auto &mf : *headers) {
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;
}
}
Expand Down
6 changes: 5 additions & 1 deletion proxy/http2/Http2Stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,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
Expand Down

0 comments on commit 07de4d3

Please sign in to comment.