Remember user decisions on invalid certificates behind a flag

chrome/browser/ssl/chrome_ssl_host_state_decisions.cc

+// Copyright (c) 2014 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/ssl/chrome_ssl_host_state_decisions.h"
+
+#include "base/base64.h"
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
+#include "chrome/browser/content_settings/host_content_settings_map.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/content_settings_types.h"
+#include "url/gurl.h"
+
+namespace {
+
+enum CreateDictionaryEntriesDisposition {
+  CreateDictionaryEntries,
+  DoNotCreateDictionaryEntries
+};

[Ryan Sleevi, 2014/07/08 23:53:28]

Duplicate definition as the .cc. What's up with that?

[jww, 2014/07/11 00:08:40]

This was actually fixed in the latest patch set that I uploaded. It appears that
your review and my previous path upload appeared simultaneously. Real life
TOCTTOU!

+
+// This is a helper function that returns the length of time before a
+// certificate decision expires based on the command line flags. Returns a
+// non-negative value in seconds, where a value of 0 specifically indicates that
+// decisions should not be remembered after the current session has ended (but
+// should be remembered indefinitely as long as the session does not end), which
+// is the "old" style of certificate decision memory. Returns an int64 rather
+// than uint64 (even though guaranteed to be non-negative) because that's what
+// callers expect.
+int64 GetExpirationDelta() {
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kRememberCertErrorDecisions))
+    return 0;

[Ryan Sleevi, 2014/07/08 23:53:28]

Braces since you wrap?

[jww, 2014/07/11 00:08:41]

Done.

+
+  std::string switch_value =
+      CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kRememberCertErrorDecisions);
+  // If the expiration is set to 0, that means cert proceed decisions should
+  // never expire, until a restart of the profile.
+  int64 expiration_delta;
+  if (!base::StringToInt64(switch_value, &expiration_delta)) {
+    LOG(ERROR) << "Failed to parse the certificate error decision "
+               << "memory length: " << switch_value;

[Ryan Sleevi, 2014/07/08 23:53:28]

Seems to be spammy if the user didn't pass the switch.

[jww, 2014/07/11 00:08:41]

It shouldn't reach this if the user didn't include a switch because of the
HasSwitch check on line 35; this was just to make sure it was a valid value.
However, this is now irrelevant in the new patch set upload, since per felt's
suggestion, I changed it from a value switch to an explicit switch to work
better with Finch experiments.

+  } else if (expiration_delta < 0) {
+    expiration_delta = 0;
+    LOG(ERROR) << "Invalid negative certificate error decision memory length: "
+               << switch_value;
+  }
+
+  return expiration_delta;
+}
+
+const char kSSLCertDecisionCertErrorMapKey[] = "cert_exceptions_map";
+const char kSSLCertDecisionExpirationTimeKey[] = "decision_expiration_time";
+const char kSSLCertDecisionVersionKey[] = "version";
+const int kDefaultSSLCertDecisionVersion = 1;

[Ryan Sleevi, 2014/07/08 23:53:28]

anal retentive nit: Prefer constants declared first, as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
recommendation for classes

[jww, 2014/07/11 00:08:40]

Done.

