[IMPORTANT] Multiple CVEs

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -684,6 +684,34 @@ message HttpConnectionManager {
   // purposes. If unspecified, only RFC1918 IP addresses will be considered internal.
   // See the documentation for :ref:`config_http_conn_man_headers_x-envoy-internal` for more
   // information about internal/external addresses.
+  //
+  // .. warning::
+  //     In the next release, no IP addresses will be considered trusted. If you have tooling such as probes
+  //     on your private network which need to be treated as trusted (e.g. changing arbitrary x-envoy headers)
+  //     you will have to manually include those addresses or CIDR ranges like:
+  //
+  // .. validated-code-block:: yaml
+  //   :type-name: envoy.extensions.filters.network.http_connection_manager.v3.InternalAddressConfig
+  //
+  //   cidr_ranges:
+  //       address_prefix: 10.0.0.0
+  //       prefix_len: 8
+  //   cidr_ranges:
+  //       address_prefix: 192.168.0.0
+  //       prefix_len: 16
+  //   cidr_ranges:
+  //       address_prefix: 172.16.0.0
+  //       prefix_len: 12
+  //   cidr_ranges:
+  //       address_prefix: 127.0.0.1
+  //       prefix_len: 32
+  //   cidr_ranges:
+  //       address_prefix: fd00::
+  //       prefix_len: 8
+  //   cidr_ranges:
+  //       address_prefix: ::1
+  //       prefix_len: 128
+  //
   InternalAddressConfig internal_address_config = 25;
 
   // If set, Envoy will not append the remote address to the

changelogs/current.yaml

@@ -39,6 +39,14 @@ behavior_changes:
   change: |
     Added new tag extraction so that scoped rds stats have their :ref:'scope_route_config_name
     <envoy_v3_api_msg_config/route/v3/scoped_route>' and stat prefix extracted.
+- area: http
+  change: |
+    The default configuration of Envoy will continue to trust internal addresses while in the future it will not trust them by default.
+    If you have tooling such as probes on your private network which need to be treated as trusted (e.g. changing arbitrary ``x-envoy``
+    headers) please explictily include those addresses or CIDR ranges into :ref:`internal_address_config
+    <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.internal_address_config>`
+    See the config examples from the above ``internal_address_config`` link. This default no trust internal address can be turned on by
+    setting runtime guard ``envoy.reloadable_features.explicit_internal_address_config`` to ``true``.
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
@@ -86,6 +94,14 @@ minor_behavior_changes:
   change: |
     Enhanced listener filter chain execution to include the case that listener filter has maxReadBytes() of 0,
     but may return StopIteration in onAccept to wait for asynchronous callback.
+- area: http2
+  change: |
+    Changes the default value of ``envoy.reloadable_features.http2_use_oghttp2`` to ``false``. This changes the codec used for HTTP/2
+    requests and responses to address to address stability concerns. This behavior can be reverted by setting the feature to ``true``.
+- area: access_log
+  change: |
+    Sanitize SNI for potential log injection. The invalid character will be replaced by ``_`` with an ``invalid:`` marker. If runtime
+    flag ``envoy.reloadable_features.sanitize_sni_in_access_log`` is set to ``false``, the sanitize behavior is disabled.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
@@ -143,6 +159,14 @@ bug_fixes:
     Fixed an inconsistency in how boolean values are loaded in RTDS, where they were previously converted to "1"/"0"
     instead of "true"/"false". The correct string representation ("true"/"false") will now be used. This change can be
     reverted by setting the runtime guard ``envoy.reloadable_features.boolean_to_string_fix`` to false.
+- area: jwt
+  change: |
+    Fixed a bug where using ``clear_route_cache`` with remote JWKs works
+    incorrectly and may cause a crash when the modified request does not match
+    any route.
+- area: http_async_client
+  change: |
+    Fixed the local reply and destroy order crashes when using the http async client for websocket handshake.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/common/utility.cc

@@ -586,6 +586,23 @@ void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) {
   }
 }
 
