Allow sockets to enable TLS renegotiation.

TESTED=unit + manually tested user issue.
Bug: #47841
Change-Id: Iad13899135fd34f15abba3a499132d88e7f597dc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/234821
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Brian Quinlan <[email protected]>

Author: brianquinlan

CHANGELOG.md

@@ -74,6 +74,10 @@
 - Deprecate `SecureSocket.renegotiate` and `RawSecureSocket.renegotiate`,
   which were no-ops.
 
+- **Breaking Change** [#48513](https://github.com/dart-lang/sdk/issues/48513):
+  Add a new `allowLegacyUnsafeRenegotiation` poperty to `SecurityContext`,
+  which allows TLS renegotiation for client secure sockets.
+
 #### `dart:isolate`
 
 - Add `Isolate.run` to run a function in a new isolate.

runtime/bin/io_natives.cc

@@ -137,6 +137,7 @@ namespace bin {
   V(SecurityContext_SetClientAuthoritiesBytes, 3)                              \
   V(SecurityContext_SetTrustedCertificatesBytes, 3)                            \
   V(SecurityContext_TrustBuiltinRoots, 1)                                      \
+  V(SecurityContext_SetAllowTlsRenegotiation, 2)                               \
   V(SecurityContext_UseCertificateChainBytes, 3)                               \
   V(ServerSocket_Accept, 2)                                                    \
   V(ServerSocket_CreateBindListen, 7)                                          \

runtime/bin/secure_socket_filter.cc

@@ -504,6 +504,10 @@ void SSLFilter::Connect(const char* hostname,
   SSL_set_bio(ssl_, ssl_side, ssl_side);
   SSL_set_mode(ssl_, SSL_MODE_AUTO_RETRY);  // TODO(whesse): Is this right?
   SSL_set_ex_data(ssl_, filter_ssl_index, this);
+
+  if (context->allow_tls_renegotiation()) {
+    SSL_set_renegotiate_mode(ssl_, ssl_renegotiate_freely);
+  }
   context->RegisterCallbacks(ssl_);
   SSL_set_ex_data(ssl_, ssl_cert_context_index, context);
 

runtime/bin/secure_socket_unsupported.cc

@@ -130,6 +130,12 @@ void FUNCTION_NAME(SecurityContext_TrustBuiltinRoots)(
       "Secure Sockets unsupported on this platform"));
 }
 
+void FUNCTION_NAME(SecurityContext_SetAllowTlsRenegotiation)(
+    Dart_NativeArguments args) {
+  Dart_ThrowException(DartUtils::NewDartArgumentError(
+      "Secure Sockets unsupported on this platform"));
+}
+
 void FUNCTION_NAME(SecurityContext_UseCertificateChainBytes)(
     Dart_NativeArguments args) {
   Dart_ThrowException(DartUtils::NewDartArgumentError(

runtime/bin/security_context.cc

@@ -877,6 +877,22 @@ void FUNCTION_NAME(SecurityContext_TrustBuiltinRoots)(
   context->TrustBuiltinRoots();
 }
 
+void FUNCTION_NAME(SecurityContext_SetAllowTlsRenegotiation)(
+    Dart_NativeArguments args) {
+  SSLCertContext* context = SSLCertContext::GetSecurityContext(args);
+  Dart_Handle allow_tls_handle = ThrowIfError(Dart_GetNativeArgument(args, 1));
+
+  ASSERT(context != NULL);
+  ASSERT(allow_tls_handle != NULL);
+
+  if (!Dart_IsBoolean(allow_tls_handle)) {
+    Dart_ThrowException(DartUtils::NewDartArgumentError(
+        "Non-boolean argument passed to SetAllowTlsRenegotiation"));
+  }
+  bool allow = DartUtils::GetBooleanValue(allow_tls_handle);
+  context->set_allow_tls_renegotiation(allow);
+}
+
 void FUNCTION_NAME(X509_Der)(Dart_NativeArguments args) {
   Dart_SetReturnValue(args, X509Helper::GetDer(args));
 }

runtime/bin/security_context.h

@@ -31,7 +31,8 @@ class SSLCertContext : public ReferenceCounted<SSLCertContext> {
       : ReferenceCounted(),
         context_(context),
         alpn_protocol_string_(nullptr),
-        trust_builtin_(false) {}
+        trust_builtin_(false),
+        allow_tls_renegotiation_(false) {}
 
   ~SSLCertContext() {
     SSL_CTX_free(context_);
@@ -82,6 +83,11 @@ class SSLCertContext : public ReferenceCounted<SSLCertContext> {
 
   bool trust_builtin() const { return trust_builtin_; }
 
+  void set_allow_tls_renegotiation(bool allow) {
+    allow_tls_renegotiation_ = allow;
+  }
+  bool allow_tls_renegotiation() const { return allow_tls_renegotiation_; }
+
   void set_trust_builtin(bool trust_builtin) { trust_builtin_ = trust_builtin; }
 
   void RegisterCallbacks(SSL* ssl);
@@ -112,7 +118,7 @@ class SSLCertContext : public ReferenceCounted<SSLCertContext> {
   uint8_t* alpn_protocol_string_;
 
   bool trust_builtin_;
-
+  bool allow_tls_renegotiation_;
   static bool long_ssl_cert_evaluation_;
   static bool bypass_trusting_system_roots_;
 

sdk/lib/_internal/vm/bin/secure_socket_patch.dart

@@ -209,13 +209,22 @@ class SecurityContext {
 
 class _SecurityContext extends NativeFieldWrapperClass1
     implements SecurityContext {
+  bool _allowLegacyUnsafeRenegotiation = false;
+
   _SecurityContext(bool withTrustedRoots) {
     _createNativeContext();
     if (withTrustedRoots) {
       _trustBuiltinRoots();
     }
   }
 
+  set allowLegacyUnsafeRenegotiation(bool allow) {
+    _allowLegacyUnsafeRenegotiation = allow;
+    _setAllowTlsRenegotiation(allow);
+  }
+
+  bool get allowLegacyUnsafeRenegotiation => _allowLegacyUnsafeRenegotiation;
+
   @pragma("vm:external-name", "SecurityContext_Allocate")
   external void _createNativeContext();
 
@@ -266,6 +275,8 @@ class _SecurityContext extends NativeFieldWrapperClass1
   external void _setAlpnProtocols(Uint8List protocols, bool isServer);
   @pragma("vm:external-name", "SecurityContext_TrustBuiltinRoots")
   external void _trustBuiltinRoots();
+  @pragma("vm:external-name", "SecurityContext_SetAllowTlsRenegotiation")
+  external void _setAllowTlsRenegotiation(bool allow);
 }
 
 /**

sdk/lib/io/security_context.dart

@@ -160,6 +160,16 @@ abstract class SecurityContext {
   /// or client connections.
   void setAlpnProtocols(List<String> protocols, bool isServer);
 
+  /// If `true`, the [SecurityContext] will allow TLS renegotiation.
+  /// Renegotiation is only supported as a client and the HelloRequest must be
+  /// received at a quiet point in the application protocol. This is sufficient
+  /// to support the legacy use case of requesting a new client certificate
+  /// between an HTTP request and response in (unpipelined) HTTP/1.1.
+  /// NOTE: Renegotiation is an extremely problematic protocol feature and
+  /// should only be used to communicate with legacy servers in environments
+  /// where it is known to be safe.
+  abstract bool allowLegacyUnsafeRenegotiation;
+
   /// Encodes a set of supported protocols for ALPN/NPN usage.
   ///
   /// The [protocols] list is expected to contain protocols in descending order

tests/standalone/io/secure_socket_allow_renegotiation_test.dart

@@ -0,0 +1,81 @@
+// Copyright (c) 2022, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// VMOptions=
+// VMOptions=--short_socket_read
+// VMOptions=--short_socket_write
+// VMOptions=--short_socket_read --short_socket_write
+// OtherResources=certificates/server_chain.pem
+// OtherResources=certificates/server_key.pem
+// OtherResources=certificates/trusted_certs.pem
+//
+// It is not possible to initiate TLS-renegotiation from a pure-Dart server so
+// just test that the `allowLegacyUnsafeRenegotiation` in `SecurityContext`
+// does not affect connections that do *not* do renegotiation.
+
+import "dart:async";
+import 'dart:convert';
+import "dart:io";
+
+import "package:async_helper/async_helper.dart";
+import "package:expect/expect.dart";
+
+late InternetAddress HOST;
+
+String localFile(path) => Platform.script.resolve(path).toFilePath();
+
+SecurityContext serverContext = new SecurityContext()
+  ..useCertificateChain(localFile('certificates/server_chain.pem'))
+  ..usePrivateKey(localFile('certificates/server_key.pem'),
+      password: 'dartdart');
+
+Future<SecureServerSocket> startEchoServer() {
+  return SecureServerSocket.bind(HOST, 0, serverContext).then((server) {
+    server.listen((SecureSocket client) {
+      client.fold<List<int>>(
+          <int>[], (message, data) => message..addAll(data)).then((message) {
+        client.add(message);
+        client.close();
+      });
+    });
+    return server;
+  });
+}
+
+testSuccess(SecureServerSocket server) async {
+  // NOTE: this test only verifies that `allowLegacyUnsafeRenegotiation` does
+  // not cause incorrect behavior when enabled - the server does *not* actually
+  // trigger TLS renegotiation.
+  SecurityContext clientContext = new SecurityContext()
+    ..allowLegacyUnsafeRenegotiation = true
+    ..setTrustedCertificates(localFile('certificates/trusted_certs.pem'));
+
+  await SecureSocket.connect(HOST, server.port, context: clientContext)
+      .then((socket) async {
+    socket.write("Hello server.");
+    socket.close();
+    Expect.isTrue(await utf8.decoder.bind(socket).contains("Hello server."));
+  });
+}
+
+testProperty() {
+  SecurityContext context = new SecurityContext();
+  Expect.isFalse(context.allowLegacyUnsafeRenegotiation);
+  context.allowLegacyUnsafeRenegotiation = true;
+  Expect.isTrue(context.allowLegacyUnsafeRenegotiation);
+  context.allowLegacyUnsafeRenegotiation = false;
+  Expect.isFalse(context.allowLegacyUnsafeRenegotiation);
+}
+
+void main() async {
+  asyncStart();
+  await InternetAddress.lookup("localhost").then((hosts) => HOST = hosts.first);
+  final server = await startEchoServer();
+
+  await testSuccess(server);
+  testProperty();
+
+  await server.close();
+  asyncEnd();
+}

tests/standalone_2/io/secure_socket_allow_renegotiation_test.dart

@@ -0,0 +1,83 @@
+// Copyright (c) 2022, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// VMOptions=
+// VMOptions=--short_socket_read
+// VMOptions=--short_socket_write
+// VMOptions=--short_socket_read --short_socket_write
+// OtherResources=certificates/server_chain.pem
+// OtherResources=certificates/server_key.pem
+// OtherResources=certificates/trusted_certs.pem
+//
+// It is not possible to initiate TLS-renegotiation from a pure-Dart server so
+// just test that the `allowLegacyUnsafeRenegotiation` in `SecurityContext`
+// does not affect connections that do *not* do renegotiation.
+
+// @dart = 2.9
+
+import "dart:async";
+import 'dart:convert';
+import "dart:io";
+
+import "package:async_helper/async_helper.dart";
+import "package:expect/expect.dart";
+
+InternetAddress HOST;
+
+String localFile(path) => Platform.script.resolve(path).toFilePath();
+
+SecurityContext serverContext = new SecurityContext()
+  ..useCertificateChain(localFile('certificates/server_chain.pem'))
+  ..usePrivateKey(localFile('certificates/server_key.pem'),
+      password: 'dartdart');
+
+Future<SecureServerSocket> startEchoServer() {
+  return SecureServerSocket.bind(HOST, 0, serverContext).then((server) {
+    server.listen((SecureSocket client) {
+      client.fold<List<int>>(
+          <int>[], (message, data) => message..addAll(data)).then((message) {
+        client.add(message);
+        client.close();
+      });
+    });
+    return server;
+  });
+}
+
+testSuccess(SecureServerSocket server) async {
+  // NOTE: this test only verifies that `allowLegacyUnsafeRenegotiation` does
+  // not cause incorrect behavior when enabled - the server does *not* actually
+  // trigger TLS renegotiation.
+  SecurityContext clientContext = new SecurityContext()
+    ..allowLegacyUnsafeRenegotiation = true
+    ..setTrustedCertificates(localFile('certificates/trusted_certs.pem'));
+
+  await SecureSocket.connect(HOST, server.port, context: clientContext)
+      .then((socket) async {
+    socket.write("Hello server.");
+    socket.close();
+    Expect.isTrue(await utf8.decoder.bind(socket).contains("Hello server."));
+  });
+}
+
+testProperty() {
+  SecurityContext context = new SecurityContext();
+  Expect.isFalse(context.allowLegacyUnsafeRenegotiation);
+  context.allowLegacyUnsafeRenegotiation = true;
+  Expect.isTrue(context.allowLegacyUnsafeRenegotiation);
+  context.allowLegacyUnsafeRenegotiation = false;
+  Expect.isFalse(context.allowLegacyUnsafeRenegotiation);
+}
+
+void main() async {
+  asyncStart();
+  await InternetAddress.lookup("localhost").then((hosts) => HOST = hosts.first);
+  final server = await startEchoServer();
+
+  await testSuccess(server);
+  testProperty();
+
+  await server.close();
+  asyncEnd();
+}