Password manager internals page: Improve security

components/autofill/core/common/save_password_progress_logger.h

Index: components/autofill/core/common/save_password_progress_logger.h
diff --git a/components/autofill/core/common/save_password_progress_logger.h b/components/autofill/core/common/save_password_progress_logger.h
index 9df99080152f6479ab5d61aeaee518a491ab7d68..bfea98c36c4bb4e37152465b068e799584fb7cd4 100644
--- a/components/autofill/core/common/save_password_progress_logger.h
+++ b/components/autofill/core/common/save_password_progress_logger.h
@@ -6,7 +6,9 @@
 #define COMPONENTS_AUTOFILL_CORE_COMMON_SAVE_PASSWORD_PROGRESS_LOGGER_H_
 
 #include <string>
+#include <vector>
 
+#include "base/strings/string16.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -32,39 +34,123 @@ struct PasswordForm;
 // to stay in autofill as well.
 class SavePasswordProgressLogger {
  public:
-  // All three possible decisions about saving a password. Call LogFinalDecision
-  // as soon as one is taken by the password management code.
-  enum Decision { DECISION_SAVE, DECISION_ASK, DECISION_DROP };
+  // IDs of strings allowed in the logs: for security reasons, we only pass the
+  // IDs from the renderer, and map them to strings in the browser.
+  // Make sure to keep this in the same order as |kIDsToMessages| in the .cc
+  // file.

[Ilya Sherman, 2014/04/12 00:00:51]

Rather than requiring the order to match, please just use a switch stmt with
cases.  It's a little longer, but it's also more robust.

[vabr (Chromium), 2014/04/13 20:12:30]

Done.

+  // The code relies on them being automatically numbered, starting with 0 and
+  // increasing by 1 (guaranteed by the C++ standard, see ISO/IEC 14882:2003
+  // 7.2.1).
+  enum StringID {
+    STRING_DECISION_ASK,
+    STRING_DECISION_DROP,
+    STRING_DECISION_SAVE,
+    STRING_METHOD,
+    STRING_METHOD_GET,
+    STRING_METHOD_POST,
+    STRING_METHOD_EMPTY,
+    STRING_OTHER,
+    STRING_SCHEME_HTML,
+    STRING_SCHEME_BASIC,
+    STRING_SCHEME_DIGEST,
+    STRING_SCHEME_MESSAGE,
+    STRING_SIGNON_REALM,
+    STRING_ORIGINAL_SIGNON_REALM,
+    STRING_ORIGIN,
+    STRING_ACTION,
+    STRING_USERNAME_ELEMENT,
+    STRING_PASSWORD_ELEMENT,
+    STRING_PASSWORD_AUTOCOMPLETE_SET,
+    STRING_OLD_PASSWORD_ELEMENT,
+    STRING_SSL_VALID,
+    STRING_PASSWORD_GENERATED,
+    STRING_TIMES_USED,
+    STRING_USE_ADDITIONAL_AUTHENTICATION,
+    STRING_PSL_MATCH,
+    STRING_NAME_OR_ID,
+    STRING_MESSAGE,
+    STRING_INVALID,  // Represents a string returned in a case of an error.
+    STRING_MAX = STRING_INVALID
+  };
+
+  // A "structured" log, as opposed to just a log string, is passed from the
+  // place of log creation (which might be in a renderer code) to browser for
+  // display. We need to keep it structured to allow browser-side sanitization
+  // for security reasons.
+  struct StructuredLog {
+    // Each log has one of these types. Depending on the type, the valid data
+    // members are shown in comments (in addition to |log_type| itself).
+    enum LogType {
+      LOG_TYPE_LABEL,       // |label|
+      LOG_TYPE_GURL,        // |label|, |url|
+      LOG_TYPE_MESSAGE,     // |label|, |message|
+      LOG_TYPE_NUMBER,      // |label|, |number|
+      LOG_TYPE_BOOL,        // |label|, |truth_value|
+      LOG_TYPE_ELEMENT_ID,  // |label|, |element_id|
+      LOG_TYPE_MAX = LOG_TYPE_ELEMENT_ID
+    };
+
+    LogType log_type;
+    StringID label;
+    // The members below would ideally form a union, but cannot, because of
+    // non-POD members. The size overhead does not seem worse than having
+    // multiple IPC methods specialized for each possible log type.
+    GURL url;
+    StringID message;
+    int number;
+    bool truth_value;
+    // Only use |element_id| to store an HTML element name or id; for security
+    // reasons all characters other than alphanumeric ones will be replaced by
+    // spaces.
+    std::string element_id;
+
+    StructuredLog();
+    explicit StructuredLog(StringID label);
+    StructuredLog(StringID label, GURL url);
+    StructuredLog(StringID label, StringID message);
+    StructuredLog(StringID label, int number);
+    StructuredLog(StringID label, bool truth_value);
+    StructuredLog(StringID label, const std::string& element_id);
+    StructuredLog(StringID label, const base::string16& element_id);
+    ~StructuredLog();
+
+    // Equality operators for testing.
+    bool operator==(const StructuredLog& form) const;
+    bool operator!=(const StructuredLog& form) const;
+  };

[Ilya Sherman, 2014/04/12 00:00:51]

I'm not convinced that passing a struct rather than a string across the process
boundary improves security in any way.  I think it's sufficient to just replace
the string "label" parameters with StringIDs instead.

If either of you disagree, could you please convey what security or privacy
threat the StructuredLog class prevents, over and above the guarantees made by
the LogFoo() methods below?

[vabr (Chromium), 2014/04/12 09:11:38]

My understanding is that LogFoo don't actually present any guarantees, as long
as they can be called from the (untrusted) renderer.

If I understand you correctly, you suggest passing a StringID for a label and a
string for the data through the IPC?
If so, then it will be hardly possible to make sure on the browser side, that,
e.g., a password or a sensitive URL is not included in the string.

[Ilya Sherman, 2014/04/12 22:25:31]

I would keep the previous IPC design, i.e. just send a string.  However, I would
keep the StringID changes for the LogFoo() methods in this class.

With this design, it's not possible to write code that accidentally emits a
password or a sensitive URL, other than by going through some contortions to,
say, pass the password as the name_or_id param to LogHTMLForm().  That is,
there's no chance that I as a developer might add code to log a password, unless
I really abuse the API... but if I'm really abusing the API, a code reviewer
would likely catch that.  LogMessage() previously did not have this property --
I could easily build up a complex string and pass it to LogMessage(), which
would be harder for a reviewer to reason about -- did everything get sanitized
properly or not?

Now, in terms of a compromised renderer: First of all, if you've compromised the
renderer, your next step probably isn't going to be to ask the user to enable
logging, and then try to send across a password or a sensitive URL over IPC. 
Even if that is your goal, you can still do it, by logging the sensitive data as
a "name_or_id".

In other words, I think the threat model for sending sensitive data from the
renderer to the browser is coding mistakes, not malicious websites.  For that
threat model, we can keep the cleaner option.  (Of course, Tom is much more
expert in this area than I am, so perhaps I am overlooking something that he
sees.  Corrections are definitely welcome.)

On the browser side, it's still very important to sanitize the received data by
escaping it (as you've done now), because there is a much higher incentive for a
malicious website to try to run arbitrary code in a chrome:// page than there is
to reveal a user's password.

[vabr (Chromium), 2014/04/16 20:55:36]

Done.

 
   SavePasswordProgressLogger();
   virtual ~SavePasswordProgressLogger();
 
-  // Logging: specialized methods (for logging forms, URLs, etc.) take care of
-  // proper removing of sensitive data where appropriate.
-  void LogPasswordForm(const std::string& message,
+  // Call SanitizeStructuredLogs only in the browser, to get a safe-for-display
+  // string representation of |logs|. The vector |logs| needs to be organised in
+  // one of the two following ways:
+  // 1) there is only one log in the vector, of type different than
+  //    LOG_TYPE_LABEL, or
+  // 2) there are more logs, and exactly the first one has type LOG_TYPE_LABEL,
+  //    providing a label for the whole group.
+  static std::string SanitizeStructuredLogs(
+      const std::vector<StructuredLog>& logs);

[Ilya Sherman, 2014/04/12 00:00:51]

How about only making this function available in the browser process?

[vabr (Chromium), 2014/04/16 20:55:36]

That function is gone.

+
+  // Logging: these methods create and send a StructuredLog for sanitization and
+  // display.
+  void LogPasswordForm(StringID label,
                        const autofill::PasswordForm& form);
-  void LogHTMLForm(const std::string& message,
+  void LogHTMLForm(StringID label,
                    const std::string& name_or_id,
                    const std::string& method,
                    const GURL& action);
-  void LogURL(const std::string& message, const GURL& url);
-  void LogBoolean(const std::string& message, bool value);
-  void LogNumber(const std::string& message, int value);
-  void LogNumber(const std::string& message, size_t value);
-  void LogFinalDecision(Decision decision);
-  // Do not use LogMessage when there is an appropriate specialized method
-  // above. LogMessage performs no scrubbing of sensitive data.
-  void LogMessage(const std::string& message);
+  void LogURL(StringID label, const GURL& url);
+  void LogBoolean(StringID label, bool value);
+  void LogNumber(StringID label, int value);
+  void LogNumber(StringID label, size_t value);
+  void LogMessage(StringID message);
 
  protected:
-  // Sends |log| immediately for display.
-  virtual void SendLog(const std::string& log) = 0;
+  // Sends |logs| immediately for display.
+  virtual void SendLog(const std::vector<StructuredLog>& logs) = 0;
 
  private:
-  // Takes a structured |log|, converts it to a string suitable for plain text
-  // output, adds the |name| as a caption, and sends out via SendLog.
-  void LogValue(const std::string& name, const base::Value& log);
-
   DISALLOW_COPY_AND_ASSIGN(SavePasswordProgressLogger);
 };