CVE-2024-32475: Insufficient Information (NVD-CWE-noinfo)
value: Envoy is a cloud-native, open source edge and service proxy. When an upstream TLS cluster is used with `auto_sni` enabled, a request containing a `host`/`:authority` header longer than 255 characters triggers an abnormal termination of Envoy process. Envoy does not gracefully handle an error when setting SNI for outbound TLS connection. The error can occur when Envoy attempts to use the `host`/`:authority` header value longer than 255 characters as SNI for outbound TLS connection. SNI length is limited to 255 characters per the standard. Envoy always expects this operation to succeed and abnormally aborts the process when it fails. This vulnerability is fixed in 1.30.1, 1.29.4, 1.28.3, and 1.27.5.

CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-04-18T15:15Z
Repository: https://github.com/envoyproxy/envoy
Language: C++
Files changed: changelogs/current.yaml, source/common/tls/context_impl.cc, source/common/tls/context_impl.h, source/common/tls/ssl_socket.cc, source/common/tls/ssl_socket.h, test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
Commit date: 2024-04-18 00:33:22 +0000
Commit message: tls: fix RELEASE_ASSERT when using `auto_sni` (#33637)

* tls: fix RELEASE_ASSERT when using `auto_sni`

If the `:authority` was longer than 255 characters, Envoy would
RELEASE_ASSERT when creating an upstream TLS connection when
`auto_sni` (https://www.envoyproxy.io/docs/envoy/v1.30.0/api-v3/config/core/v3/protocol.proto.html#config-core-v3-upstreamhttpprotocoloptions)
was used.

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Diff: diff --git a/changelogs/current.yaml b/changelogs/current.yaml
index 093541643ac..07e7712a124 100644
--- a/changelogs/current.yaml
+++ b/changelogs/current.yaml
@@ -11,10 +11,15 @@ behavior_changes:
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: tls
+  change: |
+    Fix a RELEASE_ASSERT when using :ref:`auto_sni <envoy_v3_api_field_config.core.v3.UpstreamHttpProtocolOptions.auto_sni>`
+    if the downstream request ``:authority`` was longer than 255 characters.
+
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
 
 new_features:
diff --git a/source/common/tls/context_impl.cc b/source/common/tls/context_impl.cc
index 2669fd8559f..223bf44dff5 100644
--- a/source/common/tls/context_impl.cc
+++ b/source/common/tls/context_impl.cc
@@ -461,11 +461,11 @@ std::vector<uint8_t> ContextImpl::parseAlpnProtocols(const std::string& alpn_pro
   }
 
   return out;
 }
 
-bssl::UniquePtr<SSL>
+absl::StatusOr<bssl::UniquePtr<SSL>>
 ContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) {
   // We use the first certificate for a new SSL object, later in the
   // SSL_CTX_set_select_certificate_cb() callback following ClientHello, we replace with the
   // selected certificate via SSL_set_SSL_CTX().
   auto ssl_con = bssl::UniquePtr<SSL>(SSL_new(tls_contexts_[0].ssl_ctx_.get()));
@@ -699,20 +699,29 @@ bool ContextImpl::parseAndSetAlpn(const std::vector<std::string>& alpn, SSL& ssl
   }
 
   return false;
 }
 
