From c286b76c2db394b72bd8ae79b32d024c2d25c52b Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Thu, 31 Mar 2022 06:40:51 +0000 Subject: [PATCH] Allow sockets to enable TLS renegotiation. TESTED=unit + manually tested user issue. Bug: https://github.com/dart-lang/sdk/issues/47841 Change-Id: Iad13899135fd34f15abba3a499132d88e7f597dc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/234821 Reviewed-by: Alexander Aprelev Commit-Queue: Brian Quinlan --- CHANGELOG.md | 4 + runtime/bin/io_natives.cc | 1 + runtime/bin/secure_socket_filter.cc | 4 + runtime/bin/secure_socket_unsupported.cc | 6 ++ runtime/bin/security_context.cc | 16 ++++ runtime/bin/security_context.h | 10 ++- .../_internal/vm/bin/secure_socket_patch.dart | 11 +++ sdk/lib/io/security_context.dart | 10 +++ ...ecure_socket_allow_renegotiation_test.dart | 81 ++++++++++++++++++ ...ecure_socket_allow_renegotiation_test.dart | 83 +++++++++++++++++++ 10 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 tests/standalone/io/secure_socket_allow_renegotiation_test.dart create mode 100644 tests/standalone_2/io/secure_socket_allow_renegotiation_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e7b432736d..cb298a32059 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/runtime/bin/io_natives.cc b/runtime/bin/io_natives.cc index 6eddb5ab424..4d2dad3a948 100644 --- a/runtime/bin/io_natives.cc +++ b/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) \ diff --git a/runtime/bin/secure_socket_filter.cc b/runtime/bin/secure_socket_filter.cc index 9e1492e4ed8..a78b635b995 100644 --- a/runtime/bin/secure_socket_filter.cc +++ b/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); diff --git a/runtime/bin/secure_socket_unsupported.cc b/runtime/bin/secure_socket_unsupported.cc index f64203d573a..5d5241db681 100644 --- a/runtime/bin/secure_socket_unsupported.cc +++ b/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( diff --git a/runtime/bin/security_context.cc b/runtime/bin/security_context.cc index 8ccc6a3d26b..b59212e3531 100644 --- a/runtime/bin/security_context.cc +++ b/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)); } diff --git a/runtime/bin/security_context.h b/runtime/bin/security_context.h index b5bc66b37fe..a9eec35d452 100644 --- a/runtime/bin/security_context.h +++ b/runtime/bin/security_context.h @@ -31,7 +31,8 @@ class SSLCertContext : public ReferenceCounted { : 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 { 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 { uint8_t* alpn_protocol_string_; bool trust_builtin_; - + bool allow_tls_renegotiation_; static bool long_ssl_cert_evaluation_; static bool bypass_trusting_system_roots_; diff --git a/sdk/lib/_internal/vm/bin/secure_socket_patch.dart b/sdk/lib/_internal/vm/bin/secure_socket_patch.dart index 680adcf3066..bfdc6bdf951 100644 --- a/sdk/lib/_internal/vm/bin/secure_socket_patch.dart +++ b/sdk/lib/_internal/vm/bin/secure_socket_patch.dart @@ -209,6 +209,8 @@ class SecurityContext { class _SecurityContext extends NativeFieldWrapperClass1 implements SecurityContext { + bool _allowLegacyUnsafeRenegotiation = false; + _SecurityContext(bool withTrustedRoots) { _createNativeContext(); if (withTrustedRoots) { @@ -216,6 +218,13 @@ class _SecurityContext extends NativeFieldWrapperClass1 } } + 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); } /** diff --git a/sdk/lib/io/security_context.dart b/sdk/lib/io/security_context.dart index d041975fed1..8e9839e7716 100644 --- a/sdk/lib/io/security_context.dart +++ b/sdk/lib/io/security_context.dart @@ -160,6 +160,16 @@ abstract class SecurityContext { /// or client connections. void setAlpnProtocols(List 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 diff --git a/tests/standalone/io/secure_socket_allow_renegotiation_test.dart b/tests/standalone/io/secure_socket_allow_renegotiation_test.dart new file mode 100644 index 00000000000..f2f5716b531 --- /dev/null +++ b/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 startEchoServer() { + return SecureServerSocket.bind(HOST, 0, serverContext).then((server) { + server.listen((SecureSocket client) { + client.fold>( + [], (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(); +} diff --git a/tests/standalone_2/io/secure_socket_allow_renegotiation_test.dart b/tests/standalone_2/io/secure_socket_allow_renegotiation_test.dart new file mode 100644 index 00000000000..16790e1bcbe --- /dev/null +++ b/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 startEchoServer() { + return SecureServerSocket.bind(HOST, 0, serverContext).then((server) { + server.listen((SecureSocket client) { + client.fold>( + [], (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(); +}