Reland of "Add Finch param to control TLS 1.3 Downgrade enforcement on known/unknown roots."

Using an explicit string in about_flags to avoid static initializer.

Bug: boringssl:226
Change-Id: I095c5060e903eb610576705acd760a535e55325e
Reviewed-on: https://chromium-review.googlesource.com/c/1335872
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608092}

chrome/browser/about_flags.cc

@@ -789,6 +789,13 @@ const FeatureEntry::Choice kTLS13VariantChoices[] = {
      switches::kTLS13VariantFinal},
 };
 
+const FeatureEntry::FeatureParam kEnforceTLS13DowngradeKnownOnly[] = {
+    {"known_roots_only", "true"}};
+
+const FeatureEntry::FeatureVariation kEnforceTLS13DowngradeFeatureVariations[] =
+    {{"(Known Root Only)", kEnforceTLS13DowngradeKnownOnly,
+      base::size(kEnforceTLS13DowngradeKnownOnly), nullptr}};
+
 #if !defined(OS_ANDROID)
 const FeatureEntry::Choice kEnableAudioFocusChoices[] = {
     {flag_descriptions::kEnableAudioFocusDisabled, "", ""},
@@ -2605,7 +2612,9 @@ const FeatureEntry kFeatureEntries[] = {
      MULTI_VALUE_TYPE(kTLS13VariantChoices)},
     {"enforce-tls13-downgrade", flag_descriptions::kEnforceTLS13DowngradeName,
      flag_descriptions::kEnforceTLS13DowngradeDescription, kOsAll,
-     FEATURE_VALUE_TYPE(net::features::kEnforceTLS13Downgrade)},
+     FEATURE_WITH_PARAMS_VALUE_TYPE(net::features::kEnforceTLS13Downgrade,
+                                    kEnforceTLS13DowngradeFeatureVariations,
+                                    "EnforceTLS13Downgrade")},
     {"enable-scroll-anchor-serialization",
      flag_descriptions::kEnableScrollAnchorSerializationName,
      flag_descriptions::kEnableScrollAnchorSerializationDescription, kOsAll,

net/socket/ssl_client_socket_impl.cc

@@ -845,7 +845,9 @@ int SSLClientSocketImpl::Init() {
       break;
   }
 
-  if (!base::FeatureList::IsEnabled(features::kEnforceTLS13Downgrade)) {
+  if (!base::FeatureList::IsEnabled(features::kEnforceTLS13Downgrade) ||
+      base::GetFieldTrialParamByFeatureAsBool(features::kEnforceTLS13Downgrade,
+                                              "known_roots_only", false)) {
     SSL_set_ignore_tls13_downgrade(ssl_.get(), 1);
   }
 
@@ -1141,17 +1143,21 @@ int SSLClientSocketImpl::DoVerifyCertComplete(int result) {
       }
     }
 
-    if (!base::FeatureList::IsEnabled(features::kEnforceTLS13Downgrade)) {
+    bool enforce_tls13_downgrade_known_roots_only =
+        base::GetFieldTrialParamByFeatureAsBool(
+            features::kEnforceTLS13Downgrade, "known_roots_only", false);
+    if (!base::FeatureList::IsEnabled(features::kEnforceTLS13Downgrade) ||
+        enforce_tls13_downgrade_known_roots_only) {
       // Record metrics on the TLS 1.3 anti-downgrade mechanism. This is only
       // recorded when enforcement is disabled. (When enforcement is enabled,
       // the connection will fail with ERR_TLS13_DOWNGRADE_DETECTED.) See
       // https://crbug.com/boringssl/226.
       //
       // Record metrics for both servers overall and the TLS 1.3 experiment
-      // set. These metrics are only useful on TLS 1.3 servers, so the latter is
-      // more precise, but there is a large enough TLS 1.3 deployment that the
-      // overall numbers may be more robust. In particular, the DowngradeType
-      // metrics do not need to be filtered.
+      // set. These metrics are only useful on TLS 1.3 servers, so the latter
+      // is more precise, but there is a large enough TLS 1.3 deployment that
+      // the overall numbers may be more robust. In particular, the
+      // DowngradeType metrics do not need to be filtered.
       bool is_downgrade = !!SSL_is_tls13_downgrade(ssl_.get());
       UMA_HISTOGRAM_BOOLEAN("Net.SSLTLS13Downgrade", is_downgrade);
       bool is_tls13_experiment_host =
@@ -1165,7 +1171,8 @@ int SSLClientSocketImpl::DoVerifyCertComplete(int result) {
         // Record whether connections which hit the downgrade used known vs
         // unknown roots and which key exchange type.
 
-        // This enum is persisted into histograms. Values may not be renumbered.
+        // This enum is persisted into histograms. Values may not be
+        // renumbered.
         enum class DowngradeType {
           kKnownRootRSA = 0,
           kKnownRootECDHE = 1,
@@ -1190,6 +1197,14 @@ int SSLClientSocketImpl::DoVerifyCertComplete(int result) {
                                     type);
         }
       }
+
+      if (enforce_tls13_downgrade_known_roots_only &&
+          server_cert_verify_result_.is_issued_by_known_root) {
+        // Exit DoHandshakeLoop and return the result to the caller to
+        // Connect.
+        DCHECK_EQ(STATE_NONE, next_handshake_state_);
+        return ERR_TLS13_DOWNGRADE_DETECTED;
+      }
     }
   }
 

net/socket/ssl_client_socket_unittest.cc

@@ -4846,6 +4846,64 @@ TEST_F(SSLClientSocketTest, TLS13DowngradeIgnoredAtTLS10) {
             SSLConnectionStatusToVersion(info.connection_status));
 }
 
