From 7ed6d2187df94c4cb96f7dccb8643bf764af2ccb Mon Sep 17 00:00:00 2001 From: htuch Date: Wed, 27 Mar 2019 12:48:04 -0400 Subject: [PATCH] hcm: path normalization. (#1) Provide the HTTP path normalization per RFC 3986 (sans case normalization). This addresses CVE-2019-9901. The config HttpConnectionManager.normalize_path needs to be set for each HCM configuration to enable (default is off). There is also a runtime optione http_connection_manager.normalize_path to change this default when not set in HCM. Risk level: Low Testing: New unit and integration tests added. Signed-off-by: Yuchen Dai Signed-off-by: Harvey Tuch --- .../v2/http_connection_manager.proto | 14 ++- .../configuration/http_conn_man/runtime.rst | 8 ++ docs/root/intro/version_history.rst | 6 +- source/common/http/BUILD | 13 ++ source/common/http/conn_manager_config.h | 5 + source/common/http/conn_manager_impl.cc | 10 ++ source/common/http/conn_manager_utility.cc | 11 ++ source/common/http/conn_manager_utility.h | 5 + source/common/http/header_map_impl.cc | 2 + source/common/http/header_map_impl.h | 1 + source/common/http/path_utility.cc | 55 +++++++++ source/common/http/path_utility.h | 19 +++ .../network/http_connection_manager/config.cc | 9 +- .../network/http_connection_manager/config.h | 2 + source/server/http/admin.h | 1 + test/common/http/BUILD | 9 ++ .../http/conn_manager_impl_fuzz_test.cc | 4 +- test/common/http/conn_manager_impl_test.cc | 112 +++++++++++++++++- test/common/http/conn_manager_utility_test.cc | 34 ++++++ test/common/http/path_utility_test.cc | 89 ++++++++++++++ .../http/rbac/rbac_filter_integration_test.cc | 64 ++++++++++ .../http_connection_manager/config_test.cc | 71 +++++++++++ test/integration/header_integration_test.cc | 79 ++++++++++++ 23 files changed, 617 insertions(+), 6 deletions(-) create mode 100644 source/common/http/path_utility.cc create mode 100644 source/common/http/path_utility.h create mode 100644 test/common/http/path_utility_test.cc diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 71f29474a1c9..b2bc2b8e3c3e 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -380,8 +380,18 @@ message HttpConnectionManager { reserved 27; - // This is reserved for a pending security fix. - reserved 30; + // Should paths be normalized according to RFC 3986 before any processing of + // requests by HTTP filters or routing? This affects the upstream *:path* header + // as well. For paths that fail this check, Envoy will respond with 400 to + // paths that are malformed. This defaults to false currently but will default + // true in the future. When not specified, this value may be overridden by the + // runtime variable + // :ref:`http_connection_manager.normalize_path`. + // See `Normalization and Comparison ` + // for details of normalization. + // Note that Envoy does not perform + // `case normalization ` + google.protobuf.BoolValue normalize_path = 30; } message Rds { diff --git a/docs/root/configuration/http_conn_man/runtime.rst b/docs/root/configuration/http_conn_man/runtime.rst index 22fc453b3ad9..dcc85412c631 100644 --- a/docs/root/configuration/http_conn_man/runtime.rst +++ b/docs/root/configuration/http_conn_man/runtime.rst @@ -5,6 +5,14 @@ Runtime The HTTP connection manager supports the following runtime settings: +.. _config_http_conn_man_runtime_normalize_path: + +http_connection_manager.normalize_path + % of requests that will have path normalization applied if not already configured in + :ref:`normalize_path `. + This is evaluated at configuration load time and will apply to all requests for a given + configuration. + .. _config_http_conn_man_runtime_client_enabled: tracing.client_enabled diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 6880dd4d1fa6..049cf1cba924 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -81,13 +81,17 @@ Version history * tracing: added :ref:`verbose ` to support logging annotations on spans. * upstream: added support for host weighting and :ref:`locality weighting ` in the :ref:`ring hash load balancer `, and added a :ref:`maximum_ring_size` config parameter to strictly bound the ring size. * zookeeper: added a ZooKeeper proxy filter that parses ZooKeeper messages (requests/responses/events). - Refer to ::ref:`ZooKeeper proxy` for more details. + Refer to :ref:`ZooKeeper proxy` for more details. * upstream: added configuration option to select any host when the fallback policy fails. * upstream: stopped incrementing upstream_rq_total for HTTP/1 conn pool when request is circuit broken. 1.9.1 (Apr 2, 2019) =================== * http: fixed CVE-2019-9900 by rejecting HTTP/1.x headers with embedded NUL characters. +* http: fixed CVE-2019-9901 by normalizing HTTP paths prior to routing or L7 data plane processing. + This defaults off and is configurable via either HTTP connection manager :ref:`normalize_path + ` + or the :ref:`runtime `. 1.9.0 (Dec 20, 2018) ==================== diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 903f86182865..b947d3b5293d 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -141,6 +141,7 @@ envoy_cc_library( ":exception_lib", ":header_map_lib", ":headers_lib", + ":path_utility_lib", ":user_agent_lib", ":utility_lib", "//include/envoy/access_log:access_log_interface", @@ -314,3 +315,15 @@ envoy_cc_library( "@envoy_api//envoy/type:range_cc", ], ) + +envoy_cc_library( + name = "path_utility_lib", + srcs = ["path_utility.cc"], + hdrs = ["path_utility.h"], + external_deps = ["abseil_optional"], + deps = [ + "//include/envoy/http:header_map_interface", + "//source/common/chromium_url", + "//source/common/common:logger_lib", + ], +) diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index d5cb27c025fb..2fb4ee9e9c57 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -340,6 +340,11 @@ class ConnectionManagerConfig { * @return supplies the http1 settings. */ virtual const Http::Http1Settings& http1Settings() const PURE; + + /** + * @return if the HttpConnectionManager should normalize url following RFC3986 + */ + virtual bool shouldNormalizePath() const PURE; }; } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 9c23eb39b6d1..80932dc69bd1 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -29,9 +29,11 @@ #include "common/http/headers.h" #include "common/http/http1/codec_impl.h" #include "common/http/http2/codec_impl.h" +#include "common/http/path_utility.h" #include "common/http/utility.h" #include "common/network/utility.h" +#include "absl/strings/escaping.h" #include "absl/strings/match.h" namespace Envoy { @@ -666,6 +668,14 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, return; } + // Path sanitization should happen before any path access other than the above sanity check. + if (!ConnectionManagerUtility::maybeNormalizePath(*request_headers_, + connection_manager_.config_)) { + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", + nullptr, is_head_request_, absl::nullopt); + return; + } + if (protocol == Protocol::Http11 && request_headers_->Connection() && absl::EqualsIgnoreCase(request_headers_->Connection()->value().getStringView(), Http::Headers::get().ConnectionValues.Close)) { diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index f85965cf05cc..164b8712c295 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -10,6 +10,7 @@ #include "common/http/headers.h" #include "common/http/http1/codec_impl.h" #include "common/http/http2/codec_impl.h" +#include "common/http/path_utility.h" #include "common/http/utility.h" #include "common/network/utility.h" #include "common/runtime/uuid_util.h" @@ -364,5 +365,15 @@ void ConnectionManagerUtility::mutateResponseHeaders(HeaderMap& response_headers } } +/* static */ +bool ConnectionManagerUtility::maybeNormalizePath(HeaderMap& request_headers, + const ConnectionManagerConfig& config) { + ASSERT(request_headers.Path()); + if (config.shouldNormalizePath()) { + return PathUtil::canonicalPath(*request_headers.Path()); + } + return true; +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 0d313f185e64..126982df7753 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -59,6 +59,11 @@ class ConnectionManagerUtility { static void mutateResponseHeaders(HeaderMap& response_headers, const HeaderMap* request_headers, const std::string& via); + // Sanitize the path in the header map if forced by config. + // Side affect: the string view of Path header is invalidated. + // Return false if error happens during the sanitization. + static bool maybeNormalizePath(HeaderMap& request_headers, const ConnectionManagerConfig& config); + private: /** * Mutate request headers if request needs to be traced. diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 56185e9058ec..66500d425383 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -349,6 +349,8 @@ bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { return true; } +bool HeaderMapImpl::operator!=(const HeaderMapImpl& rhs) const { return !operator==(rhs); } + void HeaderMapImpl::insertByKey(HeaderString&& key, HeaderString&& value) { EntryCb cb = ConstSingleton::get().find(key.c_str()); if (cb) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 564fbd0c7495..376311b4536f 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -61,6 +61,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { * comparison (order matters). */ bool operator==(const HeaderMapImpl& rhs) const; + bool operator!=(const HeaderMapImpl& rhs) const; // Http::HeaderMap void addReference(const LowerCaseString& key, const std::string& value) override; diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc new file mode 100644 index 000000000000..796c2c1cbd52 --- /dev/null +++ b/source/common/http/path_utility.cc @@ -0,0 +1,55 @@ +#include "common/http/path_utility.h" + +#include "common/chromium_url/url_canon.h" +#include "common/chromium_url/url_canon_stdstring.h" +#include "common/common/logger.h" + +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" + +namespace Envoy { +namespace Http { + +namespace { +absl::optional canonicalizePath(absl::string_view original_path) { + std::string canonical_path; + url::Component in_component(0, original_path.size()); + url::Component out_component; + url::StdStringCanonOutput output(&canonical_path); + if (!url::CanonicalizePath(original_path.data(), in_component, &output, &out_component)) { + return absl::nullopt; + } else { + output.Complete(); + return absl::make_optional(std::move(canonical_path)); + } +} +} // namespace + +/* static */ +bool PathUtil::canonicalPath(HeaderEntry& path_header) { + const auto original_path = path_header.value().getStringView(); + // canonicalPath is supposed to apply on path component in URL instead of :path header + const auto query_pos = original_path.find('?'); + auto normalized_path_opt = canonicalizePath( + query_pos == original_path.npos + ? original_path + : absl::string_view(original_path.data(), query_pos) // '?' is not included + ); + + if (!normalized_path_opt.has_value()) { + return false; + } + auto& normalized_path = normalized_path_opt.value(); + const absl::string_view query_suffix = + query_pos == original_path.npos + ? absl::string_view{} + : absl::string_view{original_path.data() + query_pos, original_path.size() - query_pos}; + if (query_suffix.size() > 0) { + normalized_path.insert(normalized_path.end(), query_suffix.begin(), query_suffix.end()); + } + path_header.value(std::move(normalized_path)); + return true; +} + +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h new file mode 100644 index 000000000000..ad0d32c3ff7d --- /dev/null +++ b/source/common/http/path_utility.h @@ -0,0 +1,19 @@ +#pragma once + +#include "envoy/http/header_map.h" + +namespace Envoy { +namespace Http { + +/** + * Path helper extracted from chromium project. + */ +class PathUtil { +public: + // Returns if the normalization succeeds. + // If it is successful, the param will be updated with the normalized path. + static bool canonicalPath(HeaderEntry& path_header); +}; + +} // namespace Http +} // namespace Envoy diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index e5069bbf39e6..0b0b294913e7 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -150,7 +150,14 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( listener_stats_(Http::ConnectionManagerImpl::generateListenerStats(stats_prefix_, context_.listenerScope())), proxy_100_continue_(config.proxy_100_continue()), - delayed_close_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, delayed_close_timeout, 1000)) { + delayed_close_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, delayed_close_timeout, 1000)), + normalize_path_( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, normalize_path, + // TODO(htuch): we should have a + // boolean variant of featureEnabled() + // here. + context.runtime().snapshot().featureEnabled( + "http_connection_manager.normalize_path", 0))) { route_config_provider_ = Router::RouteConfigProviderUtil::create(config, context_, stats_prefix_, route_config_provider_manager_); diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 0d6bcd87668a..c1e7332b653c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -127,6 +127,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } + bool shouldNormalizePath() const override { return normalize_path_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } private: @@ -167,6 +168,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerListenerStats listener_stats_; const bool proxy_100_continue_; std::chrono::milliseconds delayed_close_timeout_; + const bool normalize_path_; // Default idle timeout is 5 minutes if nothing is specified in the HCM config. static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 31d8511a9c57..7bffdeb2b036 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -126,6 +126,7 @@ class AdminImpl : public Admin, Http::ConnectionManagerListenerStats& listenerStats() override { return listener_->stats_; } bool proxy100Continue() const override { return false; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } + bool shouldNormalizePath() const override { return true; } Http::Code request(absl::string_view path_and_query, absl::string_view method, Http::HeaderMap& response_headers, std::string& body) override; void closeSocket(); diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 91e16157e144..aaf0f6d92024 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -321,3 +321,12 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "path_utility_test", + srcs = ["path_utility_test.cc"], + deps = [ + "//source/common/http:header_map_lib", + "//source/common/http:path_utility_lib", + ], +) diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 334edf234f8f..07762102ba9a 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -116,6 +116,7 @@ class FuzzConfig : public ConnectionManagerConfig { ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } + bool shouldNormalizePath() const override { return false; } const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager config_; std::list access_logs_; @@ -142,9 +143,10 @@ class FuzzConfig : public ConnectionManagerConfig { Network::Address::Ipv4Instance local_address_{"127.0.0.1"}; absl::optional user_agent_; TracingConnectionManagerConfigPtr tracing_config_; - bool proxy_100_continue_ = true; + bool proxy_100_continue_{true}; Http::Http1Settings http1_settings_; Http::DefaultInternalAddressConfig internal_address_config_; + bool normalize_path_{true}; }; // Internal representation of stream state. Encapsulates the stream state, mocks diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index cbec81157ee1..05a9796879fa 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -257,6 +257,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } + bool shouldNormalizePath() const override { return normalize_path_; } DangerousDeprecatedTestTime test_time_; RouteConfigProvider route_config_provider_; @@ -302,6 +303,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan ConnectionManagerListenerStats listener_stats_; bool proxy_100_continue_ = false; Http::Http1Settings http1_settings_; + bool normalize_path_ = false; NiceMock upstream_conn_; // for websocket tests NiceMock conn_pool_; // for websocket tests @@ -540,6 +542,115 @@ TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { conn_manager_->onData(fake_input, false); } +// Invalid paths are rejected with 400. +TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { + InSequence s; + setup(false, ""); + // Enable path sanitizer + normalize_path_ = true; + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, + {":path", "/ab%00c"}, // "%00" is not valid in path according to RFC + {":method", "GET"}}}; + decoder->decodeHeaders(std::move(headers), true); + data.drain(4); + })); + + // This test also verifies that decoder/encoder filters have onDestroy() called only once. + MockStreamFilter* filter = new MockStreamFilter(); + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillOnce(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(StreamFilterSharedPtr{filter}); + })); + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); + + EXPECT_CALL(*filter, encodeHeaders(_, true)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, true)) + .WillOnce(Invoke([](const HeaderMap& headers, bool) -> void { + EXPECT_STREQ("400", headers.Status()->value().c_str()); + })); + EXPECT_CALL(*filter, onDestroy()); + + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + +// Filters observe normalized paths, not the original path, when path +// normalization is configured. +TEST_F(HttpConnectionManagerImplTest, FilterShouldUseSantizedPath) { + setup(false, ""); + // Enable path sanitizer + normalize_path_ = true; + const std::string original_path = "/x/%2E%2e/z"; + const std::string normalized_path = "/z"; + + MockStreamFilter* filter = new MockStreamFilter(); + + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillOnce(Invoke([&](FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(StreamDecoderFilterSharedPtr{filter}); + })); + + EXPECT_CALL(*filter, decodeHeaders(_, true)) + .WillRepeatedly(Invoke([&](HeaderMap& header_map, bool) -> FilterHeadersStatus { + EXPECT_EQ(normalized_path, header_map.Path()->value().c_str()); + return FilterHeadersStatus::StopIteration; + })); + + EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{new TestHeaderMapImpl{ + {":authority", "host"}, {":path", original_path}, {":method", "GET"}}}; + decoder->decodeHeaders(std::move(headers), true); + })); + + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + +// The router observes normalized paths, not the original path, when path +// normalization is configured. +TEST_F(HttpConnectionManagerImplTest, RouteShouldUseSantizedPath) { + setup(false, ""); + // Enable path sanitizer + normalize_path_ = true; + const std::string original_path = "/x/%2E%2e/z"; + const std::string normalized_path = "/z"; + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{new TestHeaderMapImpl{ + {":authority", "host"}, {":path", original_path}, {":method", "GET"}}}; + decoder->decodeHeaders(std::move(headers), true); + })); + + const std::string fake_cluster_name = "fake_cluster"; + + std::shared_ptr fake_cluster = + std::make_shared>(); + std::shared_ptr route = std::make_shared>(); + EXPECT_CALL(route->route_entry_, clusterName()).WillRepeatedly(ReturnRef(fake_cluster_name)); + + EXPECT_CALL(*route_config_provider_.route_config_, route(_, _)) + .WillOnce(Invoke([&](const Http::HeaderMap& header_map, uint64_t) { + EXPECT_EQ(normalized_path, header_map.Path()->value().c_str()); + return route; + })); + EXPECT_CALL(filter_factory_, createFilterChain(_)) + .WillOnce(Invoke([&](FilterChainFactoryCallbacks&) -> void {})); + + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { setup(false, ""); @@ -3905,6 +4016,5 @@ TEST_F(HttpConnectionManagerImplTest, OverlyLongHeadersAcceptedIfConfigured) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); // kick off request } - } // namespace Http } // namespace Envoy diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 523cfb01ee9b..d1bc974e8b8c 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -76,6 +76,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(listenerStats, ConnectionManagerListenerStats&()); MOCK_CONST_METHOD0(proxy100Continue, bool()); MOCK_CONST_METHOD0(http1Settings, const Http::Http1Settings&()); + MOCK_CONST_METHOD0(shouldNormalizePath, bool()); std::unique_ptr internal_address_config_ = std::make_unique(); @@ -1101,5 +1102,38 @@ TEST_F(ConnectionManagerUtilityTest, RemovesProxyResponseHeaders) { EXPECT_FALSE(response_headers.has("proxy-connection")); } +// maybeNormalizePath() does nothing by default. +TEST_F(ConnectionManagerUtilityTest, SanitizePathDefaultOff) { + ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(false)); + HeaderMapImpl original_headers; + original_headers.insertPath().value(std::string("/xyz/../a")); + + HeaderMapImpl header_map(static_cast(original_headers)); + ConnectionManagerUtility::maybeNormalizePath(header_map, config_); + EXPECT_EQ(original_headers, header_map); +} + +// maybeNormalizePath() leaves already normal paths alone. +TEST_F(ConnectionManagerUtilityTest, SanitizePathNormalPath) { + ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true)); + HeaderMapImpl original_headers; + original_headers.insertPath().value(std::string("/xyz")); + + HeaderMapImpl header_map(static_cast(original_headers)); + ConnectionManagerUtility::maybeNormalizePath(header_map, config_); + EXPECT_EQ(original_headers, header_map); +} + +// maybeNormalizePath() normalizes relative paths. +TEST_F(ConnectionManagerUtilityTest, SanitizePathRelativePAth) { + ON_CALL(config_, shouldNormalizePath()).WillByDefault(Return(true)); + HeaderMapImpl original_headers; + original_headers.insertPath().value(std::string("/xyz/../abc")); + + HeaderMapImpl header_map(static_cast(original_headers)); + ConnectionManagerUtility::maybeNormalizePath(header_map, config_); + EXPECT_EQ(header_map.Path()->value().getStringView(), "/abc"); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/http/path_utility_test.cc b/test/common/http/path_utility_test.cc new file mode 100644 index 000000000000..2cc299465add --- /dev/null +++ b/test/common/http/path_utility_test.cc @@ -0,0 +1,89 @@ +#include +#include + +#include "common/http/header_map_impl.h" +#include "common/http/path_utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Http { + +class PathUtilityTest : public testing::Test { +public: + // This is an indirect way to build a header entry for + // PathUtil::canonicalPath(), since we don't have direct access to the + // HeaderMapImpl constructor. + HeaderEntry& pathHeaderEntry(const std::string& path_value) { + headers_.insertPath().value(path_value); + return *headers_.Path(); + } + HeaderMapImpl headers_; +}; + +// Already normalized path don't change. +TEST_F(PathUtilityTest, AlreadyNormalPaths) { + const std::vector normal_paths{"/xyz", "/x/y/z"}; + for (const auto& path : normal_paths) { + auto& path_header = pathHeaderEntry(path); + const auto result = PathUtil::canonicalPath(path_header); + EXPECT_TRUE(result) << "original path: " << path; + EXPECT_EQ(path_header.value().getStringView(), absl::string_view(path)); + } +} + +// Invalid paths are rejected. +TEST_F(PathUtilityTest, InvalidPaths) { + const std::vector invalid_paths{"/xyz/.%00../abc", "/xyz/%00.%00./abc", + "/xyz/AAAAA%%0000/abc"}; + for (const auto& path : invalid_paths) { + auto& path_header = pathHeaderEntry(path); + EXPECT_FALSE(PathUtil::canonicalPath(path_header)) << "original path: " << path; + } +} + +// Paths that are valid get normalized. +TEST_F(PathUtilityTest, NormalizeValidPaths) { + const std::vector> non_normal_pairs{ + {"/a/b/../c", "/a/c"}, // parent dir + {"/a/b/./c", "/a/b/c"}, // current dir + {"a/b/../c", "/a/c"}, // non / start + {"/a/b/../../../../c", "/c"}, // out number parent + {"/a/..\\c", "/c"}, // "..\\" canonicalization + {"/%c0%af", "/%c0%af"}, // 2 bytes unicode reserved characters + {"/%5c%25", "/%5c%25"}, // reserved characters + {"/a/b/%2E%2E/c", "/a/c"} // %2E escape + }; + + for (const auto& path_pair : non_normal_pairs) { + auto& path_header = pathHeaderEntry(path_pair.first); + const auto result = PathUtil::canonicalPath(path_header); + EXPECT_TRUE(result) << "original path: " << path_pair.first; + EXPECT_EQ(path_header.value().getStringView(), path_pair.second) + << "original path: " << path_pair.second; + } +} + +// Paths that are valid get normalized. +TEST_F(PathUtilityTest, NormalizeCasePath) { + const std::vector> non_normal_pairs{ + {"/A/B/C", "/A/B/C"}, // not normalize to lower case + {"/a/b/%2E%2E/c", "/a/c"}, // %2E can be normalized to . + {"/a/b/%2e%2e/c", "/a/c"}, // %2e can be normalized to . + {"/a/%2F%2f/c", "/a/%2F%2f/c"}, // %2F is not normalized to %2f + }; + + for (const auto& path_pair : non_normal_pairs) { + auto& path_header = pathHeaderEntry(path_pair.first); + const auto result = PathUtil::canonicalPath(path_header); + EXPECT_TRUE(result) << "original path: " << path_pair.first; + EXPECT_EQ(path_header.value().getStringView(), path_pair.second) + << "original path: " << path_pair.second; + } +} +// These test cases are explicitly not covered above: +// "/../c\r\n\" '\n' '\r' should be excluded by http parser +// "/a/\0c", '\0' should be excluded by http parser + +} // namespace Http +} // namespace Envoy diff --git a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc index cc2b654ac057..d79359d41a06 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc @@ -19,6 +19,18 @@ name: envoy.filters.http.rbac - any: true )EOF"; +const std::string RBAC_CONFIG_WITH_PREFIX_MATCH = R"EOF( +name: envoy.filters.http.rbac +config: + rules: + policies: + foo: + permissions: + - header: { name: ":path", prefix_match: "/foo" } + principals: + - any: true +)EOF"; + typedef HttpProtocolIntegrationTest RBACIntegrationTest; INSTANTIATE_TEST_SUITE_P(Protocols, RBACIntegrationTest, @@ -68,6 +80,58 @@ TEST_P(RBACIntegrationTest, Denied) { EXPECT_STREQ("403", response->headers().Status()->value().c_str()); } +TEST_P(RBACIntegrationTest, DeniedWithPrefixRule) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& cfg) { + cfg.mutable_normalize_path()->set_value(false); + }); + config_helper_.addFilter(RBAC_CONFIG_WITH_PREFIX_MATCH); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/foo/../bar"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + }, + 1024); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); +} + +TEST_P(RBACIntegrationTest, RbacPrefixRuleUseNormalizePath) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& cfg) { + cfg.mutable_normalize_path()->set_value(true); + }); + config_helper_.addFilter(RBAC_CONFIG_WITH_PREFIX_MATCH); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/foo/../bar"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + }, + 1024); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("403", response->headers().Status()->value().c_str()); +} + TEST_P(RBACIntegrationTest, DeniedHeadReply) { config_helper_.addFilter(RBAC_CONFIG); initialize(); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index d11761322838..aec8ee0da0b3 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -186,6 +186,77 @@ TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) { EXPECT_EQ(0, config.streamIdleTimeout().count()); } +// Validated that by default we don't normalize paths +TEST_F(HttpConnectionManagerConfigTest, NormalizePathDefault) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_); + EXPECT_FALSE(config.shouldNormalizePath()); +} + +// Validated that we normalize paths with runtime override when not specified. +TEST_F(HttpConnectionManagerConfigTest, NormalizePathRuntime) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + http_filters: + - name: envoy.router + )EOF"; + + EXPECT_CALL(context_.runtime_loader_.snapshot_, + featureEnabled("http_connection_manager.normalize_path", 0)) + .WillOnce(Return(true)); + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_); + EXPECT_TRUE(config.shouldNormalizePath()); +} + +// Validated that when configured, we normalize paths, ignoring runtime. +TEST_F(HttpConnectionManagerConfigTest, NormalizePathTrue) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + normalize_path: true + http_filters: + - name: envoy.router + )EOF"; + + EXPECT_CALL(context_.runtime_loader_.snapshot_, + featureEnabled("http_connection_manager.normalize_path", 0)) + .Times(0); + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_); + EXPECT_TRUE(config.shouldNormalizePath()); +} + +// Validated that when explicitly set false, we don't normalize, ignoring runtime. +TEST_F(HttpConnectionManagerConfigTest, NormalizePathFalse) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + normalize_path: false + http_filters: + - name: envoy.router + )EOF"; + + EXPECT_CALL(context_.runtime_loader_.snapshot_, + featureEnabled("http_connection_manager.normalize_path", 0)) + .Times(0); + HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, + date_provider_, route_config_provider_manager_); + EXPECT_FALSE(config.shouldNormalizePath()); +} + TEST_F(HttpConnectionManagerConfigTest, ConfiguredRequestTimeout) { const std::string yaml_string = R"EOF( stat_prefix: ingress_http diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index 484b4430b005..bfe95607a17a 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -145,6 +145,23 @@ stat_prefix: header_test - header: key: "authorization" value: "token2" + - name: path-sanitization + domains: ["path-sanitization.com"] + routes: + - match: { prefix: "/private" } + route: + cluster: cluster_0 + request_headers_to_add: + - header: + key: "x-site" + value: "private" + - match: { prefix: "/public" } + route: + cluster: cluster_0 + request_headers_to_add: + - header: + key: "x-site" + value: "public" )EOF"; } // namespace @@ -294,6 +311,8 @@ class HeaderIntegrationTest } } + hcm.mutable_normalize_path()->set_value(normalize_path_); + if (append) { // The config specifies append by default: no modifications needed. return; @@ -412,6 +431,7 @@ class HeaderIntegrationTest } bool use_eds_{false}; + bool normalize_path_{false}; FakeHttpConnectionPtr eds_connection_; FakeStreamPtr eds_stream_; }; @@ -988,4 +1008,63 @@ TEST_P(HeaderIntegrationTest, TestAppendSameHeaders) { }); } +// Validates behavior when normalize path is off. +// Route selection and path to upstream are the exact string literal +// from downstream. +TEST_P(HeaderIntegrationTest, TestPathAndRouteWhenNormalizePathOff) { + normalize_path_ = false; + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/private/../public"}, + {":scheme", "http"}, + {":authority", "path-sanitization.com"}, + }, + Http::TestHeaderMapImpl{{":authority", "path-sanitization.com"}, + {":path", "/private/../public"}, + {":method", "GET"}, + {"x-site", "private"}}, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} + +// Validates behavior when normalize path is on. +// Path to decide route and path to upstream are both +// the normalized. +TEST_P(HeaderIntegrationTest, TestPathAndRouteOnNormalizedPath) { + normalize_path_ = true; + initializeFilter(HeaderMode::Append, false); + performRequest( + Http::TestHeaderMapImpl{ + {":method", "GET"}, + {":path", "/private/../public"}, + {":scheme", "http"}, + {":authority", "path-sanitization.com"}, + }, + Http::TestHeaderMapImpl{{":authority", "path-sanitization.com"}, + {":path", "/public"}, + {":method", "GET"}, + {"x-site", "public"}}, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"content-length", "0"}, + {":status", "200"}, + {"x-unmodified", "response"}, + }, + Http::TestHeaderMapImpl{ + {"server", "envoy"}, + {"x-unmodified", "response"}, + {":status", "200"}, + }); +} } // namespace Envoy