+
+std::string GetKey(net::X509Certificate* cert, net::CertStatus error) {
+  net::SHA1HashValue fingerprint = cert->fingerprint();
+  std::string base64_fingerprint;
+  base::Base64Encode(
+      base::StringPiece(reinterpret_cast<const char*>(fingerprint.data),
+                        sizeof(fingerprint.data)),
+      &base64_fingerprint);
+  return base::UintToString(error) + base64_fingerprint;
+}
+
+}  // namespace
+
+// This helper function gets the dictionary of certificate fingerprints to
+// errors of certificates that have been accepted by the user from the content
+// dictionary that has been passed in. The returned pointer is owned by the the
+// argument dict that is passed in.
+//
+// If create_entries is set to |DoNotCreateDictionaryEntries|,
+// GetValidCertDecisionsDict will return NULL if there is anything invalid about
+// the setting, such as an invalid version or invalid value types (in addition
+// to there not be any values in the dictionary). If create_entries is set to
+// |CreateDictionaryEntries|, if no dictionary is found or the decisions are
+// expired, a new dictionary will be created
+base::DictionaryValue* ChromeSSLHostStateDecisions::GetValidCertDecisionsDict(
+    base::DictionaryValue* dict,
+    CreateDictionaryEntriesDisposition create_entries) {
+  // Extract the version of the certificate decision structure from the content
+  // setting.
+  int version;
+  bool success = dict->GetInteger(kSSLCertDecisionVersionKey, &version);
+  if (!success) {
+    if (create_entries == DoNotCreateDictionaryEntries)
+      return NULL;
+
+    dict->SetInteger(kSSLCertDecisionVersionKey,
+                     kDefaultSSLCertDecisionVersion);
+    version = kDefaultSSLCertDecisionVersion;
+  }
+
+  // If the version is somehow a newer version than Chrome can handle, there's
+  // really nothing to do other than fail silently and pretend it doesn't exist
+  // (or is malformed).
+  if (version > kDefaultSSLCertDecisionVersion)
+    return NULL;
+
+  // Extract the certificate decision's expiration time from the content
+  // setting. If there is no expiration time, that means it should never expire
+  // and it should resest only at session restart, so skip all of the expiration
+  // checks.
+  bool expired = false;
+  double decision_expiration_dbl;
+  base::Time now = clock_->Now();
+  base::Time decision_expiration;
+  if (dict->HasKey(kSSLCertDecisionExpirationTimeKey)) {
+    success = dict->GetDouble(kSSLCertDecisionExpirationTimeKey,
+                              &decision_expiration_dbl);
+    decision_expiration = base::Time::FromDoubleT(decision_expiration_dbl);
+  }
+
+  // Check if the user certificate decision has expired. If it has expired, and
+  // create_entries is |DoNotCreateDictionaryEntries|, treat the entry as if it
+  // doesn't exist and return |NULL|. If it has expired, and create_entries is
+  // |DoNotCreateDictionaryEntries|, set the entry's expiration date to the
+  // current time plus the expiration time delta.  Also, set expired to |true|
+  // which will force the create of a new dictionary in the dictionary
+  // extraction pass. None of this should be done if expiration happens on
+  // session end (i.e. the expiration delta is 0).

[Ryan Sleevi, 2014/07/08 23:53:28]

This feels like it's violating the cardinal rule of comments, in that it
describes what the code does, even though a few bits and pieces are smattered as
to why.

Perhaps a better structure for the comment is to list what the 'expected'
behaviours are, and let the reader validate the code matches said expectations?

// Check to see if the user's certificate decision has expired.
// - Expired and |create_entries| is DoNoCreateDicitionaryEntries, return NULL
// - Expired and |create_entries| is CreateDictionaryEntries, update the
expiration

Note that pipes around variables, not values, and you describe
expired+|DoNotCreateDictionaryEntries| twice, with two different behaviours.

In short, rework it all :)

[jww, 2014/07/11 00:08:41]

Done.

