Password manager internals page: Improve security

components/autofill/core/common/save_password_progress_logger.cc

Index: components/autofill/core/common/save_password_progress_logger.cc
diff --git a/components/autofill/core/common/save_password_progress_logger.cc b/components/autofill/core/common/save_password_progress_logger.cc
index cdb8dd7a177e97f1542db30a714749683b3094f1..a580b827b463c013c2f1f366c56fef0e805429a0 100644
--- a/components/autofill/core/common/save_password_progress_logger.cc
+++ b/components/autofill/core/common/save_password_progress_logger.cc
@@ -4,9 +4,15 @@
 
 #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"
 
@@ -20,6 +26,76 @@ 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 agains speed.

[Ilya Sherman, 2014/04/16 22:02:11]

nit: "agains" -> "against"

[vabr (Chromium), 2014/04/17 12:33:09]

Done.

+// 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 NULL;

[Ilya Sherman, 2014/04/16 22:02:11]

NULL isn't a valid std::string.  I think you meant "std::string()" instead.

[vabr (Chromium), 2014/04/17 12:33:09]

Thanks! That was a relic from the time this returned const char*.
Done.

+};
+
 // Removes privacy sensitive parts of |url| (currently all but host and scheme).
 std::string ScrubURL(const GURL& url) {
   if (url.is_valid())
@@ -27,112 +103,173 @@ std::string ScrubURL(const GURL& url) {
   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 !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();
+  return SavePasswordProgressLogger::StringID::STRING_INVALID;
 }
 
-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());
+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;
+}
+
+// GetValueFromLog: Sanitize the arguments, and return them as base::Value.
+scoped_ptr<Value> GetValueFromLog(const GURL& url) {
+  return scoped_ptr<Value>(Value::CreateStringValue(ScrubURL(url)));
+}
+
+scoped_ptr<Value> GetValueFromLog(
+    SavePasswordProgressLogger::StringID message) {
+  return scoped_ptr<Value>(Value::CreateStringValue(GetStringFromID(message)));
+}
+
+scoped_ptr<Value> GetValueFromLog(int number) {
+  return scoped_ptr<Value>(Value::CreateIntegerValue(number));
+}
+
+scoped_ptr<Value> GetValueFromLog(bool truth_value) {
+  return scoped_ptr<Value>(Value::CreateBooleanValue(truth_value));
+}
+
+scoped_ptr<Value> GetValueFromLog(const std::string& element_id) {
+  return scoped_ptr<Value>(
+      Value::CreateStringValue(ScrubElementID(element_id)));
+}
+
+scoped_ptr<Value> GetValueFromLog(const base::string16& element_id) {
+  return scoped_ptr<Value>(
+      Value::CreateStringValue(ScrubElementID(UTF16ToUTF8(element_id))));
+}
+
+template <class T>
+void AddToDict(DictionaryValue* dict,
+               SavePasswordProgressLogger::StringID label,
+               T log) {
+  dict->Set(GetStringFromID(label), GetValueFromLog(log).release());
 }

[Ilya Sherman, 2014/04/16 22:02:11]

Hmm, I'm not thrilled about the addition of this method and all of the
overloaded GetValueFromLog() methods.  Overloaded methods are discouraged by the
style guide, and by most reviewers in general.  Why did you switch to this
approach, rather than keeping the previous design?

[vabr (Chromium), 2014/04/17 12:33:09]

As for the style guide, it says: "Use overloaded functions (including
constructors) only if a reader looking at a call site can get a good idea of
what is happening without having to first figure out exactly which overload is
being called."
I understood it as: so long the function does the same thing (here: convert some
input to a base::Value), it should be OK to name it the same for multiple
type-dependent implementations.
But I acknowledge that the cited piece of the guide allows wide interpretations.
My goal was to make the inside of LogPasswordForm readable. That means that the
code adding a value to the dictionary should clearly display what values are
added, as opposed to "GetStringFromID" boilerplate. I will do it without the
template and overloading, at the cost of having different copies of AddToDict,
but also at the benefit of knowing the data type straight from the Add* method
name. Both is acceptable to me, please let me know if the new code looks better
to you.

Note: I still kept one instance of overloading: AddElementIDToDict for
std::string and base::string16. Overloading for 8- and 16-bit versions of
strings seems to be accepted in the codebase. If you disagree, just let me know
and I'll split the name here as well.

 
 }  // namespace
 
-SavePasswordProgressLogger::SavePasswordProgressLogger() {}
+SavePasswordProgressLogger::SavePasswordProgressLogger() {
+}
 
