Expose TLS settings in the Security panel overview, and call out individual obsolete settings.

chrome/browser/ui/browser.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram.h"
 #include "base/process/process_info.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ skipping 169 matching lines @@
 #include "content/public/common/webplugininfo.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handlers/background_info.h"
 #include "net/base/filename_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/cookie_monster.h"
+#include "net/ssl/ssl_cipher_suite_names.h"
+#include "net/ssl/ssl_connection_status_flags.h"
 #include "net/url_request/url_request_context.h"
 #include "third_party/WebKit/public/web/WebWindowFeatures.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/window_open_disposition.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/shell_dialogs/selected_file_info.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
@@ skipping 1084 matching lines @@
                            blink::WebDragOperationsMask operations_allowed) {
   // Disallow drag-and-drop navigation for Settings windows which do not support
   // external navigation.
   if ((operations_allowed & blink::WebDragOperationLink) &&
       chrome::SettingsWindowManager::GetInstance()->IsSettingsBrowser(this)) {
     return false;
   }
   return true;
 }
 
 content::SecurityStyle Browser::GetSecurityStyle(

[estark, 2016/04/18 11:46:44]

This function is getting pretty huge. We should probably move it in to
chrome/browser/ssl, either as a utility function in its own file, or maybe as a
method on ChromeSecurityStateModelClient. That can be a separate CL though.
Filed bug 604339.

     WebContents* web_contents,
     content::SecurityStyleExplanations* security_style_explanations) {
   ChromeSecurityStateModelClient* model_client =
       ChromeSecurityStateModelClient::FromWebContents(web_contents);
   DCHECK(model_client);
   const SecurityStateModel::SecurityInfo& security_info =
       model_client->GetSecurityInfo();
 
   const content::SecurityStyle security_style =
       SecurityLevelToSecurityStyle(security_info.security_level);
@@ skipping 68 matching lines @@
         SecurityStateModel::NO_DEPRECATED_SHA1) {
       security_style_explanations->secure_explanations.push_back(
           content::SecurityStyleExplanation(
               l10n_util::GetStringUTF8(IDS_VALID_SERVER_CERTIFICATE),
               l10n_util::GetStringUTF8(
                   IDS_VALID_SERVER_CERTIFICATE_DESCRIPTION),
               security_info.cert_id));
     }
   }
 
-  if (security_info.is_secure_protocol_and_ciphersuite) {
+  int ssl_version =
+      net::SSLConnectionStatusToVersion(security_info.connection_status);
+  const char* protocol;
+  net::SSLVersionToString(&protocol, ssl_version);
+
+  const char* key_exchange;
+  const char* cipher;
+  const char* mac;
+  bool is_aead;
+  uint16_t cipher_suite =
+      net::SSLConnectionStatusToCipherSuite(security_info.connection_status);
+  net::SSLCipherSuiteToStrings(&key_exchange, &cipher, &mac, &is_aead,
+                               cipher_suite);
+
+  base::string16 protocol_name = base::ASCIIToUTF16(protocol);
+  base::string16 key_exchange_name = base::ASCIIToUTF16(key_exchange);
+  const base::string16 cipher_suite_name =

[estark, 2016/04/18 11:46:44]

Why do these all need to be string16s?

[lgarron, 2016/04/25 23:56:53]

GetStringFUTF8() as well as GetStringFUTF16() take `string16`s. In particular, I
have so many arguments that I have to pass a vector of replacements rather than
positional arguments, and we only have the GetStringFUTF16() variant of the
function that takes a vector [1] Ergo, I have to construct a
std::vector<base::string16>.
I didn't feel that this warranted a comment; do you think it would worth adding
one?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_...

+      (mac == NULL) ? base::ASCIIToUTF16(cipher)
+                    : l10n_util::GetStringFUTF16(IDS_CIPHERSUITE_WITH_MAC,
+                                                 base::ASCIIToUTF16(cipher),
+                                                 base::ASCIIToUTF16(mac));
+
+  if (security_info.obsolete_ssl_status == net::OBSOLETE_SSL_NONE) {
     security_style_explanations->secure_explanations.push_back(
         content::SecurityStyleExplanation(
             l10n_util::GetStringUTF8(IDS_SECURE_PROTOCOL_AND_CIPHERSUITE),
-            l10n_util::GetStringUTF8(
-                IDS_SECURE_PROTOCOL_AND_CIPHERSUITE_DESCRIPTION)));
+            l10n_util::GetStringFUTF8(
+                IDS_SECURE_PROTOCOL_AND_CIPHERSUITE_DESCRIPTION, protocol_name,
+                key_exchange_name, cipher_suite_name)));
+  } else if (security_info.cert_id != 0) {

[estark, 2016/04/18 11:46:44]

Can you please explain this check in a comment? It doesn't look like we check
for a 0 cert_id elsewhere, so I'm wondering why we need to here.

[lgarron, 2016/04/25 23:56:53]

The reason is net errors. If we don't include this check, visiting
dh480.badssl.com will result in:
"The connection to this site uses an obsolete protocol (???), an obsolete key
exchange (NULL), and an obsolete cipher suite (NULL with NULL)."

I've added a comment:

+    std::vector<base::string16> summary_replacements;
+    std::vector<base::string16> description_replacements;
+

[estark, 2016/04/18 11:46:44]

Hmmmm, reading this constructing-the-string code hurts my brain. It would
probably help if you separated it out into a helper function. Other optional
suggestions for possible simplifications:

1.) Are you sure we need the summary string to be so descriptive? Could it just
be "Obsolete TLS settings" or "Secure TLS connection", rather than actually
specifying the obsolete settings in the summary?

