ssl: fix crash when peer sends an SSL Alert with an unknown code (env…

…oyproxy#239) Fix for CVE-2021-28683 (crash when peer sends an SSL Alert with an unknown code) Signed-off-by: Greg Greenway <[email protected]> Co-authored-by: Christoph Pakulski <[email protected]> Signed-off-by: Tony Allen <[email protected]> Signed-off-by: Gokul Nair <[email protected]>
gokulnair · May 6, 2021 · 27aa291 · 27aa291
1 parent 63b54eb
commit 27aa291
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 6 deletions.
diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -176,9 +176,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c
       if (ctx.cert_chain_ == nullptr ||
           !SSL_CTX_use_certificate(ctx.ssl_ctx_.get(), ctx.cert_chain_.get())) {
         while (uint64_t err = ERR_get_error()) {
-          ENVOY_LOG_MISC(debug, "SSL error: {}:{}:{}:{}", err, ERR_lib_error_string(err),
-                         ERR_func_error_string(err), ERR_GET_REASON(err),
-                         ERR_reason_error_string(err));
+          ENVOY_LOG_MISC(debug, "SSL error: {}:{}:{}:{}", err,
+                         absl::NullSafeStringView(ERR_lib_error_string(err)),
+                         absl::NullSafeStringView(ERR_func_error_string(err)), ERR_GET_REASON(err),
+                         absl::NullSafeStringView(ERR_reason_error_string(err)));
         }
         throw EnvoyException(
             absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_));

diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc
@@ -210,9 +210,10 @@ void SslSocket::drainErrorQueue() {
     if (failure_reason_.empty()) {
       failure_reason_ = "TLS error:";
     }
-    failure_reason_.append(absl::StrCat(" ", err, ":", ERR_lib_error_string(err), ":",
-                                        ERR_func_error_string(err), ":",
-                                        ERR_reason_error_string(err)));
+    failure_reason_.append(absl::StrCat(" ", err, ":",
+                                        absl::NullSafeStringView(ERR_lib_error_string(err)), ":",
+                                        absl::NullSafeStringView(ERR_func_error_string(err)), ":",
+                                        absl::NullSafeStringView(ERR_reason_error_string(err))));
   }
   if (!failure_reason_.empty()) {
     ENVOY_CONN_LOG(debug, "{}", callbacks_->connection(), failure_reason_);

diff --git a/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc b/test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc
@@ -92,6 +92,36 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, SslIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);
 
+// Test that Envoy behaves correctly when receiving an SSLAlert for an unspecified code. The codes
+// are defined in the standard, and assigned codes have a string associated with them in BoringSSL,
+// which is included in logs. For an unknown code, verify that no crash occurs.
+TEST_P(SslIntegrationTest, UnknownSslAlert) {
+  initialize();
+  Network::ClientConnectionPtr connection = makeSslClientConnection({});
+  ConnectionStatusCallbacks callbacks;
+  connection->addConnectionCallbacks(callbacks);
+  connection->connect();
+  while (!callbacks.connected()) {
+    dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+  }
+
+  Ssl::ConnectionInfoConstSharedPtr ssl_info = connection->ssl();
+  SSL* ssl =
+      dynamic_cast<const Extensions::TransportSockets::Tls::SslHandshakerImpl*>(ssl_info.get())
+          ->ssl();
+  ASSERT_EQ(connection->state(), Network::Connection::State::Open);
+  ASSERT_NE(ssl, nullptr);
+  SSL_send_fatal_alert(ssl, 255);
+  while (!callbacks.closed()) {
+    dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+  }
+
+  const std::string counter_name = listenerStatPrefix("ssl.connection_error");
+  Stats::CounterSharedPtr counter = test_server_->counter(counter_name);
+  test_server_->waitForCounterGe(counter_name, 1);
+  connection->close(Network::ConnectionCloseType::NoFlush);
+}
+
 TEST_P(SslIntegrationTest, RouterRequestAndResponseWithGiantBodyBuffer) {
   ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr {
     return makeSslClientConnection({});