-bssl::UniquePtr<SSL>
+absl::StatusOr<bssl::UniquePtr<SSL>>
 ClientContextImpl::newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) {
-  bssl::UniquePtr<SSL> ssl_con(ContextImpl::newSsl(options));
+  absl::StatusOr<bssl::UniquePtr<SSL>> ssl_con_or_status(ContextImpl::newSsl(options));
+  if (!ssl_con_or_status.ok()) {
+    return ssl_con_or_status;
+  }
+
+  bssl::UniquePtr<SSL> ssl_con = std::move(ssl_con_or_status.value());
 
   const std::string server_name_indication = options && options->serverNameOverride().has_value()
                                                  ? options->serverNameOverride().value()
                                                  : server_name_indication_;
   if (!server_name_indication.empty()) {
     const int rc = SSL_set_tlsext_host_name(ssl_con.get(), server_name_indication.c_str());
-    RELEASE_ASSERT(rc, Utility::getLastCryptoError().value_or(""));
+    if (rc != 1) {
+      return absl::InvalidArgumentError(
+          absl::StrCat("Failed to create upstream TLS due to failure setting SNI: ",
+                       Utility::getLastCryptoError().value_or("unknown")));
+    }
   }
 
   if (options && !options->verifySubjectAltNameListOverride().empty()) {
     SSL_set_verify(ssl_con.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr);
   }
diff --git a/source/common/tls/context_impl.h b/source/common/tls/context_impl.h
index 71f04229af5..b445df66e3c 100644
--- a/source/common/tls/context_impl.h
+++ b/source/common/tls/context_impl.h
@@ -76,11 +76,12 @@ namespace TransportSockets {
 namespace Tls {
 
 class ContextImpl : public virtual Envoy::Ssl::Context,
                     protected Logger::Loggable<Logger::Id::config> {
 public:
-  virtual bssl::UniquePtr<SSL> newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
+  virtual absl::StatusOr<bssl::UniquePtr<SSL>>
+  newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
 
   /**
    * Logs successful TLS handshake and updates stats.
    * @param ssl the connection to log
    */
@@ -174,11 +175,11 @@ using ContextImplSharedPtr = std::shared_ptr<ContextImpl>;
 class ClientContextImpl : public ContextImpl, public Envoy::Ssl::ClientContext {
 public:
   ClientContextImpl(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config,
                     Server::Configuration::CommonFactoryContext& factory_context);
 
-  bssl::UniquePtr<SSL>
+  absl::StatusOr<bssl::UniquePtr<SSL>>
   newSsl(const Network::TransportSocketOptionsConstSharedPtr& options) override;
 
 private:
   int newSessionKey(SSL_SESSION* session);
 
diff --git a/source/common/tls/ssl_socket.cc b/source/common/tls/ssl_socket.cc
index 02111ead009..20c38d2a620 100644
--- a/source/common/tls/ssl_socket.cc
+++ b/source/common/tls/ssl_socket.cc
@@ -24,17 +24,15 @@ namespace Tls {
 
 namespace {
 
 constexpr absl::string_view NotReadyReason{"TLS error: Secret is not supplied by SDS"};
 
-// This SslSocket will be used when SSL secret is not fetched from SDS server.
-class NotReadySslSocket : public Network::TransportSocket {
+class InvalidSslSocket : public Network::TransportSocket {
 public:
   // Network::TransportSocket
   void setTransportSocketCallbacks(Network::TransportSocketCallbacks&) override {}
   std::string protocol() const override { return EMPTY_STRING; }
-  absl::string_view failureReason() const override { return NotReadyReason; }
   bool canFlushClose() override { return true; }
   void closeSocket(Network::ConnectionEvent) override {}
   Network::IoResult doRead(Buffer::Instance&) override { return {PostIoAction::Close, 0, false}; }
   Network::IoResult doWrite(Buffer::Instance&, bool) override {
     return {PostIoAction::Close, 0, false};
@@ -43,25 +41,66 @@ public:
   Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; }
   bool startSecureTransport() override { return false; }
   void configureInitialCongestionWindow(uint64_t, std::chrono::microseconds) override {}
 };
 
+// This SslSocket will be used when SSL secret is not fetched from SDS server.
+class NotReadySslSocket : public InvalidSslSocket {
+public:
+  // Network::TransportSocket
+  absl::string_view failureReason() const override { return NotReadyReason; }
+};
+
+class ErrorSslSocket : public InvalidSslSocket {
+public:
+  ErrorSslSocket(absl::string_view error) : error_(error) {}
+
+  // Network::TransportSocket
+  absl::string_view failureReason() const override { return error_; }
+
+private:
+  std::string error_;
+};
+
 } // namespace
 
-SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
-                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
-                     Ssl::HandshakerFactoryCb handshaker_factory_cb)
+absl::StatusOr<std::unique_ptr<SslSocket>>
+SslSocket::create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
+                  const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
+                  Ssl::HandshakerFactoryCb handshaker_factory_cb) {
+  std::unique_ptr<SslSocket> socket(new SslSocket(ctx, transport_socket_options));
+  auto status = socket->initialize(state, handshaker_factory_cb);
+  if (status.ok()) {
+    return socket;
+  } else {
+    return status;
+  }
+}
+
+SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
+                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options)
     : transport_socket_options_(transport_socket_options),
-      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)),
-      info_(std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
-          ctx_->newSsl(transport_socket_options_), ctx_->sslExtendedSocketInfoIndex(), this))) {
+      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)) {}
+
+absl::Status SslSocket::initialize(InitialState state,
+                                   Ssl::HandshakerFactoryCb handshaker_factory_cb) {
+  auto status_or_ssl = ctx_->newSsl(transport_socket_options_);
+  if (!status_or_ssl.ok()) {
+    return status_or_ssl.status();
+  }
+
+  info_ = std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
+      std::move(status_or_ssl.value()), ctx_->sslExtendedSocketInfoIndex(), this));
+
   if (state == InitialState::Client) {
     SSL_set_connect_state(rawSsl());
   } else {
     ASSERT(state == InitialState::Server);
     SSL_set_accept_state(rawSsl());
   }