+std::string StringUtil::sanitizeInvalidHostname(const absl::string_view source) {
+  std::string ret_str = std::string(source);
+  bool sanitized = false;
+  for (size_t i = 0; i < ret_str.size(); ++i) {
+    if (absl::ascii_isalnum(ret_str[i]) || ret_str[i] == '.' || ret_str[i] == '-') {
+      continue;
+    }
+    sanitized = true;
+    ret_str[i] = '_';
+  }
+
+  if (sanitized) {
+    ret_str = absl::StrCat("invalid:", ret_str);
+  }
+  return ret_str;
+}
+
 const std::string& getDefaultDateFormat(bool local_time) {
   if (local_time) {
     CONSTRUCT_ON_FIRST_USE(std::string, "%Y-%m-%dT%H:%M:%E3S%z");

source/common/common/utility.h

@@ -490,6 +490,15 @@ class StringUtil {
    */
   static void escapeToOstream(std::ostream& os, absl::string_view view);
 
+  /**
+   * Sanitize host name strings for logging purposes. Replace invalid hostname characters (anything
+   * that's not alphanumeric, hyphen, or period) with underscore. The sanitized string is not a
+   * valid host name.
+   * @param source supplies the string to sanitize.
+   * @return sanitized string.
+   */
+  static std::string sanitizeInvalidHostname(const absl::string_view source);
+
   /**
    * Provide a default value for a string if empty.
    * @param s string.

source/common/formatter/stream_info_formatter.cc

@@ -1353,8 +1353,14 @@ const StreamInfoFormatterProviderLookupTable& getKnownStreamInfoFormatterProvide
                   [](const StreamInfo::StreamInfo& stream_info) {
                     absl::optional<std::string> result;
                     if (!stream_info.downstreamAddressProvider().requestedServerName().empty()) {
-                      result = std::string(
-                          stream_info.downstreamAddressProvider().requestedServerName());
+                      if (Runtime::runtimeFeatureEnabled(
+                              "envoy.reloadable_features.sanitize_sni_in_access_log")) {
+                        result = StringUtil::sanitizeInvalidHostname(
+                            stream_info.downstreamAddressProvider().requestedServerName());
+                      } else {
+                        result = std::string(
+                            stream_info.downstreamAddressProvider().requestedServerName());
+                      }
                     }
                     return result;
                   });

source/common/http/async_client_impl.cc

@@ -9,6 +9,7 @@
 #include "source/common/grpc/common.h"
 #include "source/common/http/null_route_impl.h"
 #include "source/common/http/utility.h"
+#include "source/common/local_reply/local_reply.h"
 #include "source/common/protobuf/message_validator_impl.h"
 #include "source/common/stream_info/filter_state_impl.h"
 #include "source/common/tracing/http_tracer_impl.h"
@@ -32,7 +33,8 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterInfoConstSharedPtr cluster,
           factory_context.api().randomGenerator(), std::move(shadow_writer), true, false, false,
           false, false, false, Protobuf::RepeatedPtrField<std::string>{}, dispatcher.timeSource(),
           http_context, router_context)),
-      dispatcher_(dispatcher), runtime_(factory_context.runtime()) {}
+      dispatcher_(dispatcher), runtime_(factory_context.runtime()),
+      local_reply_(LocalReply::Factory::createDefault()) {}
 
 AsyncClientImpl::~AsyncClientImpl() {
   while (!active_streams_.empty()) {
@@ -120,7 +122,7 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal
                        ? options.filter_state
                        : std::make_shared<StreamInfo::FilterStateImpl>(
                              StreamInfo::FilterState::LifeSpan::FilterChain)),
-      tracing_config_(Tracing::EgressConfig::get()),
+      tracing_config_(Tracing::EgressConfig::get()), local_reply_(*parent.local_reply_),
       retry_policy_(createRetryPolicy(parent, options, parent_.factory_context_, creation_status)),
       route_(std::make_shared<NullRouteImpl>(
           parent_.cluster_->name(),
@@ -145,6 +147,35 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal
   // TODO(mattklein123): Correctly set protocol in stream info when we support access logging.
 }
 
+void AsyncStreamImpl::sendLocalReply(Code code, absl::string_view body,
+                                     std::function<void(ResponseHeaderMap& headers)> modify_headers,
+                                     const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
+                                     absl::string_view details) {
+  if (encoded_response_headers_) {
+    resetStream();
+    return;
+  }
+  Utility::sendLocalReply(
+      remote_closed_,
+      Utility::EncodeFunctions{
+          [modify_headers](ResponseHeaderMap& headers) -> void {
+            if (modify_headers != nullptr) {
+              modify_headers(headers);
+            }
+          },
+          [this](ResponseHeaderMap& response_headers, Code& code, std::string& body,
+                 absl::string_view& content_type) -> void {
+            local_reply_.rewrite(request_headers_, response_headers, stream_info_, code, body,
+                                 content_type);
+          },
+          [this, &details](ResponseHeaderMapPtr&& headers, bool end_stream) -> void {
+            encodeHeaders(std::move(headers), end_stream, details);
+          },
+          [this](Buffer::Instance& data, bool end_stream) -> void {
+            encodeData(data, end_stream);
+          }},
+      Utility::LocalReplyData{is_grpc_request_, code, body, grpc_status, is_head_request_});
+}
 void AsyncStreamImpl::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
                                     absl::string_view) {
   ENVOY_LOG(debug, "async http request response headers (end_stream={}):\n{}", end_stream,
@@ -288,16 +319,24 @@ void AsyncStreamImpl::closeRemote(bool end_stream) {
 }
 
 void AsyncStreamImpl::reset() {
-  router_.onDestroy();
+  routerDestroy();
   resetStream();
 }
 
+void AsyncStreamImpl::routerDestroy() {
+  if (!router_destroyed_) {
+    router_destroyed_ = true;
+    router_.onDestroy();
+  }
+}
+
 void AsyncStreamImpl::cleanup() {
   ASSERT(dispatcher().isThreadSafe());
   local_closed_ = remote_closed_ = true;
   // This will destroy us, but only do so if we are actually in a list. This does not happen in
   // the immediate failure case.
   if (inserted()) {
+    routerDestroy();
     dispatcher().deferredDelete(removeFromList(parent_.active_streams_));
   }
 }

source/common/http/async_client_impl.h

@@ -38,6 +38,7 @@
 #include "source/common/common/linked_object.h"
 #include "source/common/http/message_impl.h"
 #include "source/common/http/null_route_impl.h"
+#include "source/common/local_reply/local_reply.h"
 #include "source/common/router/config_impl.h"
 #include "source/common/router/router.h"
 #include "source/common/stream_info/stream_info_impl.h"
@@ -84,6 +85,7 @@ class AsyncClientImpl final : public AsyncClient {
   Event::Dispatcher& dispatcher_;
   std::list<std::unique_ptr<AsyncStreamImpl>> active_streams_;
   Runtime::Loader& runtime_;
+  const LocalReply::LocalReplyPtr local_reply_;
 
   friend class AsyncStreamImpl;
   friend class AsyncRequestSharedImpl;
@@ -109,7 +111,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream,
   }
 
   ~AsyncStreamImpl() override {
-    router_.onDestroy();
+    routerDestroy();
     // UpstreamRequest::cleanUp() is guaranteed to reset the high watermark calls.
     ENVOY_BUG(high_watermark_calls_ == 0, "Excess high watermark calls after async stream ended.");
     if (destructor_callback_.has_value()) {
@@ -171,6 +173,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream,
   void cleanup();
   void closeRemote(bool end_stream);
   bool complete() { return local_closed_ && remote_closed_; }
+  void routerDestroy();
 
   // Http::StreamDecoderFilterCallbacks
   OptRef<const Network::Connection> connection() override { return {}; }
@@ -201,26 +204,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream,
   void sendLocalReply(Code code, absl::string_view body,
                       std::function<void(ResponseHeaderMap& headers)> modify_headers,
                       const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
-                      absl::string_view details) override {
-    if (encoded_response_headers_) {
-      resetStream();
-      return;
-    }
-    Utility::sendLocalReply(
-        remote_closed_,
-        Utility::EncodeFunctions{nullptr, nullptr,
-                                 [this, modify_headers, &details](ResponseHeaderMapPtr&& headers,
-                                                                  bool end_stream) -> void {
-                                   if (modify_headers != nullptr) {
-                                     modify_headers(*headers);
-                                   }
-                                   encodeHeaders(std::move(headers), end_stream, details);
-                                 },
-                                 [this](Buffer::Instance& data, bool end_stream) -> void {
-                                   encodeData(data, end_stream);
-                                 }},
-        Utility::LocalReplyData{is_grpc_request_, code, body, grpc_status, is_head_request_});
-  }
+                      absl::string_view details) override;
   // The async client won't pause if sending 1xx headers so simply swallow any.
   void encode1xxHeaders(ResponseHeaderMapPtr&&) override {}
   void encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
@@ -290,6 +274,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream,
   StreamInfo::StreamInfoImpl stream_info_;
   Tracing::NullSpan active_span_;
   const Tracing::Config& tracing_config_;
+  const LocalReply::LocalReply& local_reply_;
   const std::unique_ptr<const Router::RetryPolicy> retry_policy_;
   std::shared_ptr<NullRouteImpl> route_;
   uint32_t high_watermark_calls_{};
@@ -305,6 +290,7 @@ class AsyncStreamImpl : public virtual AsyncClient::Stream,
   bool is_head_request_{false};
   bool send_xff_{true};
   bool send_internal_{true};
+  bool router_destroyed_{false};
 
   friend class AsyncClientImpl;
   friend class AsyncClientImplUnitTest;

source/common/http/conn_manager_config.h

@@ -191,6 +191,10 @@ class InternalAddressConfig {
 class DefaultInternalAddressConfig : public Http::InternalAddressConfig {
 public:
   bool isInternalAddress(const Network::Address::Instance& address) const override {
+    if (Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.explicit_internal_address_config")) {
+      return false;
+    }
     return Network::Utility::isInternalAddress(address);
   }
 };

source/common/runtime/runtime_features.cc

@@ -56,7 +56,6 @@ RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_disallow_lone_cr_in_chunk_ex
 RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser);
 RUNTIME_GUARD(envoy_reloadable_features_http2_discard_host_header);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year.
-RUNTIME_GUARD(envoy_reloadable_features_http2_use_oghttp2);
 RUNTIME_GUARD(envoy_reloadable_features_http2_use_visitor_for_data);
 RUNTIME_GUARD(envoy_reloadable_features_http3_happy_eyeballs);
 RUNTIME_GUARD(envoy_reloadable_features_http3_remove_empty_trailers);
@@ -87,6 +86,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_socket_use_address_cache_f
 RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml);
 RUNTIME_GUARD(envoy_reloadable_features_report_stream_reset_error_code);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_http2_headers_without_nghttp2);
+RUNTIME_GUARD(envoy_reloadable_features_sanitize_sni_in_access_log);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_te);
 RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request);
 RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests);
@@ -123,6 +123,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_test_feature_false);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_streaming_shadow);
 // TODO(adisuissa) reset to true to enable unified mux by default
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_unified_mux);
+// TODO(birenroy) flip after the security issue is addressed.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_http2_use_oghttp2);
 // Used to track if runtime is initialized.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_runtime_initialized);
 // TODO(mattklein123): Flip this to true and/or remove completely once verified by Envoy Mobile.
@@ -152,6 +154,8 @@ FALSE_RUNTIME_GUARD(envoy_restart_features_xds_failover_support);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_dns_cache_set_ip_version_to_remove);
 // TODO(alyssawilk): evaluate and make this a config knob or remove.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_reset_brokenness_on_nework_change);
+// TODO(botengyao): this will be default true in the next release after this warning release.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_explicit_internal_address_config);
 
 // A flag to set the maximum TLS version for google_grpc client to TLS1.2, when needed for
 // compliance restrictions.

source/extensions/filters/http/jwt_authn/authenticator.cc

@@ -480,6 +480,11 @@ void AuthenticatorImpl::doneWithStatus(const Status& status) {
   // Unless allowing failed or missing, all tokens must be verified successfully.
   if ((Status::Ok != status && !is_allow_failed_ && !is_allow_missing_) || tokens_.empty()) {
     tokens_.clear();
+    if (clear_route_cache_ && clear_route_cb_) {
+      clear_route_cb_();
+    }
+    clear_route_cb_ = nullptr;
+
     if (is_allow_failed_) {
       callback_(Status::Ok);
     } else if (is_allow_missing_ && status == Status::JwtMissed) {
@@ -489,11 +494,6 @@ void AuthenticatorImpl::doneWithStatus(const Status& status) {
     }
     callback_ = nullptr;
 
-    if (clear_route_cache_ && clear_route_cb_) {
-      clear_route_cb_();
-    }
-    clear_route_cb_ = nullptr;
-
     return;
   }
 

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

@@ -77,6 +77,10 @@ std::unique_ptr<Http::InternalAddressConfig> createInternalAddressConfig(
     return std::make_unique<InternalAddressConfig>(config.internal_address_config(),
                                                    creation_status);
   }
+  ENVOY_LOG_ONCE_MISC(warn,
+                      "internal_address_config is not configured. The existing default behaviour "
+                      "will trust RFC1918 IP addresses, but this will be changed in next release. "
+                      "Please explictily config internal address config as the migration step.");
 
   return std::make_unique<Http::DefaultInternalAddressConfig>();
 }