-SavePasswordProgressLogger::~SavePasswordProgressLogger() {}
+SavePasswordProgressLogger::~SavePasswordProgressLogger() {
+}
 
-void SavePasswordProgressLogger::LogPasswordForm(const std::string& message,
-                                                 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) {
+void SavePasswordProgressLogger::LogPasswordForm(
+    SavePasswordProgressLogger::StringID label,
+    const PasswordForm& form) {
   DictionaryValue log;
-  log.SetString("name_or_id", name_or_id);
-  log.SetString("method", method);
-  log.SetString("action", ScrubURL(action));
-  LogValue(message, log);
+  AddToDict(&log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme));
+  AddToDict(&log, STRING_SCHEME_MESSAGE, FormSchemeToStringID(form.scheme));
+  AddToDict(&log, STRING_SIGNON_REALM, GURL(form.signon_realm));
+  AddToDict(
+      &log, STRING_ORIGINAL_SIGNON_REALM, GURL(form.original_signon_realm));
+  AddToDict(&log, STRING_ORIGIN, form.origin);
+  AddToDict(&log, STRING_ACTION, form.action);
+  AddToDict(&log, STRING_USERNAME_ELEMENT, form.username_element);
+  AddToDict(&log, STRING_PASSWORD_ELEMENT, form.password_element);
+  AddToDict(
+      &log, STRING_PASSWORD_AUTOCOMPLETE_SET, form.password_autocomplete_set);
+  AddToDict(&log, STRING_OLD_PASSWORD_ELEMENT, form.old_password_element);
+  AddToDict(&log, STRING_SSL_VALID, form.ssl_valid);
+  AddToDict(&log,
+            STRING_PASSWORD_GENERATED,
+            form.type == PasswordForm::TYPE_GENERATED);
+  AddToDict(&log, STRING_TIMES_USED, form.times_used);
+  AddToDict(&log,
+            STRING_USE_ADDITIONAL_AUTHENTICATION,
+            form.use_additional_authentication);
+  AddToDict(&log, STRING_PSL_MATCH, form.IsPublicSuffixMatch());
+  LogValue(label, &log);
 }
 
-void SavePasswordProgressLogger::LogURL(const std::string& message,
-                                        const GURL& url) {
-  LogValue(message, StringValue(ScrubURL(url)));
+void SavePasswordProgressLogger::LogHTMLForm(
+    SavePasswordProgressLogger::StringID label,
+    const std::string& name_or_id,
+    const std::string& method,
+    const GURL& action) {
+  DictionaryValue log;
+  AddToDict(&log, STRING_NAME_OR_ID, name_or_id);
+  AddToDict(&log, STRING_METHOD, FormMethodToStringID(method));
+  AddToDict(&log, STRING_ACTION, action);
+  LogValue(label, &log);
 }
 
-void SavePasswordProgressLogger::LogBoolean(const std::string& message,
-                                            bool value) {
-  LogValue(message, FundamentalValue(value));
+void SavePasswordProgressLogger::LogURL(
+    SavePasswordProgressLogger::StringID label,
+    const GURL& url) {
+  LogValue(label, GetValueFromLog(url).get());
 }
 
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           int value) {
-  LogValue(message, FundamentalValue(value));
+void SavePasswordProgressLogger::LogBoolean(
+    SavePasswordProgressLogger::StringID label,
+    bool truth_value) {
+  LogValue(label, GetValueFromLog(truth_value).get());
 }
 
-void SavePasswordProgressLogger::LogNumber(const std::string& message,
-                                           size_t value) {
-  LogValue(message, FundamentalValue(checked_cast<int, size_t>(value)));
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    int signed_number) {
+  LogValue(label, GetValueFromLog(signed_number).get());
 }
 
-void SavePasswordProgressLogger::LogFinalDecision(Decision decision) {
-  LogValue("Final decision taken", DecisionToStringValue(decision));
+void SavePasswordProgressLogger::LogNumber(
+    SavePasswordProgressLogger::StringID label,
+    size_t unsigned_number) {
+  LogValue(label,
+           GetValueFromLog(checked_cast<int, size_t>(unsigned_number)).get());
 }
 
-void SavePasswordProgressLogger::LogMessage(const std::string& message) {
-  LogValue("Message", StringValue(message));
+void SavePasswordProgressLogger::LogMessage(
+    SavePasswordProgressLogger::StringID message) {
+  LogValue(STRING_MESSAGE, GetValueFromLog(message).get());
 }
 
-void SavePasswordProgressLogger::LogValue(const std::string& name,
-                                          const Value& log) {
+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