+
+  return absl::OkStatus();
 }
 
 void SslSocket::setTransportSocketCallbacks(Network::TransportSocketCallbacks& callbacks) {
   ASSERT(!callbacks_);
   callbacks_ = &callbacks;
@@ -399,12 +438,17 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket(
   {
     absl::ReaderMutexLock l(&ssl_ctx_mu_);
     ssl_ctx = ssl_ctx_;
   }
   if (ssl_ctx) {
-    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Client,
-                                       transport_socket_options, config_->createHandshaker());
+    auto status_or_socket =
+        SslSocket::create(std::move(ssl_ctx), InitialState::Client, transport_socket_options,
+                          config_->createHandshaker());
+    if (status_or_socket.ok()) {
+      return std::move(status_or_socket.value());
+    }
+    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
   } else {
     ENVOY_LOG(debug, "Create NotReadySslSocket");
     stats_.upstream_context_secrets_not_ready_.inc();
     return std::make_unique<NotReadySslSocket>();
   }
@@ -448,12 +492,16 @@ Network::TransportSocketPtr ServerSslSocketFactory::createDownstreamTransportSoc
   {
     absl::ReaderMutexLock l(&ssl_ctx_mu_);
     ssl_ctx = ssl_ctx_;
   }
   if (ssl_ctx) {
-    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Server, nullptr,
-                                       config_->createHandshaker());
+    auto status_or_socket = SslSocket::create(std::move(ssl_ctx), InitialState::Server, nullptr,
+                                              config_->createHandshaker());
+    if (status_or_socket.ok()) {
+      return std::move(status_or_socket.value());
+    }
+    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
   } else {
     ENVOY_LOG(debug, "Create NotReadySslSocket");
     stats_.downstream_context_secrets_not_ready_.inc();
     return std::make_unique<NotReadySslSocket>();
   }
diff --git a/source/common/tls/ssl_socket.h b/source/common/tls/ssl_socket.h
index 2e4392c5cdd..a412b1e7ecb 100644
--- a/source/common/tls/ssl_socket.h
+++ b/source/common/tls/ssl_socket.h
@@ -46,13 +46,14 @@ enum class InitialState { Client, Server };
 class SslSocket : public Network::TransportSocket,
                   public Envoy::Ssl::PrivateKeyConnectionCallbacks,
                   public Ssl::HandshakeCallbacks,
                   protected Logger::Loggable<Logger::Id::connection> {
 public:
-  SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
-            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
-            Ssl::HandshakerFactoryCb handshaker_factory_cb);
+  static absl::StatusOr<std::unique_ptr<SslSocket>>
+  create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
+         const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
+         Ssl::HandshakerFactoryCb handshaker_factory_cb);
 
   // Network::TransportSocket
   void setTransportSocketCallbacks(Network::TransportSocketCallbacks& callbacks) override;
   std::string protocol() const override;
   absl::string_view failureReason() const override;