+  if (defaultSSLCertDecisionExpirationDelta_.InSeconds() > 0 &&
+      decision_expiration < now) {
+    if (create_entries == DoNotCreateDictionaryEntries)
+      return NULL;
+
+    expired = true;
+
+    // If defaultSSLCertDecisionExpirationDelta_ is 0, that means that decisions

[Ryan Sleevi, 2014/07/08 23:53:28]

bad variable name - default_ssl_cert_decision_expiration_delta

[jww, 2014/07/11 00:08:41]

Done.

+    // should never expire. Thus, in that case, no key should be set.
+    if (defaultSSLCertDecisionExpirationDelta_.InSeconds() != 0) {
+      base::Time expiration_time = now + defaultSSLCertDecisionExpirationDelta_;
+      dict->SetDouble(kSSLCertDecisionExpirationTimeKey,
+                      expiration_time.ToDoubleT());
+    }
+  }
+
+  // Extract the map of certificate fingerprints to errors from the setting.
+  base::DictionaryValue* cert_error_dict;  // Owned by dict
+  if (expired ||
+      !dict->GetDictionary(kSSLCertDecisionCertErrorMapKey, &cert_error_dict)) {
+    if (create_entries == DoNotCreateDictionaryEntries)
+      return NULL;
+
+    cert_error_dict = new base::DictionaryValue();
+    // dict takes ownership of cert_error_dict
+    dict->Set(kSSLCertDecisionCertErrorMapKey, cert_error_dict);
+  }
+
+  return cert_error_dict;
+}
+
+// If defaultSSLCertDecisionExpirationDelta_ is 0, that means that all invalid
+// certificate proceed decisions should be forgotten when the session ends. At
+// attempt is made in the destructor to remove the entries, but in the case that
+// things didn't shut down cleanly, on start, Clear is called to guarantee a
+// clean state.
+ChromeSSLHostStateDecisions::ChromeSSLHostStateDecisions(Profile* profile)
+    : clock_(new base::DefaultClock()),
+      defaultSSLCertDecisionExpirationDelta_(
+          base::TimeDelta::FromSeconds(GetExpirationDelta())),
+      profile_(profile) {
+  if (defaultSSLCertDecisionExpirationDelta_.InSeconds() == 0)
+    Clear();
+}
+
+ChromeSSLHostStateDecisions::~ChromeSSLHostStateDecisions() {
+  if (defaultSSLCertDecisionExpirationDelta_.InSeconds() == 0)
+    Clear();
+}
+
+void ChromeSSLHostStateDecisions::DenyCert(const GURL& url,
+                                           net::X509Certificate* cert,
+                                           net::CertStatus error) {
+  ChangeCertPolicy(url, cert, error, net::CertPolicy::DENIED);
+}
+
+void ChromeSSLHostStateDecisions::AllowCert(const GURL& url,
+                                            net::X509Certificate* cert,
+                                            net::CertStatus error) {
+  ChangeCertPolicy(url, cert, error, net::CertPolicy::ALLOWED);
+}
+
+void ChromeSSLHostStateDecisions::Clear() {
+  profile_->GetHostContentSettingsMap()->ClearSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS);
+}
+
+net::CertPolicy::Judgment ChromeSSLHostStateDecisions::QueryPolicy(
+    const GURL& url,
+    net::X509Certificate* cert,
+    net::CertStatus error) {
+  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+  scoped_ptr<base::Value> value(map->GetWebsiteSetting(
+      url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));

[Ryan Sleevi, 2014/07/08 23:53:28]

std::string(), not ""

[jww, 2014/07/11 00:08:40]

Done.

+
+  DCHECK(value.get());
+
+  if (!value->IsType(base::Value::TYPE_DICTIONARY))
+    return net::CertPolicy::UNKNOWN;
+
+  base::DictionaryValue* dict;  // Owned by value
+  int policy_decision;          // Owned by dict
+  bool success = value->GetAsDictionary(&dict);
+  DCHECK(success);
+
+  // First, check that the version of the dictionary of decisions is a version
+  // that is known. Otherwise, fail.
+  base::DictionaryValue* cert_error_dict;  // Owned by value
+  cert_error_dict =
+      GetValidCertDecisionsDict(dict, DoNotCreateDictionaryEntries);
+  if (!cert_error_dict)
+    return net::CertPolicy::UNKNOWN;
+
+  success = cert_error_dict->GetIntegerWithoutPathExpansion(GetKey(cert, error),
+                                                            &policy_decision);
+
+  if (!success)
+    return net::CertPolicy::Judgment::UNKNOWN;
+
+  return static_cast<net::CertPolicy::Judgment>(policy_decision);
+}
+
+void ChromeSSLHostStateDecisions::RevokeAllowAndDenyPreferences(
+    const GURL& url) {
+  const ContentSettingsPattern pattern =
+      ContentSettingsPattern::FromURLNoWildcard(url);
+  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+
+  map->SetWebsiteSetting(
+      pattern, pattern, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL);

[Ryan Sleevi, 2014/07/08 23:53:28]

std::string() - and throughout rest of file.

[jww, 2014/07/11 00:08:41]

Done.

+}
+
+bool ChromeSSLHostStateDecisions::HasAllowedOrDeniedCert(const GURL& url) {
+  const ContentSettingsPattern pattern =
+      ContentSettingsPattern::FromURLNoWildcard(url);
+  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+
+  scoped_ptr<base::Value> value(map->GetWebsiteSetting(
+      url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));

[Ryan Sleevi, 2014/07/08 23:53:28]

Why don't you DCHECK here, but you do on line 204?

[jww, 2014/07/11 00:08:40]

Good catch. The DCHECK on 204 was from testing earlier, and really this should
all explicitly test for NULL.

+
+  if (!value->IsType(base::Value::TYPE_DICTIONARY))
+    return false;
+
+  base::DictionaryValue* dict;  // Owned by value
+  bool success = value->GetAsDictionary(&dict);
+  DCHECK(success);
+
+  for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
+    int policy_decision;  // Owned by dict
+    success = it.value().GetAsInteger(&policy_decision);
+    if (success && (static_cast<net::CertPolicy::Judgment>(policy_decision) !=
+                    net::CertPolicy::UNKNOWN))
+      return true;
+  }
+
+  return false;
+}
+
+void ChromeSSLHostStateDecisions::ChangeCertPolicy(
+    const GURL& url,
+    net::X509Certificate* cert,
+    net::CertStatus error,
+    net::CertPolicy::Judgment judgment) {
+  const ContentSettingsPattern pattern =
+      ContentSettingsPattern::FromURLNoWildcard(url);
+  HostContentSettingsMap* map = profile_->GetHostContentSettingsMap();
+  scoped_ptr<base::Value> value(map->GetWebsiteSetting(
+      url, url, CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS, "", NULL));

[Ryan Sleevi, 2014/07/08 23:53:28]

ditto DCHECK

[jww, 2014/07/11 00:08:41]

See above.

+
+  if (!value->IsType(base::Value::TYPE_DICTIONARY))
+    value.reset(new base::DictionaryValue());
+
+  base::DictionaryValue* dict;
+  bool success = value->GetAsDictionary(&dict);
+  DCHECK(success);
+
+  base::DictionaryValue* cert_dict =
+      GetValidCertDecisionsDict(dict, CreateDictionaryEntries);
+  // If a a valid certificate dictionary cannot be extracted from the content
+  // setting, that means it's in an unknown format. Unfortunately, there's
+  // nothing to be done in that case, so a silent fail is the only option.
+  if (!cert_dict)
+    return;
+
+  dict->SetIntegerWithoutPathExpansion(kSSLCertDecisionVersionKey,
+                                       kDefaultSSLCertDecisionVersion);
+  cert_dict->SetIntegerWithoutPathExpansion(GetKey(cert, error), judgment);
+
+  // The map takes ownership of the value, so it is released in the call to
+  // SetWebsiteSetting.
+  map->SetWebsiteSetting(pattern,
+                         pattern,
+                         CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS,
+                         "",
+                         value.release());
+}