2.) Maybe factor out yet another helper function for the repetitive code in
lines 1442-1451, 1453-1463, and 1465-1474. Or, this totally might be personal
preference, but I think I might find this easier to read if you defined more
strings, and had OBSOLETE_PROTOCOL, MODERN_PROTOCOL, OBSOLETE_KEY_EXCHANGE,
MODERN_KEY_EXCHANGE, OBSOLETE_CIPHER, MODERN_CIPHER, instead of combining
obsolete/modern with protocol/key exchange/cipher for each one.

[lgarron, 2016/04/25 23:56:53]

I've simplified things considerably, taking your string change suggestions from
1. and 2.
I could still factor it out, but I think it's not bad right now.

+    if (security_info.obsolete_ssl_status & net::OBSOLETE_SSL_MASK_PROTOCOL) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_PROTOCOL));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(protocol_name);
+
+    if (security_info.obsolete_ssl_status &
+        net::OBSOLETE_SSL_MASK_KEY_EXCHANGE) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_KEY_EXCHANGE));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(key_exchange_name);
+
+    if (security_info.obsolete_ssl_status & net::OBSOLETE_SSL_MASK_CIPHER) {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_AN_OBSOLETE));
+      summary_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_CIPHER_SUITE));
+    } else {
+      description_replacements.push_back(
+          l10n_util::GetStringUTF16(IDS_SSL_A_MODERN));
+    }
+    description_replacements.push_back(cipher_suite_name);
+
+    int summary_message_id;
+    switch (summary_replacements.size()) {
+      case 1:
+        summary_message_id = IDS_SINGLE_OBSOLETE_TLS_SETTING;
+        break;
+      case 2:
+        summary_message_id = IDS_TWO_OBSOLETE_TLS_SETTINGS;
+        break;
+      case 3:
+        summary_message_id = IDS_THREE_OBSOLETE_TLS_SETTINGS;
+        break;
+      default:
+        summary_message_id = 0;
+        NOTREACHED();
+    }
+
+    security_style_explanations->info_explanations.push_back(
+        content::SecurityStyleExplanation(
+            base::UTF16ToUTF8(l10n_util::GetStringFUTF16(
+                summary_message_id, summary_replacements, nullptr)),
+            base::UTF16ToUTF8(l10n_util::GetStringFUTF16(
+                IDS_OBSOLETE_SSL_DESCRIPTION, description_replacements,
+                nullptr))));
   }
 
   return security_style;
 }
 
 void Browser::ShowCertificateViewerInDevTools(
     content::WebContents* web_contents, int cert_id) {
   DevToolsWindow* devtools_window =
       DevToolsWindow::GetInstanceForInspectedWebContents(web_contents);
   if (devtools_window)
@@ skipping 1323 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         ui::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }