Fixed an UAF in DNS cache (#1709)

Fix for GHSA-g9vw-6pvx-7gmw

Signed-off-by: Yan Avlasov <[email protected]>

Author: yanavlasov

changelogs/current.yaml

@@ -110,6 +110,10 @@ bug_fixes:
   change: |
     Fixed an issue where cookies prefixed with ``__Secure-`` or ``__Host-`` were not receiving a
     Secure attribute.
+- area: dns
+  change: |
+    Fixed an UAF in DNS cache that can occur when the Host header is modified between the Dynamic Forwarding and Router
+    filters.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

@@ -642,12 +642,16 @@ void DnsCacheImpl::ThreadLocalHostInfo::onHostMapUpdate(
     const HostMapUpdateInfoSharedPtr& resolved_host) {
   auto host_it = pending_resolutions_.find(resolved_host->host_);
   if (host_it != pending_resolutions_.end()) {
-    for (auto* resolution : host_it->second) {
+    // Calling the onLoadDnsCacheComplete may trigger more host resolutions adding more elements
+    // to the `pending_resolutions_` map, potentially invalidating the host_it iterator. So we
+    // copy the list of handles to a local variable before cleaning up the map.
+    std::list<LoadDnsCacheEntryHandleImpl*> completed_resolutions(std::move(host_it->second));
+    pending_resolutions_.erase(host_it);
+    for (auto* resolution : completed_resolutions) {
       auto& callbacks = resolution->callbacks_;
       resolution->cancel();
       callbacks.onLoadDnsCacheComplete(resolved_host->info_);
     }
-    pending_resolutions_.erase(host_it);
   }
 }
 

test/extensions/filters/http/dynamic_forward_proxy/BUILD

@@ -66,6 +66,20 @@ envoy_cc_test_library(
     alwayslink = 1,
 )
 
+envoy_cc_test_library(
+    name = "modify_host_filter_lib",
+    srcs = ["modify_host_filter.cc"],
+    deps = [
+        "//envoy/http:filter_interface",
+        "//envoy/registry",
+        "//envoy/server:filter_config_interface",
+        "//source/extensions/filters/http/common:factory_base_lib",
+        "//source/extensions/filters/http/common:pass_through_filter_lib",
+        "//test/extensions/filters/http/common:empty_http_filter_config_lib",
+        "//test/integration/filters:common_lib",
+    ],
+)
+
 envoy_extension_cc_test(
     name = "proxy_filter_integration_test",
     size = "large",
@@ -79,6 +93,7 @@ envoy_extension_cc_test(
     #   https://gist.github.com/wrowe/a152cb1d12c2f751916122aed39d8517
     tags = ["fails_on_clang_cl"],
     deps = [
+        ":modify_host_filter_lib",
         ":test_resolver_lib",
         "//source/extensions/clusters/dns:dns_cluster_lib",
         "//source/extensions/clusters/dynamic_forward_proxy:cluster",
@@ -114,6 +129,7 @@ envoy_extension_cc_test(
     #   https://gist.github.com/wrowe/a152cb1d12c2f751916122aed39d8517
     tags = ["fails_on_clang_cl"],
     deps = [
+        ":modify_host_filter_lib",
         ":test_resolver_lib",
         "//source/extensions/clusters/dns:dns_cluster_lib",
         "//source/extensions/clusters/dynamic_forward_proxy:cluster",

test/extensions/filters/http/dynamic_forward_proxy/modify_host_filter.cc

@@ -0,0 +1,37 @@
+#include "envoy/http/filter.h"
+#include "envoy/registry/registry.h"
+#include "envoy/server/filter_config.h"
+
+#include "source/extensions/filters/http/common/factory_base.h"
+#include "source/extensions/filters/http/common/pass_through_filter.h"
+
+#include "test/integration/filters/common.h"
+
+namespace Envoy {
+
+class ModifyHostFilter : public Http::PassThroughFilter {
+public:
+  ModifyHostFilter() = default;
+  Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override {
+    headers.setHost("non-existing.foo.bar.bats.com");
+    return Http::FilterHeadersStatus::Continue;
+  }
+};
+
+class ModifyHostFilterFactory : public Extensions::HttpFilters::Common::EmptyHttpDualFilterConfig {
+public:
+  ModifyHostFilterFactory() : EmptyHttpDualFilterConfig("modify-host-filter") {}
+  absl::StatusOr<Http::FilterFactoryCb>
+  createDualFilter(const std::string&, Server::Configuration::ServerFactoryContext&) override {
+    return [](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+      callbacks.addStreamFilter(std::make_shared<::Envoy::ModifyHostFilter>());
+    };
+  }
+};
+
+// perform static registration
+static Registry::RegisterFactory<ModifyHostFilterFactory,
+                                 Server::Configuration::NamedHttpFilterConfigFactory>
+    register_;
+
+} // namespace Envoy

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

@@ -112,7 +112,8 @@ class ProxyFilterIntegrationTest : public testing::TestWithParam<Network::Addres
       bool use_sub_cluster = false, double dns_query_timeout = 5,
       bool disable_dns_refresh_on_failure = false,
       bool allow_dynamic_host_from_filter_state = false,
-      const absl::optional<std::string>& prepend_custom_filter_config_yaml = absl::nullopt) {
+      const absl::optional<std::string>& prepend_custom_filter_config_yaml = absl::nullopt,
+      bool use_dfp_even_when_cluster_resolves_hosts = false) {
     const std::string filter_use_sub_cluster = R"EOF(
 name: dynamic_forward_proxy
 typed_config:
@@ -147,7 +148,8 @@ name: stream-info-to-headers-filter
 
     if (prepend_custom_filter_config_yaml.has_value()) {
       // Prepend DFP filter.
-      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
+      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts") ||
+          use_dfp_even_when_cluster_resolves_hosts) {
         config_helper_.prependFilter(use_sub_cluster ? filter_use_sub_cluster
                                                      : filter_use_dns_cache);
       } else if (use_sub_cluster) {
@@ -162,7 +164,8 @@ name: stream-info-to-headers-filter
       // Prepend stream_info_filter.
       config_helper_.prependFilter(stream_info_filter_config_str);
     } else {
-      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts")) {
+      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.dfp_cluster_resolves_hosts") ||
+          use_dfp_even_when_cluster_resolves_hosts) {
         config_helper_.prependFilter(use_sub_cluster ? filter_use_sub_cluster
                                                      : filter_use_dns_cache);
       } else if (use_sub_cluster) {
@@ -1677,5 +1680,31 @@ TEST_P(ProxyFilterIntegrationTest, ResetStreamDuringDnsLookup) {
   EXPECT_EQ("504", response->headers().getStatusValue());
 }
 
+// This test validates that processing of DNS resolutions on worker threads is handled correctly.
+// The test uses specific scenario where DFP filter AND async resolution in DFP cluster are enabled.
+// Normally DFP filter is not needed, however this configuration can occur as the
+// envoy.reloadable_features.dfp_cluster_resolves_hosts flag is now enabled by default. The test
+// also requires the Host header to be modified between DFP and Router filters to trigger abnormal
+// behavior in the DNS resolution processing loop.
+TEST_P(ProxyFilterIntegrationTest, DoubleResolution) {
+  config_helper_.addRuntimeOverride("envoy.reloadable_features.dfp_cluster_resolves_hosts", "true");
+  config_helper_.addRuntimeOverride(
+      "envoy.reloadable_features.skip_dns_lookup_for_proxied_requests", "true");
+  upstream_tls_ = false;
+  autonomous_upstream_ = true;
+  // Add DFP filter even if async DNS resolution is enabled.
+  config_helper_.prependFilter("{ name: modify-host-filter }");
+  initializeWithArgs(1024, 1024, "", typed_dns_resolver_config_, false, 5, false, false,
+                     absl::nullopt, true);
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+
+  ASSERT_TRUE(response->waitForEndStream());
+  // The host modification filter sets a non-existing host which should result in a 503.
+  EXPECT_EQ("503", response->headers().getStatusValue());
+}
+
 } // namespace
 } // namespace Envoy