From 6a6ac1ccf928aa9866f19e2cf151c5dd339bb8e6 Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Tue, 10 Dec 2019 06:04:30 -0500 Subject: [PATCH] Stricter validation of HTTP/1 headers (CVE-2019-18802). (#72) Signed-off-by: Alyssa Wilk --- bazel/foreign_cc/nghttp2.patch | 223 ++++++++++++++++++ docs/root/intro/version_history.rst | 2 + include/envoy/stream_info/stream_info.h | 2 + source/common/http/conn_manager_impl.cc | 9 + source/common/http/header_utility.cc | 5 + source/common/http/header_utility.h | 6 + source/common/http/http1/codec_impl.cc | 6 +- source/common/runtime/runtime_features.cc | 2 + test/common/http/header_utility_test.cc | 7 + test/common/http/http1/codec_impl_test.cc | 39 +++ test/common/http/http2/BUILD | 1 + test/integration/integration_test.cc | 6 +- test/integration/protocol_integration_test.cc | 26 ++ 13 files changed, 330 insertions(+), 4 deletions(-) diff --git a/bazel/foreign_cc/nghttp2.patch b/bazel/foreign_cc/nghttp2.patch index e7b001673a56..bfe51d07dad0 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 43606cde7f43..621921e28270 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -53,9 +53,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 196d2fb50f2e..06ed2f17a82b 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 aaa5c77c13d7..aa7de6f07fed 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 71b390121fbb..7bb74e0ea932 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 51c5dc659857..aa2a92e3ece3 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 9b4550169ec6..81b98689815a 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -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/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6c3076d5466b..f08acfbb9da2 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/header_utility_test.cc b/test/common/http/header_utility_test.cc index c58318d76cfb..067e0a45f776 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 d5f36e9dbb8e..cc77c9370984 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(); @@ -449,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; diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 3a93f99146e4..ded96525c02b 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/integration/integration_test.cc b/test/integration/integration_test.cc index 78134daffed4..5dbb8ce7b622 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 bc723103476c..c306bdbb4827 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1462,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,