Password manager internals page: Improve security

components/autofill/core/common/save_password_progress_logger.cc

 // Copyright 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 "components/autofill/core/common/save_password_progress_logger.h"
 
+#include <algorithm>
+
 #include "base/json/json_writer.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/autofill/core/common/password_form.h"
 
 using base::checked_cast;
 using base::Value;
 using base::DictionaryValue;
 using base::FundamentalValue;
 using base::StringValue;
 
 namespace autofill {
 
 namespace {
 
+// Note 1: Caching the ID->string map in an array would be probably faster, but
+// the switch statement is (a) robust against re-ordering, and (b) checks in
+// compile-time, that all IDs get a string assigned. The expected frequency of
+// calls is low enough (in particular, zero if password manager internals page
+// is not open), that optimizing for code robustness is preferred against speed.
+// Note 2: The messages can be used as dictionary keys. Do not use '.' in them.
+std::string GetStringFromID(SavePasswordProgressLogger::StringID id) {
+  switch (id) {
+    case SavePasswordProgressLogger::STRING_DECISION_ASK:
+      return "Decision: ASK the user";
+    case SavePasswordProgressLogger::STRING_DECISION_DROP:
+      return "Decision: DROP the password";
+    case SavePasswordProgressLogger::STRING_DECISION_SAVE:
+      return "Decision: SAVE the password";
+    case SavePasswordProgressLogger::STRING_METHOD:
+      return "Form method";
+    case SavePasswordProgressLogger::STRING_METHOD_GET:
+      return "GET";
+    case SavePasswordProgressLogger::STRING_METHOD_POST:
+      return "POST";
+    case SavePasswordProgressLogger::STRING_METHOD_EMPTY:
+      return "(empty)";
+    case SavePasswordProgressLogger::STRING_OTHER:
+      return "(other)";
+    case SavePasswordProgressLogger::STRING_SCHEME_HTML:
+      return "HTML";
+    case SavePasswordProgressLogger::STRING_SCHEME_BASIC:
+      return "Basic";
+    case SavePasswordProgressLogger::STRING_SCHEME_DIGEST:
+      return "Digest";
+    case SavePasswordProgressLogger::STRING_SCHEME_MESSAGE:
+      return "Scheme";
+    case SavePasswordProgressLogger::STRING_SIGNON_REALM:
+      return "Signon realm";
+    case SavePasswordProgressLogger::STRING_ORIGINAL_SIGNON_REALM:
+      return "Original signon realm";
+    case SavePasswordProgressLogger::STRING_ORIGIN:
+      return "Origin";
+    case SavePasswordProgressLogger::STRING_ACTION:
+      return "Action";
+    case SavePasswordProgressLogger::STRING_USERNAME_ELEMENT:
+      return "Username element";
+    case SavePasswordProgressLogger::STRING_PASSWORD_ELEMENT:
+      return "Password element";
+    case SavePasswordProgressLogger::STRING_PASSWORD_AUTOCOMPLETE_SET:
+      return "Password autocomplete set";
+    case SavePasswordProgressLogger::STRING_OLD_PASSWORD_ELEMENT:
+      return "Old password element";
+    case SavePasswordProgressLogger::STRING_SSL_VALID:
+      return "SSL valid";
+    case SavePasswordProgressLogger::STRING_PASSWORD_GENERATED:
+      return "Password generated";
+    case SavePasswordProgressLogger::STRING_TIMES_USED:
+      return "Times used";
+    case SavePasswordProgressLogger::STRING_USE_ADDITIONAL_AUTHENTICATION:
+      return "Use additional authentication";
+    case SavePasswordProgressLogger::STRING_PSL_MATCH:
+      return "PSL match";
+    case SavePasswordProgressLogger::STRING_NAME_OR_ID:
+      return "Form name or ID";
+    case SavePasswordProgressLogger::STRING_MESSAGE:
+      return "Message";
+    case SavePasswordProgressLogger::STRING_INVALID:
+      return "INVALID";
+      // Intentionally no default: clause here -- all IDs need to get covered.
+  }
+  NOTREACHED();  // Win compilers don't believe this is unreachable.
+  return std::string();
+};
+
 // Removes privacy sensitive parts of |url| (currently all but host and scheme).
 std::string ScrubURL(const GURL& url) {
   if (url.is_valid())
     return url.GetWithEmptyPath().spec();
   return std::string();
 }
 
-std::string FormSchemeToString(PasswordForm::Scheme scheme) {
+// Returns true for all characters which we don't want to see in the logged IDs
+// or names of HTML elements.
+bool IsUnwantedInElementID(char c) {
+  return !(c == '_' || c == '-' || IsAsciiAlpha(c) || IsAsciiDigit(c));
+}
+
+// Replaces all characters satisfying IsUnwantedInElementID by a ' ', and turns
+// all characters to lowercase. This damages some valid HTML element IDs or
+// names, but it is likely that it will be still possible to match the scrubbed
+// string to the original ID or name in the HTML doc. That's good enough for the
+// logging purposes, and provides some security benefits.
+std::string ScrubElementID(std::string element_id) {
+  std::replace_if(
+      element_id.begin(), element_id.end(), IsUnwantedInElementID, ' ');
+  return StringToLowerASCII(element_id);
+}
+
+SavePasswordProgressLogger::StringID FormSchemeToStringID(
+    PasswordForm::Scheme scheme) {
   switch (scheme) {
     case PasswordForm::SCHEME_HTML:
-      return "HTML";
+      return SavePasswordProgressLogger::STRING_SCHEME_HTML;
     case PasswordForm::SCHEME_BASIC:
-      return "BASIC";
+      return SavePasswordProgressLogger::STRING_SCHEME_BASIC;
     case PasswordForm::SCHEME_DIGEST:
-      return "DIGEST";
+      return SavePasswordProgressLogger::STRING_SCHEME_DIGEST;
     case PasswordForm::SCHEME_OTHER:
-      return "OTHER";
+      return SavePasswordProgressLogger::STRING_OTHER;
   }
   NOTREACHED();  // Win compilers don't believe this is unreachable.
-  return std::string();
-}
-
-StringValue DecisionToStringValue(
-    SavePasswordProgressLogger::Decision decision) {
-  switch (decision) {
-    case SavePasswordProgressLogger::DECISION_SAVE:
-      return StringValue("SAVE the password");
-    case SavePasswordProgressLogger::DECISION_ASK:
-      return StringValue("ASK the user whether to save the password");
-    case SavePasswordProgressLogger::DECISION_DROP:
-      return StringValue("DROP the password");
-  }
-  NOTREACHED();  // Win compilers don't believe this is unreachable.
-  return StringValue(std::string());
+  return SavePasswordProgressLogger::STRING_INVALID;
+}
+
+SavePasswordProgressLogger::StringID FormMethodToStringID(
+    const std::string& method) {
+  std::string method_processed;
+  base::TrimWhitespaceASCII(
+      StringToLowerASCII(method), base::TRIM_ALL, &method_processed);
+  if (method_processed.empty())
+    return SavePasswordProgressLogger::STRING_METHOD_EMPTY;
+  else if (method_processed == "get")
+    return SavePasswordProgressLogger::STRING_METHOD_GET;
+  else if (method_processed == "post")
+    return SavePasswordProgressLogger::STRING_METHOD_POST;
+  else
+    return SavePasswordProgressLogger::STRING_OTHER;
+}
+
+// GetValueFrom*: Sanitize the arguments, and return them as base::Value.
+scoped_ptr<Value> GetValueFromURL(const GURL& url) {
+  return scoped_ptr<Value>(Value::CreateStringValue(ScrubURL(url)));
+}
+
+scoped_ptr<Value> GetValueFromMessage(
+    SavePasswordProgressLogger::StringID message) {
+  return scoped_ptr<Value>(Value::CreateStringValue(GetStringFromID(message)));
+}
+
+scoped_ptr<Value> GetValueFromNumber(int number) {
+  return scoped_ptr<Value>(Value::CreateIntegerValue(number));
+}
+
+scoped_ptr<Value> GetValueFromBoolean(bool truth_value) {
+  return scoped_ptr<Value>(Value::CreateBooleanValue(truth_value));
+}
+
+scoped_ptr<Value> GetValueFromElementID(const std::string& element_id) {
+  return scoped_ptr<Value>(
+      Value::CreateStringValue(ScrubElementID(element_id)));
+}
+
+void AddURLToDict(DictionaryValue* dict,
+                  SavePasswordProgressLogger::StringID label,
+                  const GURL& url) {
+  dict->Set(GetStringFromID(label), GetValueFromURL(url).release());
+}
+
+void AddMessageToDict(DictionaryValue* dict,
+                      SavePasswordProgressLogger::StringID label,
+                      SavePasswordProgressLogger::StringID message) {
+  dict->Set(GetStringFromID(label), GetValueFromMessage(message).release());
+}
+
+void AddNumberToDict(DictionaryValue* dict,
+                     SavePasswordProgressLogger::StringID label,
+                     int number) {
+  dict->Set(GetStringFromID(label), GetValueFromNumber(number).release());
+}
+
+void AddBooleanToDict(DictionaryValue* dict,
+                      SavePasswordProgressLogger::StringID label,
+                      bool truth_value) {
+  dict->Set(GetStringFromID(label), GetValueFromBoolean(truth_value).release());
+}
+
+void AddElementIDToDict(DictionaryValue* dict,
+                        SavePasswordProgressLogger::StringID label,
+                        const std::string& element_id) {
+  dict->Set(GetStringFromID(label),
+            GetValueFromElementID(element_id).release());
+}
+
+void AddElementIDToDict(DictionaryValue* dict,
+                        SavePasswordProgressLogger::StringID label,
+                        const base::string16& element_id) {
+  dict->Set(GetStringFromID(label),
+            GetValueFromElementID(base::UTF16ToUTF8(element_id)).release());
 }
 
 }  // namespace
 
-SavePasswordProgressLogger::SavePasswordProgressLogger() {}
-
-SavePasswordProgressLogger::~SavePasswordProgressLogger() {}
-
-void SavePasswordProgressLogger::LogPasswordForm(const std::string& message,
-                                                 const PasswordForm& form) {
+SavePasswordProgressLogger::SavePasswordProgressLogger() {
+}
+
+SavePasswordProgressLogger::~SavePasswordProgressLogger() {
+}
+
+void SavePasswordProgressLogger::LogPasswordForm(
+    SavePasswordProgressLogger::StringID label,
+    const PasswordForm& form) {
   DictionaryValue log;
-  // Do not use the "<<" operator for PasswordForms, because it also prints
-  // passwords. Also, that operator is only for testing.
-  log.SetString("scheme", FormSchemeToString(form.scheme));
-  log.SetString("signon realm", ScrubURL(GURL(form.signon_realm)));
-  log.SetString("original signon realm",
-                ScrubURL(GURL(form.original_signon_realm)));
-  log.SetString("origin", ScrubURL(form.origin));
-  log.SetString("action", ScrubURL(form.action));
-  log.SetString("username element", form.username_element);
-  log.SetString("password element", form.password_element);
-  log.SetBoolean("password autocomplete set", form.password_autocomplete_set);
-  log.SetString("old password element", form.old_password_element);
-  log.SetBoolean("ssl valid", form.ssl_valid);
-  log.SetBoolean("password generated",
-                 form.type == PasswordForm::TYPE_GENERATED);
-  log.SetInteger("times used", form.times_used);
-  log.SetBoolean("use additional authentication",
-                 form.use_additional_authentication);
-  log.SetBoolean("is PSL match", form.IsPublicSuffixMatch());
-  LogValue(message, log);
-}
-
-void SavePasswordProgressLogger::LogHTMLForm(const std::string& message,
-                                             const std::string& name_or_id,
-                                             const std::string& method,
-                                             const GURL& action) {
+  AddMessageToDict(
+      &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme));

[Ilya Sherman, 2014/04/17 21:12:05]

Comparing

AddMessageToDict(
    &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme));

vs.

log.SetString(GetStringFromID(STRING_SCHEME_MESSAGE),
              FormSchemeToString(form.scheme));

I actually think the second is more readable, because it's clear to me from the
call site exactly what's happening: A single element is being added to the
dictionary.  With AddMessageToDict(), I have to look at that method to see that
it's just setting a single field.  Plus, the Add*ToDict and GetValueFrom*
methods add something like 60 lines of boilerplate.

All that said, this is a fairly minor style issue.  If you really don't find my
reasoning convincing, then I won't block this CL over this disagreement.

[vabr (Chromium), 2014/04/19 07:39:53]

It was, in fact, very convincing.
Happy to remove the unnecessary helper methods. Thanks!

+  AddMessageToDict(
+      &log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme));
+  AddURLToDict(&log, STRING_SIGNON_REALM, GURL(form.signon_realm));
+  AddURLToDict(
+      &log, STRING_ORIGINAL_SIGNON_REALM, GURL(form.original_signon_realm));
+  AddURLToDict(&log, STRING_ORIGIN, form.origin);
+  AddURLToDict(&log, STRING_ACTION, form.action);
+  AddElementIDToDict(&log, STRING_USERNAME_ELEMENT, form.username_element);
+  AddElementIDToDict(&log, STRING_PASSWORD_ELEMENT, form.password_element);
+  AddBooleanToDict(
+      &log, STRING_PASSWORD_AUTOCOMPLETE_SET, form.password_autocomplete_set);
+  AddElementIDToDict(
+      &log, STRING_OLD_PASSWORD_ELEMENT, form.old_password_element);
+  AddBooleanToDict(&log, STRING_SSL_VALID, form.ssl_valid);
+  AddBooleanToDict(&log,
+                   STRING_PASSWORD_GENERATED,
+                   form.type == PasswordForm::TYPE_GENERATED);
+  AddNumberToDict(&log, STRING_TIMES_USED, form.times_used);
+  AddBooleanToDict(&log,
+                   STRING_USE_ADDITIONAL_AUTHENTICATION,
+                   form.use_additional_authentication);
+  AddBooleanToDict(&log, STRING_PSL_MATCH, form.IsPublicSuffixMatch());
+  LogValue(label, &log);
+}
+
+void SavePasswordProgressLogger::LogHTMLForm(
+    SavePasswordProgressLogger::StringID label,
+    const std::string& name_or_id,
+    const std::string& method,
+    const GURL& action) {
   DictionaryValue log;
-  log.SetString("name_or_id", name_or_id);
-  log.SetString("method", method);
-  log.SetString("action", ScrubURL(action));
-  LogValue(message, log);
-}
-
-void SavePasswordProgressLogger::LogURL(const std::string& message,
-                                        const GURL& url) {
-  LogValue(message, StringValue(ScrubURL(url)));
-}
-
-void SavePasswordProgressLogger::LogBoolean(const std::string& message,
-                                            bool value) {
-  LogValue(message, FundamentalValue(value));
-}
-
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           int value) {
-  LogValue(message, FundamentalValue(value));
-}
-
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           size_t value) {
-  LogValue(message, FundamentalValue(checked_cast<int, size_t>(value)));
-}
-
-void SavePasswordProgressLogger::LogFinalDecision(Decision decision) {
-  LogValue("Final decision taken", DecisionToStringValue(decision));
-}
-
-void SavePasswordProgressLogger::LogMessage(const std::string& message) {
-  LogValue("Message", StringValue(message));
-}
-
-void SavePasswordProgressLogger::LogValue(const std::string& name,
-                                          const Value& log) {
+  AddElementIDToDict(&log, STRING_NAME_OR_ID, name_or_id);
+  AddMessageToDict(&log, STRING_METHOD, FormMethodToStringID(method));
+  AddURLToDict(&log, STRING_ACTION, action);
+  LogValue(label, &log);
+}
+
+void SavePasswordProgressLogger::LogURL(
+    SavePasswordProgressLogger::StringID label,
+    const GURL& url) {
+  LogValue(label, GetValueFromURL(url).get());
+}
+
+void SavePasswordProgressLogger::LogBoolean(
+    SavePasswordProgressLogger::StringID label,
+    bool truth_value) {
+  LogValue(label, GetValueFromBoolean(truth_value).get());
+}
+
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    int signed_number) {
+  LogValue(label, GetValueFromNumber(signed_number).get());
+}
+
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    size_t unsigned_number) {
+  LogValue(
+      label,
+      GetValueFromNumber(checked_cast<int, size_t>(unsigned_number)).get());
+}
+
+void SavePasswordProgressLogger::LogMessage(
+    SavePasswordProgressLogger::StringID message) {
+  LogValue(STRING_MESSAGE, GetValueFromMessage(message).get());
+}
+
+void SavePasswordProgressLogger::LogValue(StringID label, const Value* log) {
   std::string log_string;
   bool conversion_to_string_successful = base::JSONWriter::WriteWithOptions(
-      &log, base::JSONWriter::OPTIONS_PRETTY_PRINT, &log_string);
+      log, base::JSONWriter::OPTIONS_PRETTY_PRINT, &log_string);
   DCHECK(conversion_to_string_successful);
-  SendLog(name + ": " + log_string);
+  SendLog(GetStringFromID(label) + ": " + log_string);
 }
 
 }  // namespace autofill