@@ -77,10 +78,14 @@ public:
 
 protected:
   SSL* rawSsl() const { return info_->ssl(); }
 
 private:
+  SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
+            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options);
+  absl::Status initialize(InitialState state, Ssl::HandshakerFactoryCb handshaker_factory_cb);
+
   struct ReadResult {
     uint64_t bytes_read_{0};
     absl::optional<int> error_;
   };
   ReadResult sslReadIntoSlice(Buffer::RawSlice& slice);
diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
index 44fafa92c7f..ff6958c4134 100644
--- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
+++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
@@ -600,10 +600,28 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithIpHost) {
   upstream_request_->encodeHeaders(default_response_headers_, true);
   ASSERT_TRUE(response->waitForEndStream());
   checkSimpleRequestSuccess(0, 0, response.get());
 }
 
+TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithTooLongSni) {
+  upstream_tls_ = true;
+  initializeWithArgs(1024, 1024, "x-host");
+  std::string too_long_sni(300, 'a');
+  ASSERT_EQ(too_long_sni.size(), 300); // Validate that the expected constructor was run.
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  const Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
+                                                       {":path", "/test/long/url"},
+                                                       {":scheme", "http"},
+                                                       {":authority", "localhost"},
+                                                       {"x-host", too_long_sni}};
+
+  auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_EQ("503", response->headers().getStatusValue());
+  // TODO(ggreenway): validate (in access logs probably) that failure reason is set appropriately.
+}
+
 // Verify that auto-SAN verification fails with an incorrect certificate.
 TEST_P(ProxyFilterIntegrationTest, UpstreamTlsInvalidSAN) {
   upstream_tls_ = true;
   upstream_cert_name_ = "upstream";
   initializeWithArgs();

Vulnerable code: bssl::UniquePtr<SSL>
bssl::UniquePtr<SSL>
  bssl::UniquePtr<SSL> ssl_con(ContextImpl::newSsl(options));
    RELEASE_ASSERT(rc, Utility::getLastCryptoError().value_or(""));
  virtual bssl::UniquePtr<SSL> newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
  bssl::UniquePtr<SSL>
// This SslSocket will be used when SSL secret is not fetched from SDS server.
class NotReadySslSocket : public Network::TransportSocket {
  absl::string_view failureReason() const override { return NotReadyReason; }
SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
                     Ssl::HandshakerFactoryCb handshaker_factory_cb)
      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)),
      info_(std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
          ctx_->newSsl(transport_socket_options_), ctx_->sslExtendedSocketInfoIndex(), this))) {
    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Client,
                                       transport_socket_options, config_->createHandshaker());
    return std::make_unique<SslSocket>(std::move(ssl_ctx), InitialState::Server, nullptr,
                                       config_->createHandshaker());
  SslSocket(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
            Ssl::HandshakerFactoryCb handshaker_factory_cb);
Fixed code: - area: tls
  change: |
    Fix a RELEASE_ASSERT when using :ref:`auto_sni <envoy_v3_api_field_config.core.v3.UpstreamHttpProtocolOptions.auto_sni>`
    if the downstream request ``:authority`` was longer than 255 characters.

