Restore the SSL_OP_LEGACY_SERVER_CONNECT option Things have improved significantly since https://boringssl-review.googlesource.com/6580 and this may be within reach now. Restore the toggle, but keep it enabled by default for now. Bug: 41393419 Change-Id: If92dc050561c0038d8c829dc866ffc2189c13afd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95947 Presubmit-BoringSSL-Verified: boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Lily Chen <chlily@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Lily Chen <chlily@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 055b3e0..e69b22c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -598,6 +598,12 @@ // // Options configure protocol behavior. +// SSL_OP_LEGACY_SERVER_CONNECT configures a client to permit connecting to +// legacy servers that do not implement renegotiation_info (RFC 5746). If +// disabled, connections to such servers will fail. This option is enabled by +// default, but may be disabled with |SSL_CTX_clear_options|. +#define SSL_OP_LEGACY_SERVER_CONNECT 0x00000004L + // SSL_OP_NO_QUERY_MTU, in DTLS, disables querying the MTU from the underlying // |BIO|. Instead, the MTU is configured with |SSL_set_mtu|. #define SSL_OP_NO_QUERY_MTU 0x00001000L @@ -5882,7 +5888,6 @@ #define SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION 0 #define SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS 0 #define SSL_OP_EPHEMERAL_RSA 0 -#define SSL_OP_LEGACY_SERVER_CONNECT 0 #define SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER 0 #define SSL_OP_MICROSOFT_SESS_ID_BUG 0 #define SSL_OP_MSIE_SSLV2_RSA_PADDING 0
diff --git a/ssl/extensions.cc b/ssl/extensions.cc index db1a291..3ce85b3 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc
@@ -744,10 +744,13 @@ // Strictly speaking, if we want to avoid an attack we should *always* see // RI even on initial ServerHello because the client doesn't see any // renegotiation during an attack. However this would mean we could not - // connect to any server which doesn't support RI. - // - // OpenSSL has |SSL_OP_LEGACY_SERVER_CONNECT| to control this, but in - // practical terms every client sets it so it's just assumed here. + // connect to any server which doesn't support RI. See RFC 5746, + // section 4.1. + if (!(ssl->options & SSL_OP_LEGACY_SERVER_CONNECT)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED); + *out_alert = SSL_AD_HANDSHAKE_FAILURE; + return false; + } return true; } @@ -808,6 +811,7 @@ } if (contents == nullptr) { + // Permit clients to omit the extension. We never renegotiate as a server. return true; }
diff --git a/ssl/internal.h b/ssl/internal.h index 2bea8f6..e360f0f 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -3989,7 +3989,8 @@ // Default values to use in SSL structures follow (these are copied by // SSL_new) - uint32_t options = 0; + // TODO(crbug.com/41393419): Disable SSL_OP_LEGACY_SERVER_CONNECT by default. + uint32_t options = SSL_OP_LEGACY_SERVER_CONNECT; // Disable the auto-chaining feature by default. wpa_supplicant relies on this // feature, but require callers opt into it. uint32_t mode = SSL_MODE_NO_AUTO_CHAIN;
diff --git a/ssl/test/runner/renegotiation_tests.go b/ssl/test/runner/renegotiation_tests.go index a0fd152..f67bf4d 100644 --- a/ssl/test/runner/renegotiation_tests.go +++ b/ssl/test/runner/renegotiation_tests.go
@@ -177,6 +177,18 @@ "-expect-no-secure-renegotiation", }, }) + testCases = append(testCases, testCase{ + name: "Renegotiate-Client-NoExt-Forbidden", + config: Config{ + MaxVersion: VersionTLS12, + Bugs: ProtocolBugs{ + NoRenegotiationInfo: true, + }, + }, + flags: []string{"-no-legacy-server-connect"}, + shouldFail: true, + expectedError: ":UNSAFE_LEGACY_RENEGOTIATION_DISABLED:", + }) // Test that the server may switch ciphers on renegotiation without // problems.
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 4333d91..4078b28 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -386,6 +386,8 @@ BoolFlag("-no-tls11", &TestConfig::no_tls11), BoolFlag("-no-tls1", &TestConfig::no_tls1), BoolFlag("-no-ticket", &TestConfig::no_ticket), + BoolFlag("-no-legacy-server-connect", + &TestConfig::no_legacy_server_connect), Base64Flag("-expect-channel-id", &TestConfig::expect_channel_id), BoolFlag("-enable-channel-id", &TestConfig::enable_channel_id), StringFlag("-send-channel-id", &TestConfig::send_channel_id), @@ -2444,6 +2446,9 @@ if (no_ticket) { SSL_set_options(ssl.get(), SSL_OP_NO_TICKET); } + if (no_legacy_server_connect) { + SSL_clear_options(ssl.get(), SSL_OP_LEGACY_SERVER_CONNECT); + } if (!expect_channel_id.empty() || enable_channel_id) { SSL_set_tls_channel_id_enabled(ssl.get(), 1); }
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 8c81309..cd526f0 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -106,6 +106,7 @@ bool no_tls11 = false; bool no_tls1 = false; bool no_ticket = false; + bool no_legacy_server_connect = false; std::vector<uint8_t> expect_channel_id; bool enable_channel_id = false; std::string send_channel_id;