diff --git a/bazel/foreign_cc/nghttp2.patch b/bazel/foreign_cc/nghttp2.patch index e7b001673a56d..bfe51d07dad0d 100644 --- a/bazel/foreign_cc/nghttp2.patch +++ b/bazel/foreign_cc/nghttp2.patch @@ -15,3 +15,226 @@ index 35c77d1d..47bd63f5 100644 endif() # AC_TYPE_UINT8_T # AC_TYPE_UINT16_T +diff --git a/doc/Makefile.am b/doc/Makefile.am +index c17d93382..4d73cef50 100644 +--- a/doc/Makefile.am ++++ b/doc/Makefile.am +@@ -27,6 +27,7 @@ APIDOCS= \ + macros.rst \ + enums.rst \ + types.rst \ ++ nghttp2_check_authority.rst \ + nghttp2_check_header_name.rst \ + nghttp2_check_header_value.rst \ + nghttp2_hd_deflate_bound.rst \ +diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h +index 313fb23da..e3aeb9fed 100644 +--- a/lib/includes/nghttp2/nghttp2.h ++++ b/lib/includes/nghttp2/nghttp2.h +@@ -4769,6 +4769,19 @@ NGHTTP2_EXTERN int nghttp2_check_header_name(const uint8_t *name, size_t len); + */ + NGHTTP2_EXTERN int nghttp2_check_header_value(const uint8_t *value, size_t len); + ++/** ++ * @function ++ * ++ * Returns nonzero if the |value| which is supposed to the value of ++ * :authority or host header field is valid according to ++ * https://tools.ietf.org/html/rfc3986#section-3.2 ++ * ++ * |value| is valid if it merely consists of the allowed characters. ++ * In particular, it does not check whether |value| follows the syntax ++ * of authority. ++ */ ++NGHTTP2_EXTERN int nghttp2_check_authority(const uint8_t *value, size_t len); ++ + /* HPACK API */ + + struct nghttp2_hd_deflater; +diff --git a/lib/nghttp2_helper.c b/lib/nghttp2_helper.c +index 81a8a0cf9..91136a619 100644 +--- a/lib/nghttp2_helper.c ++++ b/lib/nghttp2_helper.c +@@ -505,6 +505,84 @@ int nghttp2_check_header_value(const uint8_t *value, size_t len) { + return 1; + } + ++/* Generated by genauthroitychartbl.py */ ++static char VALID_AUTHORITY_CHARS[] = { ++ 0 /* NUL */, 0 /* SOH */, 0 /* STX */, 0 /* ETX */, ++ 0 /* EOT */, 0 /* ENQ */, 0 /* ACK */, 0 /* BEL */, ++ 0 /* BS */, 0 /* HT */, 0 /* LF */, 0 /* VT */, ++ 0 /* FF */, 0 /* CR */, 0 /* SO */, 0 /* SI */, ++ 0 /* DLE */, 0 /* DC1 */, 0 /* DC2 */, 0 /* DC3 */, ++ 0 /* DC4 */, 0 /* NAK */, 0 /* SYN */, 0 /* ETB */, ++ 0 /* CAN */, 0 /* EM */, 0 /* SUB */, 0 /* ESC */, ++ 0 /* FS */, 0 /* GS */, 0 /* RS */, 0 /* US */, ++ 0 /* SPC */, 1 /* ! */, 0 /* " */, 0 /* # */, ++ 1 /* $ */, 1 /* % */, 1 /* & */, 1 /* ' */, ++ 1 /* ( */, 1 /* ) */, 1 /* * */, 1 /* + */, ++ 1 /* , */, 1 /* - */, 1 /* . */, 0 /* / */, ++ 1 /* 0 */, 1 /* 1 */, 1 /* 2 */, 1 /* 3 */, ++ 1 /* 4 */, 1 /* 5 */, 1 /* 6 */, 1 /* 7 */, ++ 1 /* 8 */, 1 /* 9 */, 1 /* : */, 1 /* ; */, ++ 0 /* < */, 1 /* = */, 0 /* > */, 0 /* ? */, ++ 1 /* @ */, 1 /* A */, 1 /* B */, 1 /* C */, ++ 1 /* D */, 1 /* E */, 1 /* F */, 1 /* G */, ++ 1 /* H */, 1 /* I */, 1 /* J */, 1 /* K */, ++ 1 /* L */, 1 /* M */, 1 /* N */, 1 /* O */, ++ 1 /* P */, 1 /* Q */, 1 /* R */, 1 /* S */, ++ 1 /* T */, 1 /* U */, 1 /* V */, 1 /* W */, ++ 1 /* X */, 1 /* Y */, 1 /* Z */, 1 /* [ */, ++ 0 /* \ */, 1 /* ] */, 0 /* ^ */, 1 /* _ */, ++ 0 /* ` */, 1 /* a */, 1 /* b */, 1 /* c */, ++ 1 /* d */, 1 /* e */, 1 /* f */, 1 /* g */, ++ 1 /* h */, 1 /* i */, 1 /* j */, 1 /* k */, ++ 1 /* l */, 1 /* m */, 1 /* n */, 1 /* o */, ++ 1 /* p */, 1 /* q */, 1 /* r */, 1 /* s */, ++ 1 /* t */, 1 /* u */, 1 /* v */, 1 /* w */, ++ 1 /* x */, 1 /* y */, 1 /* z */, 0 /* { */, ++ 0 /* | */, 0 /* } */, 1 /* ~ */, 0 /* DEL */, ++ 0 /* 0x80 */, 0 /* 0x81 */, 0 /* 0x82 */, 0 /* 0x83 */, ++ 0 /* 0x84 */, 0 /* 0x85 */, 0 /* 0x86 */, 0 /* 0x87 */, ++ 0 /* 0x88 */, 0 /* 0x89 */, 0 /* 0x8a */, 0 /* 0x8b */, ++ 0 /* 0x8c */, 0 /* 0x8d */, 0 /* 0x8e */, 0 /* 0x8f */, ++ 0 /* 0x90 */, 0 /* 0x91 */, 0 /* 0x92 */, 0 /* 0x93 */, ++ 0 /* 0x94 */, 0 /* 0x95 */, 0 /* 0x96 */, 0 /* 0x97 */, ++ 0 /* 0x98 */, 0 /* 0x99 */, 0 /* 0x9a */, 0 /* 0x9b */, ++ 0 /* 0x9c */, 0 /* 0x9d */, 0 /* 0x9e */, 0 /* 0x9f */, ++ 0 /* 0xa0 */, 0 /* 0xa1 */, 0 /* 0xa2 */, 0 /* 0xa3 */, ++ 0 /* 0xa4 */, 0 /* 0xa5 */, 0 /* 0xa6 */, 0 /* 0xa7 */, ++ 0 /* 0xa8 */, 0 /* 0xa9 */, 0 /* 0xaa */, 0 /* 0xab */, ++ 0 /* 0xac */, 0 /* 0xad */, 0 /* 0xae */, 0 /* 0xaf */, ++ 0 /* 0xb0 */, 0 /* 0xb1 */, 0 /* 0xb2 */, 0 /* 0xb3 */, ++ 0 /* 0xb4 */, 0 /* 0xb5 */, 0 /* 0xb6 */, 0 /* 0xb7 */, ++ 0 /* 0xb8 */, 0 /* 0xb9 */, 0 /* 0xba */, 0 /* 0xbb */, ++ 0 /* 0xbc */, 0 /* 0xbd */, 0 /* 0xbe */, 0 /* 0xbf */, ++ 0 /* 0xc0 */, 0 /* 0xc1 */, 0 /* 0xc2 */, 0 /* 0xc3 */, ++ 0 /* 0xc4 */, 0 /* 0xc5 */, 0 /* 0xc6 */, 0 /* 0xc7 */, ++ 0 /* 0xc8 */, 0 /* 0xc9 */, 0 /* 0xca */, 0 /* 0xcb */, ++ 0 /* 0xcc */, 0 /* 0xcd */, 0 /* 0xce */, 0 /* 0xcf */, ++ 0 /* 0xd0 */, 0 /* 0xd1 */, 0 /* 0xd2 */, 0 /* 0xd3 */, ++ 0 /* 0xd4 */, 0 /* 0xd5 */, 0 /* 0xd6 */, 0 /* 0xd7 */, ++ 0 /* 0xd8 */, 0 /* 0xd9 */, 0 /* 0xda */, 0 /* 0xdb */, ++ 0 /* 0xdc */, 0 /* 0xdd */, 0 /* 0xde */, 0 /* 0xdf */, ++ 0 /* 0xe0 */, 0 /* 0xe1 */, 0 /* 0xe2 */, 0 /* 0xe3 */, ++ 0 /* 0xe4 */, 0 /* 0xe5 */, 0 /* 0xe6 */, 0 /* 0xe7 */, ++ 0 /* 0xe8 */, 0 /* 0xe9 */, 0 /* 0xea */, 0 /* 0xeb */, ++ 0 /* 0xec */, 0 /* 0xed */, 0 /* 0xee */, 0 /* 0xef */, ++ 0 /* 0xf0 */, 0 /* 0xf1 */, 0 /* 0xf2 */, 0 /* 0xf3 */, ++ 0 /* 0xf4 */, 0 /* 0xf5 */, 0 /* 0xf6 */, 0 /* 0xf7 */, ++ 0 /* 0xf8 */, 0 /* 0xf9 */, 0 /* 0xfa */, 0 /* 0xfb */, ++ 0 /* 0xfc */, 0 /* 0xfd */, 0 /* 0xfe */, 0 /* 0xff */ ++}; ++ ++int nghttp2_check_authority(const uint8_t *value, size_t len) { ++ const uint8_t *last; ++ for (last = value + len; value != last; ++value) { ++ if (!VALID_AUTHORITY_CHARS[*value]) { ++ return 0; ++ } ++ } ++ return 1; ++} ++ + uint8_t *nghttp2_cpymem(uint8_t *dest, const void *src, size_t len) { + if (len == 0) { + return dest; +diff --git a/lib/nghttp2_http.c b/lib/nghttp2_http.c +index 8d9902998..62f57b6ae 100644 +--- a/lib/nghttp2_http.c ++++ b/lib/nghttp2_http.c +@@ -305,84 +305,6 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_hd_nv *nv, + return 0; + } + +-/* Generated by genauthroitychartbl.py */ +-static char VALID_AUTHORITY_CHARS[] = { +- 0 /* NUL */, 0 /* SOH */, 0 /* STX */, 0 /* ETX */, +- 0 /* EOT */, 0 /* ENQ */, 0 /* ACK */, 0 /* BEL */, +- 0 /* BS */, 0 /* HT */, 0 /* LF */, 0 /* VT */, +- 0 /* FF */, 0 /* CR */, 0 /* SO */, 0 /* SI */, +- 0 /* DLE */, 0 /* DC1 */, 0 /* DC2 */, 0 /* DC3 */, +- 0 /* DC4 */, 0 /* NAK */, 0 /* SYN */, 0 /* ETB */, +- 0 /* CAN */, 0 /* EM */, 0 /* SUB */, 0 /* ESC */, +- 0 /* FS */, 0 /* GS */, 0 /* RS */, 0 /* US */, +- 0 /* SPC */, 1 /* ! */, 0 /* " */, 0 /* # */, +- 1 /* $ */, 1 /* % */, 1 /* & */, 1 /* ' */, +- 1 /* ( */, 1 /* ) */, 1 /* * */, 1 /* + */, +- 1 /* , */, 1 /* - */, 1 /* . */, 0 /* / */, +- 1 /* 0 */, 1 /* 1 */, 1 /* 2 */, 1 /* 3 */, +- 1 /* 4 */, 1 /* 5 */, 1 /* 6 */, 1 /* 7 */, +- 1 /* 8 */, 1 /* 9 */, 1 /* : */, 1 /* ; */, +- 0 /* < */, 1 /* = */, 0 /* > */, 0 /* ? */, +- 1 /* @ */, 1 /* A */, 1 /* B */, 1 /* C */, +- 1 /* D */, 1 /* E */, 1 /* F */, 1 /* G */, +- 1 /* H */, 1 /* I */, 1 /* J */, 1 /* K */, +- 1 /* L */, 1 /* M */, 1 /* N */, 1 /* O */, +- 1 /* P */, 1 /* Q */, 1 /* R */, 1 /* S */, +- 1 /* T */, 1 /* U */, 1 /* V */, 1 /* W */, +- 1 /* X */, 1 /* Y */, 1 /* Z */, 1 /* [ */, +- 0 /* \ */, 1 /* ] */, 0 /* ^ */, 1 /* _ */, +- 0 /* ` */, 1 /* a */, 1 /* b */, 1 /* c */, +- 1 /* d */, 1 /* e */, 1 /* f */, 1 /* g */, +- 1 /* h */, 1 /* i */, 1 /* j */, 1 /* k */, +- 1 /* l */, 1 /* m */, 1 /* n */, 1 /* o */, +- 1 /* p */, 1 /* q */, 1 /* r */, 1 /* s */, +- 1 /* t */, 1 /* u */, 1 /* v */, 1 /* w */, +- 1 /* x */, 1 /* y */, 1 /* z */, 0 /* { */, +- 0 /* | */, 0 /* } */, 1 /* ~ */, 0 /* DEL */, +- 0 /* 0x80 */, 0 /* 0x81 */, 0 /* 0x82 */, 0 /* 0x83 */, +- 0 /* 0x84 */, 0 /* 0x85 */, 0 /* 0x86 */, 0 /* 0x87 */, +- 0 /* 0x88 */, 0 /* 0x89 */, 0 /* 0x8a */, 0 /* 0x8b */, +- 0 /* 0x8c */, 0 /* 0x8d */, 0 /* 0x8e */, 0 /* 0x8f */, +- 0 /* 0x90 */, 0 /* 0x91 */, 0 /* 0x92 */, 0 /* 0x93 */, +- 0 /* 0x94 */, 0 /* 0x95 */, 0 /* 0x96 */, 0 /* 0x97 */, +- 0 /* 0x98 */, 0 /* 0x99 */, 0 /* 0x9a */, 0 /* 0x9b */, +- 0 /* 0x9c */, 0 /* 0x9d */, 0 /* 0x9e */, 0 /* 0x9f */, +- 0 /* 0xa0 */, 0 /* 0xa1 */, 0 /* 0xa2 */, 0 /* 0xa3 */, +- 0 /* 0xa4 */, 0 /* 0xa5 */, 0 /* 0xa6 */, 0 /* 0xa7 */, +- 0 /* 0xa8 */, 0 /* 0xa9 */, 0 /* 0xaa */, 0 /* 0xab */, +- 0 /* 0xac */, 0 /* 0xad */, 0 /* 0xae */, 0 /* 0xaf */, +- 0 /* 0xb0 */, 0 /* 0xb1 */, 0 /* 0xb2 */, 0 /* 0xb3 */, +- 0 /* 0xb4 */, 0 /* 0xb5 */, 0 /* 0xb6 */, 0 /* 0xb7 */, +- 0 /* 0xb8 */, 0 /* 0xb9 */, 0 /* 0xba */, 0 /* 0xbb */, +- 0 /* 0xbc */, 0 /* 0xbd */, 0 /* 0xbe */, 0 /* 0xbf */, +- 0 /* 0xc0 */, 0 /* 0xc1 */, 0 /* 0xc2 */, 0 /* 0xc3 */, +- 0 /* 0xc4 */, 0 /* 0xc5 */, 0 /* 0xc6 */, 0 /* 0xc7 */, +- 0 /* 0xc8 */, 0 /* 0xc9 */, 0 /* 0xca */, 0 /* 0xcb */, +- 0 /* 0xcc */, 0 /* 0xcd */, 0 /* 0xce */, 0 /* 0xcf */, +- 0 /* 0xd0 */, 0 /* 0xd1 */, 0 /* 0xd2 */, 0 /* 0xd3 */, +- 0 /* 0xd4 */, 0 /* 0xd5 */, 0 /* 0xd6 */, 0 /* 0xd7 */, +- 0 /* 0xd8 */, 0 /* 0xd9 */, 0 /* 0xda */, 0 /* 0xdb */, +- 0 /* 0xdc */, 0 /* 0xdd */, 0 /* 0xde */, 0 /* 0xdf */, +- 0 /* 0xe0 */, 0 /* 0xe1 */, 0 /* 0xe2 */, 0 /* 0xe3 */, +- 0 /* 0xe4 */, 0 /* 0xe5 */, 0 /* 0xe6 */, 0 /* 0xe7 */, +- 0 /* 0xe8 */, 0 /* 0xe9 */, 0 /* 0xea */, 0 /* 0xeb */, +- 0 /* 0xec */, 0 /* 0xed */, 0 /* 0xee */, 0 /* 0xef */, +- 0 /* 0xf0 */, 0 /* 0xf1 */, 0 /* 0xf2 */, 0 /* 0xf3 */, +- 0 /* 0xf4 */, 0 /* 0xf5 */, 0 /* 0xf6 */, 0 /* 0xf7 */, +- 0 /* 0xf8 */, 0 /* 0xf9 */, 0 /* 0xfa */, 0 /* 0xfb */, +- 0 /* 0xfc */, 0 /* 0xfd */, 0 /* 0xfe */, 0 /* 0xff */ +-}; +- +-static int check_authority(const uint8_t *value, size_t len) { +- const uint8_t *last; +- for (last = value + len; value != last; ++value) { +- if (!VALID_AUTHORITY_CHARS[*value]) { +- return 0; +- } +- } +- return 1; +-} +- + static int check_scheme(const uint8_t *value, size_t len) { + const uint8_t *last; + if (len == 0) { +@@ -440,7 +362,7 @@ int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream, + + if (nv->token == NGHTTP2_TOKEN__AUTHORITY || + nv->token == NGHTTP2_TOKEN_HOST) { +- rv = check_authority(nv->value->base, nv->value->len); ++ rv = nghttp2_check_authority(nv->value->base, nv->value->len); + } else if (nv->token == NGHTTP2_TOKEN__SCHEME) { + rv = check_scheme(nv->value->base, nv->value->len); + } else { diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index d32294de7d2d6..d9d352d7ca4e1 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -49,9 +49,11 @@ Version history * http: :ref:`AUTO ` codec protocol inference now requires the H2 magic bytes to be the first bytes transmitted by a downstream client. * http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported. * http: absolute URL support is now on by default. The prior behavior can be reinstated by setting :ref:`allow_absolute_url ` to false. +* http: added strict authority checking. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_authority_validation` to false. * http: support :ref:`host rewrite ` in the dynamic forward proxy. * http: support :ref:`disabling the filter per route ` in the grpc http1 reverse bridge filter. * http: added the ability to :ref:`configure max connection duration ` for downstream connections. +* http: trim LWS at the end of header keys, for correct HTTP/1.1 header parsing. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. * listeners: added :ref:`connection balancer ` diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 49b46080257eb..14373e31ed5f3 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -136,6 +136,8 @@ struct ResponseCodeDetailValues { // indicates that original "success" headers may have been sent downstream // despite the subsequent failure. const std::string LateUpstreamReset = "upstream_reset_after_response_started"; + // The request was rejected due to invalid characters in the HOST/:authority header. + const std::string InvalidAuthority = "invalid_authority"; }; using ResponseCodeDetails = ConstSingleton; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index aaa5c77c13d74..aa7de6f07fedd 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -779,6 +779,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } } + // Make sure the host is valid. + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") && + !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) { + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().InvalidAuthority); + return; + } + // Currently we only support relative paths at the application layer. We expect the codec to have // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this // when the allow_absolute_url flag is enabled on the HCM. diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 71b390121fbb7..7bb74e0ea9324 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -135,6 +135,11 @@ bool HeaderUtility::headerIsValid(const absl::string_view header_value) { header_value.size()) != 0); } +bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { + return (nghttp2_check_authority(reinterpret_cast(header_value.data()), + header_value.size()) != 0); +} + void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) { headers_to_add.iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 51c5dc6598575..aa2a92e3ece30 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -96,6 +96,12 @@ class HeaderUtility { */ static bool headerIsValid(const absl::string_view header_value); + /** + * Validates that the characters in the authority are valid. + * @return bool true if the header values are valid, false otherwise. + */ + static bool authorityIsValid(const absl::string_view authority_value); + /** * Add headers from one HeaderMap to another * @param headers target where headers will be added diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 4aaf32b79c278..81b98689815a7 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -303,7 +303,7 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_ head_request_ = true; } connection_.onEncodeHeaders(headers); - connection_.reserveBuffer(std::max(4096U, path->value().size() + 4096)); + connection_.reserveBuffer(path->value().size() + method->value().size() + 4096); connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); @@ -465,7 +465,9 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { return; } - const absl::string_view header_value = absl::string_view(data, length); + // Work around a bug in http_parser where trailing whitespace is not trimmed + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 + const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); if (strict_header_validation_) { if (!Http::HeaderUtility::headerIsValid(header_value)) { @@ -483,7 +485,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } header_parsing_state_ = HeaderParsingState::Value; - current_header_value_.append(data, length); + current_header_value_.append(header_value.data(), header_value.length()); // Verify that the cached value in byte size exists. ASSERT(current_header_map_->byteSize().has_value()); diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index d097dce7c698f..5ca6cf2485de4 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -384,6 +384,12 @@ bool RouteEntryImplBase::matchRoute(const Http::HeaderMap& headers, uint64_t random_value) const { bool matches = true; + // TODO(mattklein123): Currently all match types require a path header. When we support CONNECT + // we will need to figure out how to safely relax this. + if (headers.Path() == nullptr) { + return false; + } + matches &= evaluateRuntimeMatch(random_value); if (!matches) { // No need to waste further cycles calculating a route match. @@ -1066,6 +1072,11 @@ const VirtualHostImpl* RouteMatcher::findVirtualHost(const Http::HeaderMap& head return default_virtual_host_.get(); } + // There may be no authority in early reply paths in the HTTP connection manager. + if (headers.Host() == nullptr) { + return nullptr; + } + // TODO (@rshriram) Match Origin header in WebSocket // request with VHost, using wildcard match const std::string host = diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6c3076d5466bb..f08acfbb9da28 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -28,6 +28,8 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.buffer_filter_populate_content_length", "envoy.reloadable_features.trusted_forwarded_proto", "envoy.reloadable_features.outlier_detection_support_for_grpc_status", + "envoy.reloadable_features.strict_header_validation", + "envoy.reloadable_features.strict_authority_validation", }; // This is a list of configuration fields which are disallowed by default in Envoy diff --git a/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 new file mode 100644 index 0000000000000..f8d96b4c26afd --- /dev/null +++ b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5726642969772032 @@ -0,0 +1,12 @@ +actions { new_stream { request_headers { headers { key: ":method" value: " _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_s ke n key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _h new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _h new_st t_he ke new_st e new_st kr n key:e ke new_st e new_st kr ke key:e ke new_st e new_st kr ke n _he ke new_st e new_st kr ke st e new_st t_he ke new_st e new_st kr ke n key:e ke he ke new_st e new_st kr ke st e new_st t_he ket_he ke new_st e new_st kr e new_st t_he ke new_st e new_st kr ke n key:e ke newstrey:_]E]u___ }\n}," + } + headers { + key: ":method" + value: "GETactions {\n muta{\n ketruest_he key: ctions {\n ers {\n headers {\n key: ctions {\n new_streamTnrtasfTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amrtasfer-headers {headers {\n headers {u new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n headers headers {u new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti headers {u new_stream {asfer-e key: ctioew: r-e oew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headest_heade headers {u key: cti headers {u new_stream {asfer-e key: ctioew: r-e oew: r-e key: ctionsest_headers {\n he: r-e key: ctionsest_heade headers {u key: cti new_streew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headers {@ headers {u new_stream {asfer-e key: ctioew: \"Tnrtasfer-e key: ction: cti new_stream {reew_stream {asfer-e key: c{u amTkey: ctioew: new_stream {asfer-e key: ctioew: r-e key: ctionsest_headers new_stream {asfer-e key: ctioew: r-ede headers {u key: cti new_streesfer-headers {@ headers {u new_stream {asfer-e key: ctioew: \"Tnrtasfer-e key: ction: cti new_stream {\n new_streame: s {\n new_streamTnrtasfe " + } + headers { + key: ":path" + } + } + } +} diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index c58318d76cfb9..067e0a45f776d 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -464,6 +464,13 @@ TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) { EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value")); } +TEST(HeaderIsValidTest, AuthIsValid) { + EXPECT_TRUE(HeaderUtility::authorityIsValid("strangebutlegal$-%&'")); + EXPECT_FALSE(HeaderUtility::authorityIsValid("illegal{}")); + // Full checks are done by Http2CodecImplTest.CheckAuthority, cross checking + // against nghttp2 compliance. +} + TEST(HeaderAddTest, HeaderAdd) { TestHeaderMapImpl headers{{"myheader1", "123value"}}; TestHeaderMapImpl headers_to_add{{"myheader2", "456value"}}; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index e834cb412cc6f..cc77c93709843 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -61,6 +61,25 @@ class Http1ServerConnectionImplTest : public testing::Test { void testRequestHeadersExceedLimit(std::string header_string); void testRequestHeadersAccepted(std::string header_string); + // Send the request, and validate the received request headers. + // Then send a response just to clean up. + void sendAndValidateRequestAndSendResponse(absl::string_view raw_request, + const TestHeaderMapImpl& expected_request_headers) { + NiceMock decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .Times(1) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1); + Buffer::OwnedImpl buffer(raw_request); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); + } + protected: uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; @@ -167,6 +186,23 @@ TEST_F(Http1ServerConnectionImplTest, EmptyHeader) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { + initialize(); + + TestHeaderMapImpl expected_headers{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}; + + // Regression test spaces before and after the host header value. + sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); + + // Regression test tabs before and after the host header value. + sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost: host \r\n\r\n", + expected_headers); + + // Regression test mixed spaces and tabs before and after the host header value. + sendAndValidateRequestAndSendResponse( + "GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); +} + TEST_F(Http1ServerConnectionImplTest, Http10) { initialize(); @@ -358,6 +394,22 @@ TEST_F(Http1ServerConnectionImplTest, BadRequestNoStream) { EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); } +// This behavior was observed during CVE-2019-18801 and helped to limit the +// scope of affected Envoy configurations. +TEST_F(Http1ServerConnectionImplTest, RejectInvalidMethod) { + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + Buffer::OwnedImpl buffer("BAD / HTTP/1.1\r\nHost: foo\r\n"); + EXPECT_THROW(codec_->dispatch(buffer), CodecProtocolException); + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); +} + TEST_F(Http1ServerConnectionImplTest, BadRequestStartedStream) { initialize(); @@ -433,6 +485,9 @@ TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) { // Regression test for http-parser allowing embedded NULs in header values, // verify we reject them. TEST_F(Http1ServerConnectionImplTest, HeaderEmbeddedNulRejection) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.strict_header_validation", "false"}}); initialize(); InSequence sequence; @@ -556,6 +611,33 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponse) { EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output); } +// As with Http1ClientConnectionImplTest.LargeHeaderRequestEncode but validate +// the response encoder instead of request encoder. +TEST_F(Http1ServerConnectionImplTest, LargeHeaderResponseEncode) { + initialize(); + + NiceMock decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + const std::string long_header_value = std::string(79 * 1024, 'a'); + TestHeaderMapImpl headers{{":status", "200"}, {"foo", long_header_value}}; + response_encoder->encodeHeaders(headers, true); + EXPECT_EQ("HTTP/1.1 200 OK\r\nfoo: " + long_header_value + "\r\ncontent-length: 0\r\n\r\n", + output); +} + TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseTrainProperHeaders) { codec_settings_.header_key_format_ = Http1Settings::HeaderKeyFormat::ProperCase; initialize(); @@ -1425,6 +1507,56 @@ TEST_F(Http1ClientConnectionImplTest, LargeResponseHeadersAccepted) { codec_->dispatch(buffer); } +// Regression test for CVE-2019-18801. Large method headers should not trigger +// ASSERTs or ASAN, which they previously did. +TEST_F(Http1ClientConnectionImplTest, LargeMethodRequestEncode) { + initialize(); + + NiceMock response_decoder; + const std::string long_method = std::string(79 * 1024, 'a'); + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + TestHeaderMapImpl headers{{":method", long_method}, {":path", "/"}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ(long_method + " / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); +} + +// As with LargeMethodEncode, but for the path header. This was not an issue +// in CVE-2019-18801, but the related code does explicit size calculations on +// both path and method (these are the two distinguished headers). So, +// belt-and-braces. +TEST_F(Http1ClientConnectionImplTest, LargePathRequestEncode) { + initialize(); + + NiceMock response_decoder; + const std::string long_path = std::string(79 * 1024, '/'); + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + TestHeaderMapImpl headers{{":method", "GET"}, {":path", long_path}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ("GET " + long_path + " HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); +} + +// As with LargeMethodEncode, but for an arbitrary header. This was not an issue +// in CVE-2019-18801. +TEST_F(Http1ClientConnectionImplTest, LargeHeaderRequestEncode) { + initialize(); + + NiceMock response_decoder; + Http::StreamEncoder& request_encoder = codec_->newStream(response_decoder); + const std::string long_header_value = std::string(79 * 1024, 'a'); + TestHeaderMapImpl headers{ + {":method", "GET"}, {"foo", long_header_value}, {":path", "/"}, {":authority", "host"}}; + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + request_encoder.encodeHeaders(headers, true); + EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\nfoo: " + long_header_value + + "\r\ncontent-length: 0\r\n\r\n", + output); +} + // Exception called when the number of response headers exceeds the default value of 100. TEST_F(Http1ClientConnectionImplTest, ManyResponseHeadersRejected) { initialize(); diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 3a93f99146e4f..ded96525c02b0 100644 --- a/test/common/http/http2/BUILD +++ b/test/common/http/http2/BUILD @@ -19,6 +19,7 @@ envoy_cc_test( "//source/common/event:dispatcher_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/http/http2:codec_lib", "//source/common/stats:stats_lib", "//test/common/http:common_lib", diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 81707a9a2a48a..87f20fe6509aa 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1090,6 +1090,25 @@ TEST_P(Http2CodecImplTest, LargeRequestHeadersAccepted) { request_encoder_->encodeHeaders(request_headers, false); } +// This is the HTTP/2 variant of the HTTP/1 regression test for CVE-2019-18801. +// Large method headers should not trigger ASSERTs or ASAN. The underlying issue +// in CVE-2019-18801 only affected the HTTP/1 encoder, but we include a test +// here for belt-and-braces. This also demonstrates that the HTTP/2 codec will +// accept arbitrary :method headers, unlike the HTTP/1 codec (see +// Http1ServerConnectionImplTest.RejectInvalidMethod for comparison). +TEST_P(Http2CodecImplTest, LargeMethodRequestEncode) { + max_request_headers_kb_ = 80; + initialize(); + + const std::string long_method = std::string(79 * 1024, 'a'); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_headers.setReferenceKey(Headers::get().Method, long_method); + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&request_headers), false)); + EXPECT_CALL(server_stream_callbacks_, onResetStream(_, _)).Times(0); + request_encoder_->encodeHeaders(request_headers, false); +} + // Tests stream reset when the number of request headers exceeds the default maximum of 100. TEST_P(Http2CodecImplTest, ManyRequestHeadersInvokeResetStream) { initialize(); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index 1c2a74bccc2e6..f16d51932450b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -648,6 +648,17 @@ TEST_F(RouteMatcherTest, TestRoutes) { NiceMock stream_info; TestConfigImpl config(parseRouteConfigurationFromV2Yaml(yaml), factory_context_, true); + // No host header, no x-forwarded-proto and no path header testing. + EXPECT_EQ(nullptr, config.route(Http::TestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, 0)); + EXPECT_EQ( + nullptr, + config.route( + Http::TestHeaderMapImpl{{":authority", "foo"}, {":path", "/"}, {":method", "GET"}}, 0)); + EXPECT_EQ(nullptr, config.route(Http::TestHeaderMapImpl{{":authority", "foo"}, + {":method", "CONNECT"}, + {"x-forwarded-proto", "http"}}, + 0)); + // Base routing testing. EXPECT_EQ("instant-server", config.route(genHeaders("api.lyft.com", "/", "GET"), 0)->routeEntry()->clusterName()); diff --git a/test/common/router/route_fuzz_test.cc b/test/common/router/route_fuzz_test.cc index 19750392b5208..34df71b6b0a3a 100644 --- a/test/common/router/route_fuzz_test.cc +++ b/test/common/router/route_fuzz_test.cc @@ -84,17 +84,6 @@ DEFINE_PROTO_FUZZER(const test::common::router::RouteTestCase& input) { ConfigImpl config(cleanRouteConfig(input.config()), factory_context, ProtobufMessage::getNullValidationVisitor(), true); Http::TestHeaderMapImpl headers = Fuzz::fromHeaders(input.headers()); - // It's a precondition of routing that {:authority, :path, x-forwarded-proto} headers exists, - // HCM enforces this. - if (headers.Host() == nullptr) { - headers.insertHost().value(std::string("example.com")); - } - if (headers.Path() == nullptr) { - headers.insertPath().value(std::string("/")); - } - if (headers.ForwardedProto() == nullptr) { - headers.insertForwardedProto().value(std::string("http")); - } auto route = config.route(headers, stream_info, input.random_value()); if (route != nullptr && route->routeEntry() != nullptr) { route->routeEntry()->finalizeRequestHeaders(headers, stream_info, true); diff --git a/test/extensions/filters/http/lua/lua_integration_test.cc b/test/extensions/filters/http/lua/lua_integration_test.cc index 36f30b08702ed..043d2d33dde5f 100644 --- a/test/extensions/filters/http/lua/lua_integration_test.cc +++ b/test/extensions/filters/http/lua/lua_integration_test.cc @@ -21,7 +21,7 @@ class LuaIntegrationTest : public testing::TestWithParammutable_virtual_hosts(0) ->mutable_routes(0) ->mutable_match() ->set_prefix("/test/long/url"); + hcm.mutable_route_config()->mutable_virtual_hosts(0)->set_domains(0, domain); auto* new_route = hcm.mutable_route_config()->mutable_virtual_hosts(0)->add_routes(); new_route->mutable_match()->set_prefix("/alt/route"); new_route->mutable_route()->set_cluster("alt_cluster"); @@ -96,6 +97,32 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, LuaIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +// Regression test for pulling route info during early local replies using the Lua filter +// metadata() API. Covers both the upgrade required and no authority cases. +TEST_P(LuaIntegrationTest, CallMetadataDuringLocalReply) { + const std::string FILTER_AND_CODE = + R"EOF( +name: envoy.lua +typed_config: + "@type": type.googleapis.com/envoy.config.filter.http.lua.v2.Lua + inline_code: | + function envoy_on_response(response_handle) + local metadata = response_handle:metadata():get("foo.bar") + if metadata == nil then + end + end +)EOF"; + + initializeFilter(FILTER_AND_CODE, "foo"); + std::string response; + sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response, true); + EXPECT_TRUE(response.find("HTTP/1.1 426 Upgrade Required\r\n") == 0); + + response = ""; + sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.1\r\n\r\n", &response, true); + EXPECT_TRUE(response.find("HTTP/1.1 400 Bad Request\r\n") == 0); +} + // Basic request and response. TEST_P(LuaIntegrationTest, RequestAndResponse) { const std::string FILTER_AND_CODE = diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index a7ab4696804ae..49106fc3e8024 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -24,6 +24,7 @@ #include "common/common/thread.h" #include "common/grpc/codec.h" #include "common/grpc/common.h" +#include "common/http/exception.h" #include "common/network/connection_balancer_impl.h" #include "common/network/filter_impl.h" #include "common/network/listen_socket_impl.h" @@ -435,10 +436,24 @@ class FakeHttpConnection : public Http::ServerConnectionCallbacks, public FakeCo // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool) override { - parent_.codec_->dispatch(data); + try { + parent_.codec_->dispatch(data); + } catch (const Http::CodecProtocolException& e) { + ENVOY_LOG(debug, "FakeUpstream dispatch error: {}", e.what()); + // We don't do a full stream shutdown like HCM, but just shutdown the + // connection for now. + read_filter_callbacks_->connection().close( + Network::ConnectionCloseType::FlushWriteAndDelay); + } return Network::FilterStatus::StopIteration; } + void + initializeReadFilterCallbacks(Network::ReadFilterCallbacks& read_filter_callbacks) override { + read_filter_callbacks_ = &read_filter_callbacks; + } + + Network::ReadFilterCallbacks* read_filter_callbacks_{}; FakeHttpConnection& parent_; }; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 78134daffed43..5dbb8ce7b622b 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -446,13 +446,15 @@ TEST_P(IntegrationTest, TestInlineHeaders) { // Verify for HTTP/1.0 a keep-alive header results in no connection: close. // Also verify existing host headers are passed through for the HTTP/1.0 case. -TEST_P(IntegrationTest, Http10WithHostandKeepAlive) { +// This also regression tests proper handling of trailing whitespace after key +// values, specifically the host header. +TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { autonomous_upstream_ = true; config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); initialize(); std::string response; sendRawHttpAndWaitForResponse(lookupPort("http"), - "GET / HTTP/1.0\r\nHost: foo.com\r\nConnection:Keep-alive\r\n\r\n", + "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", &response, true); EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); EXPECT_THAT(response, Not(HasSubstr("connection: close"))); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 35846cf70ae16..c306bdbb4827b 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1060,6 +1060,55 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { EXPECT_EQ("200", response->headers().Status()->value().getStringView()); } +// Regression tests for CVE-2019-18801. We only validate the behavior of large +// :method request headers, since the case of other large headers is +// covered in the various testLargeRequest-based integration tests here. +// +// The table below describes the expected behaviors (in addition we should never +// see an ASSERT or ASAN failure trigger). +// +// Downstream Upstream Behavior expected +// ------------------------------------------ +// H1 H1 Envoy will reject (HTTP/1 codec behavior) +// H1 H2 Envoy will reject (HTTP/1 codec behavior) +// H2 H1 Envoy will forward but backend will reject (HTTP/1 +// codec behavior) +// H2 H2 Success +TEST_P(ProtocolIntegrationTest, LargeRequestMethod) { + const std::string long_method = std::string(48 * 1024, 'a'); + const Http::TestHeaderMapImpl request_headers{{":method", long_method}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + auto encoder_decoder = codec_client_->startRequest(request_headers); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->waitForDisconnect(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT(downstreamProtocol() == Http::CodecClient::Type::HTTP2); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + ASSERT_TRUE( + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT(upstreamProtocol() == FakeHttpConnection::Type::HTTP2); + auto response = + sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0); + EXPECT_TRUE(response->complete()); + } + } +} + // Tests StopAllIterationAndBuffer. Verifies decode-headers-return-stop-all-filter calls decodeData // once after iteration is resumed. TEST_P(DownstreamProtocolIntegrationTest, testDecodeHeadersReturnsStopAll) { @@ -1413,6 +1462,32 @@ TEST_P(ProtocolIntegrationTest, ConnDurationTimeoutNoHttpRequest) { test_server_->waitForCounterGe("http.config_test.downstream_cx_max_duration_reached", 1); } +// Make sure that invalid authority headers get blocked at or before the HCM. +TEST_P(DownstreamProtocolIntegrationTest, InvalidAuth) { + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "ho|st|"}}; + + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + // For HTTP/1 this is handled by the HCM, which sends a full 400 response. + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + // For HTTP/2 this is handled by nghttp2 which resets the connection without + // sending an HTTP response. + codec_client_->waitForDisconnect(); + ASSERT_FALSE(response->complete()); + } +} + // For tests which focus on downstream-to-Envoy behavior, and don't need to be // run with both HTTP/1 and HTTP/2 upstreams. INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest, diff --git a/tools/spelling_skip_files.txt b/tools/spelling_skip_files.txt index 6604701fc7065..8072b60c6b9e4 100644 --- a/tools/spelling_skip_files.txt +++ b/tools/spelling_skip_files.txt @@ -1,4 +1,4 @@ -OWNERS.md +OWNERS.md corpus examples/wasm/*.wat test/extensions/access_loggers/wasm/test_data/*.wat test/extensions/filters/http/wasm/test_data/*.wat