absl::StatusOr<bssl::UniquePtr<SSL>>
absl::StatusOr<bssl::UniquePtr<SSL>>
  absl::StatusOr<bssl::UniquePtr<SSL>> ssl_con_or_status(ContextImpl::newSsl(options));
  if (!ssl_con_or_status.ok()) {
    return ssl_con_or_status;
  }

  bssl::UniquePtr<SSL> ssl_con = std::move(ssl_con_or_status.value());
    if (rc != 1) {
      return absl::InvalidArgumentError(
          absl::StrCat("Failed to create upstream TLS due to failure setting SNI: ",
                       Utility::getLastCryptoError().value_or("unknown")));
    }
  virtual absl::StatusOr<bssl::UniquePtr<SSL>>
  newSsl(const Network::TransportSocketOptionsConstSharedPtr& options);
  absl::StatusOr<bssl::UniquePtr<SSL>>
class InvalidSslSocket : public Network::TransportSocket {
// This SslSocket will be used when SSL secret is not fetched from SDS server.
class NotReadySslSocket : public InvalidSslSocket {
public:
  // Network::TransportSocket
  absl::string_view failureReason() const override { return NotReadyReason; }
};

class ErrorSslSocket : public InvalidSslSocket {
public:
  ErrorSslSocket(absl::string_view error) : error_(error) {}

  // Network::TransportSocket
  absl::string_view failureReason() const override { return error_; }

private:
  std::string error_;
};

absl::StatusOr<std::unique_ptr<SslSocket>>
SslSocket::create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
                  const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
                  Ssl::HandshakerFactoryCb handshaker_factory_cb) {
  std::unique_ptr<SslSocket> socket(new SslSocket(ctx, transport_socket_options));
  auto status = socket->initialize(state, handshaker_factory_cb);
  if (status.ok()) {
    return socket;
  } else {
    return status;
  }
}

SslSocket::SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
                     const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options)
      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)) {}

absl::Status SslSocket::initialize(InitialState state,
                                   Ssl::HandshakerFactoryCb handshaker_factory_cb) {
  auto status_or_ssl = ctx_->newSsl(transport_socket_options_);
  if (!status_or_ssl.ok()) {
    return status_or_ssl.status();
  }

  info_ = std::dynamic_pointer_cast<SslHandshakerImpl>(handshaker_factory_cb(
      std::move(status_or_ssl.value()), ctx_->sslExtendedSocketInfoIndex(), this));


  return absl::OkStatus();
    auto status_or_socket =
        SslSocket::create(std::move(ssl_ctx), InitialState::Client, transport_socket_options,
                          config_->createHandshaker());
    if (status_or_socket.ok()) {
      return std::move(status_or_socket.value());
    }
    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
    auto status_or_socket = SslSocket::create(std::move(ssl_ctx), InitialState::Server, nullptr,
                                              config_->createHandshaker());
    if (status_or_socket.ok()) {
      return std::move(status_or_socket.value());
    }
    return std::make_unique<ErrorSslSocket>(status_or_socket.status().message());
  static absl::StatusOr<std::unique_ptr<SslSocket>>
  create(Envoy::Ssl::ContextSharedPtr ctx, InitialState state,
         const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options,
         Ssl::HandshakerFactoryCb handshaker_factory_cb);
  SslSocket(Envoy::Ssl::ContextSharedPtr ctx,
            const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options);
  absl::Status initialize(InitialState state, Ssl::HandshakerFactoryCb handshaker_factory_cb);

TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithTooLongSni) {
  upstream_tls_ = true;
  initializeWithArgs(1024, 1024, "x-host");
  std::string too_long_sni(300, 'a');
  ASSERT_EQ(too_long_sni.size(), 300); // Validate that the expected constructor was run.
  codec_client_ = makeHttpConnection(lookupPort("http"));
  const Http::TestRequestHeaderMapImpl request_headers{{":method", "POST"},
                                                       {":path", "/test/long/url"},
                                                       {":scheme", "http"},
                                                       {":authority", "localhost"},
                                                       {"x-host", too_long_sni}};

  auto response = codec_client_->makeHeaderOnlyRequest(request_headers);
  ASSERT_TRUE(response->waitForEndStream());
  EXPECT_EQ("503", response->headers().getStatusValue());
  // TODO(ggreenway): validate (in access logs probably) that failure reason is set appropriately.
}