+// Test downgrade enforcement enforces known roots when configured to.
+TEST_F(SSLClientSocketTest, TLS13DowngradeEnforcedKnownKnown) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      features::kEnforceTLS13Downgrade, {{"known_roots_only", "true"}});
+
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.simulate_tls13_downgrade = true;
+  ssl_options.tls_max_version =
+      SpawnedTestServer::SSLOptions::TLS_MAX_VERSION_TLS1_2;
+  ASSERT_TRUE(StartTestServer(ssl_options));
+
+  scoped_refptr<X509Certificate> server_cert =
+      spawned_test_server()->GetCertificate();
+
+  // Certificate is trusted and chains to a public root.
+  CertVerifyResult verify_result;
+  verify_result.is_issued_by_known_root = true;
+  verify_result.verified_cert = server_cert;
+  cert_verifier_->AddResultForCert(server_cert.get(), verify_result, OK);
+
+  SSLConfig config;
+  config.version_max = SSL_PROTOCOL_VERSION_TLS1_3;
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(config, &rv));
+  EXPECT_THAT(rv, IsError(ERR_TLS13_DOWNGRADE_DETECTED));
+  EXPECT_FALSE(sock_->IsConnected());
+}
+
+// Test downgrade enforcement ignores unknown roots when configured to.
+TEST_F(SSLClientSocketTest, TLS13DowngradeEnforcedKnownUnknown) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      features::kEnforceTLS13Downgrade, {{"known_roots_only", "true"}});
+
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.simulate_tls13_downgrade = true;
+  ssl_options.tls_max_version =
+      SpawnedTestServer::SSLOptions::TLS_MAX_VERSION_TLS1_2;
+  ASSERT_TRUE(StartTestServer(ssl_options));
+
+  scoped_refptr<X509Certificate> server_cert =
+      spawned_test_server()->GetCertificate();
+
+  // Certificate is trusted and chains to a public root.
+  CertVerifyResult verify_result;
+  verify_result.is_issued_by_known_root = false;
+  verify_result.verified_cert = server_cert;
+  cert_verifier_->AddResultForCert(server_cert.get(), verify_result, OK);
+
+  SSLConfig config;
+  config.version_max = SSL_PROTOCOL_VERSION_TLS1_3;
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(config, &rv));
+  EXPECT_THAT(rv, IsOk());
+  EXPECT_TRUE(sock_->IsConnected());
+}
+
 struct TLS13DowngradeMetricsParams {
   bool downgrade;
   bool known_root;