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 <silentdai@gmail.com>
Signed-off-by: Harvey Tuch <htuch@google.com>

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<config_http_conn_man_runtime_normalize_path>`.
+  // See `Normalization and Comparison <https://tools.ietf.org/html/rfc3986#section-6>`
+  // for details of normalization.
+  // Note that Envoy does not perform
+  // `case normalization <https://tools.ietf.org/html/rfc3986#section-6.2.2.1>`
+  google.protobuf.BoolValue normalize_path = 30;
 }
 
 message Rds {

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 <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.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

docs/root/intro/version_history.rst

@@ -81,13 +81,17 @@ Version history
 * tracing: added :ref:`verbose <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.tracing>` to support logging annotations on spans.
 * upstream: added support for host weighting and :ref:`locality weighting <arch_overview_load_balancing_locality_weighted_lb>` in the :ref:`ring hash load balancer <arch_overview_load_balancing_types_ring_hash>`, and added a :ref:`maximum_ring_size<envoy_api_field_Cluster.RingHashLbConfig.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<config_network_filters_zookeeper_proxy>` for more details.
+  Refer to :ref:`ZooKeeper proxy<config_network_filters_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
+  <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.normalize_path>`
+  or the :ref:`runtime <config_http_conn_man_runtime_normalize_path>`.
 
 1.9.0 (Dec 20, 2018)
 ====================

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",
+    ],
+)

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

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)) {

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

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.

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<StaticLookupTable>::get().find(key.c_str());
   if (cb) {

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;

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<std::string> 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

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

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_);

source/extensions/filters/network/http_connection_manager/config.h

@@ -127,6 +127,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   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<Logger::Id::config>,
   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;

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();

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",
+    ],
+)

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<AccessLog::InstanceSharedPtr> access_logs_;
@@ -142,9 +143,10 @@ class FuzzConfig : public ConnectionManagerConfig {
   Network::Address::Ipv4Instance local_address_{"127.0.0.1"};
   absl::optional<std::string> 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