Encrypt all stored cookies on selected operating systems.

content/browser/net/sqlite_persistent_cookie_store.cc

Index: content/browser/net/sqlite_persistent_cookie_store.cc
diff --git a/content/browser/net/sqlite_persistent_cookie_store.cc b/content/browser/net/sqlite_persistent_cookie_store.cc
index 517a8c18a1646d10af600c125a9e1b00d754d606..45879dcd78219d1c68b7747fe5faa6bdad6ea54a 100644
--- a/content/browser/net/sqlite_persistent_cookie_store.cc
+++ b/content/browser/net/sqlite_persistent_cookie_store.cc
@@ -26,6 +26,7 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
+#include "components/webdata/encryptor/encryptor.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
@@ -277,6 +278,9 @@ namespace {
 
 // Version number of the database.
 //
+// Version 7 adds encrypted values.  Old values will continue to be used but
+// all new values written will be encrypted on selected operating systems.
+//
 // Version 6 adds cookie priorities. This allows developers to influence the
 // order in which cookies are evicted in order to meet domain cookie limits.
 //
@@ -293,7 +297,7 @@ namespace {
 // Version 3 updated the database to include the last access time, so we can
 // expire them in decreasing order of use when we've reached the maximum
 // number of cookies.
-const int kCurrentVersionNumber = 6;
+const int kCurrentVersionNumber = 7;
 const int kCompatibleVersionNumber = 5;
 
 // Possible values for the 'priority' column.
@@ -363,6 +367,7 @@ bool InitTable(sql::Connection* db) {
             "host_key TEXT NOT NULL,"
             "name TEXT NOT NULL,"
             "value TEXT NOT NULL,"
+            "encrypted_value BLOB NOT NULL,"

[Scott Hess - ex-Googler, 2013/09/27 23:07:12]

Put the encrypted_value at the end so that the create-from-scratch and
version-migrated schema match.

[bcwhite, 2013/09/30 15:04:29]

Done.

             "path TEXT NOT NULL,"
             "expires_utc INTEGER NOT NULL,"
             "secure INTEGER NOT NULL,"
@@ -679,15 +684,16 @@ bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
   if (restore_old_session_cookies_) {
     smt.Assign(db_->GetCachedStatement(
         SQL_FROM_HERE,
-        "SELECT creation_utc, host_key, name, value, path, expires_utc, "
-        "secure, httponly, last_access_utc, has_expires, persistent, priority "
-        "FROM cookies WHERE host_key = ?"));
+        "SELECT creation_utc, host_key, name, value, encrypted_value, path, "
+        "expires_utc, secure, httponly, last_access_utc, has_expires, "
+        "persistent, priority FROM cookies WHERE host_key = ?"));
   } else {
     smt.Assign(db_->GetCachedStatement(
         SQL_FROM_HERE,
-        "SELECT creation_utc, host_key, name, value, path, expires_utc, "
-        "secure, httponly, last_access_utc, has_expires, persistent, priority "
-        "FROM cookies WHERE host_key = ? AND persistent = 1"));
+        "SELECT creation_utc, host_key, name, value, encrypted_value, path, "
+        "expires_utc, secure, httponly, last_access_utc, has_expires, "
+        "persistent, priority FROM cookies WHERE host_key = ? "
+        "AND persistent = 1"));
   }
   if (!smt.is_valid()) {
     smt.Clear();  // Disconnect smt_ref from db_.
@@ -701,20 +707,24 @@ bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
   for (; it != domains.end(); ++it) {
     smt.BindString(0, *it);
     while (smt.Step()) {
+      std::string value = smt.ColumnString(3);

[erikwright (departed), 2013/10/07 20:39:25]

I suppose it's possible that there's a hit for reading even an empty field.
Consider only calling ColumnString(3) if the encrypted value is empty.

[bcwhite, 2013/10/08 16:10:25]

Done.

[Scott Hess - ex-Googler, 2013/10/08 17:11:53]

The big hit is I/O, and how SQLite stores things means that you'll do the I/O
through the highest column accessed.  So this will only cost a bit of marshaling
overhead.

[bcwhite, 2013/10/08 18:23:36]

So...  Yes or no?  Personally I prefer the previous way of always extracting
value rather than have an "else" clause.

[Scott Hess - ex-Googler, 2013/10/08 19:13:51]

I don't care either way, so defer to Erik.

+      std::string encrypted_value = smt.ColumnString(4);
+      if (!encrypted_value.empty())
+        Encryptor::DecryptString(encrypted_value, &value);
       scoped_ptr<net::CanonicalCookie> cc(new net::CanonicalCookie(
           // The "source" URL is not used with persisted cookies.
           GURL(),                                         // Source
           smt.ColumnString(2),                            // name
-          smt.ColumnString(3),                            // value
+          value,                                          // value
           smt.ColumnString(1),                            // domain
-          smt.ColumnString(4),                            // path
+          smt.ColumnString(5),                            // path
           Time::FromInternalValue(smt.ColumnInt64(0)),    // creation_utc
-          Time::FromInternalValue(smt.ColumnInt64(5)),    // expires_utc
-          Time::FromInternalValue(smt.ColumnInt64(8)),    // last_access_utc
-          smt.ColumnInt(6) != 0,                          // secure
-          smt.ColumnInt(7) != 0,                          // httponly
+          Time::FromInternalValue(smt.ColumnInt64(6)),    // expires_utc
+          Time::FromInternalValue(smt.ColumnInt64(9)),    // last_access_utc
+          smt.ColumnInt(7) != 0,                          // secure
+          smt.ColumnInt(8) != 0,                          // httponly
           DBCookiePriorityToCookiePriority(
-              static_cast<DBCookiePriority>(smt.ColumnInt(11)))));  // priority
+              static_cast<DBCookiePriority>(smt.ColumnInt(12)))));  // priority
       DLOG_IF(WARNING,
               cc->CreationDate() > Time::Now()) << L"CreationDate too recent";
       cookies_per_origin_[CookieOrigin(cc->Domain(), cc->IsSecure())]++;
@@ -837,6 +847,27 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
                         base::TimeTicks::Now() - start_time);
   }
 
+  if (cur_version == 6) {
+    const base::TimeTicks start_time = base::TimeTicks::Now();
+    sql::Transaction transaction(db_.get());
+    if (!transaction.Begin())
+      return false;
+    // Alter the table to add empty "encrypted value" column.
+    std::string stmt(base::StringPrintf(
+        "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''"));
+    if (!db_->Execute(stmt.c_str())) {
+      LOG(WARNING) << "Unable to update cookie database to version 7.";
+      return false;
+    }
+    ++cur_version;
+    meta_table_.SetVersionNumber(cur_version);
+    meta_table_.SetCompatibleVersionNumber(
+        std::min(cur_version, kCompatibleVersionNumber));
+    transaction.Commit();
+    UMA_HISTOGRAM_TIMES("Cookie.TimeDatabaseMigrationToV7",
+                        base::TimeTicks::Now() - start_time);
+  }
+
   // Put future migration cases here.
 
   if (cur_version < kCurrentVersionNumber) {
@@ -921,10 +952,10 @@ void SQLitePersistentCookieStore::Backend::Commit() {
     return;
 
   sql::Statement add_smt(db_->GetCachedStatement(SQL_FROM_HERE,
-      "INSERT INTO cookies (creation_utc, host_key, name, value, path, "
-      "expires_utc, secure, httponly, last_access_utc, has_expires, "
-      "persistent, priority) "
-      "VALUES (?,?,?,?,?,?,?,?,?,?,?,?)"));
+      "INSERT INTO cookies (creation_utc, host_key, name, value, "
+      "encrypted_value, path, expires_utc, secure, httponly, last_access_utc, "
+      "has_expires, persistent, priority) "
+      "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)"));
   if (!add_smt.is_valid())
     return;
 
@@ -942,28 +973,38 @@ void SQLitePersistentCookieStore::Backend::Commit() {
   if (!transaction.Begin())
     return;
 
+  std::string value, encrypted_value;  // Only one will ever get a value below.

[Scott Hess - ex-Googler, 2013/09/27 23:07:12]

I would put this inside the loop.  Since the code always does exactly one or the
other, the value cannot leak between loop iterations, but that feels sketchy to
me.  The encryption itself is going to add way way more overhead anyhow.

That said, I think it would be even more obvious if you simply put the #if
defined around the column 3 and 4 bindings.

[bcwhite, 2013/09/30 15:04:29]

Sure.  I was trying to avoid re-constructing the objects each time but it's
hardly an issue.
Okay.  I'll have to create a sub-block to create the string there (because it's
inside a "case" and thus not necessarily initialized).  I thought it was easier
to read when put at the top.

   for (PendingOperationsList::iterator it = ops.begin();
        it != ops.end(); ++it) {
     // Free the cookies as we commit them to the database.
     scoped_ptr<PendingOperation> po(*it);
     switch (po->op()) {
       case PendingOperation::COOKIE_ADD:
+        // On operating systems that support an encryption API but don't
+        // automatically protect all user data, encrypt the cookie.
+#if defined(OS_WIN) || defined(OS_MACOSX)
+        Encryptor::EncryptString(po->cc().Value(), &encrypted_value);
+#else
+        value = po->cc().Value();
+#endif
         cookies_per_origin_[
             CookieOrigin(po->cc().Domain(), po->cc().IsSecure())]++;
         add_smt.Reset(true);
         add_smt.BindInt64(0, po->cc().CreationDate().ToInternalValue());
         add_smt.BindString(1, po->cc().Domain());
         add_smt.BindString(2, po->cc().Name());
-        add_smt.BindString(3, po->cc().Value());
-        add_smt.BindString(4, po->cc().Path());
-        add_smt.BindInt64(5, po->cc().ExpiryDate().ToInternalValue());
-        add_smt.BindInt(6, po->cc().IsSecure());
-        add_smt.BindInt(7, po->cc().IsHttpOnly());
-        add_smt.BindInt64(8, po->cc().LastAccessDate().ToInternalValue());
-        add_smt.BindInt(9, po->cc().IsPersistent());
+        add_smt.BindString(3, value);
+        add_smt.BindBlob(4, encrypted_value.data(),
+                         static_cast<int>(encrypted_value.length()));
+        add_smt.BindString(5, po->cc().Path());
+        add_smt.BindInt64(6, po->cc().ExpiryDate().ToInternalValue());
+        add_smt.BindInt(7, po->cc().IsSecure());
+        add_smt.BindInt(8, po->cc().IsHttpOnly());
+        add_smt.BindInt64(9, po->cc().LastAccessDate().ToInternalValue());
         add_smt.BindInt(10, po->cc().IsPersistent());
+        add_smt.BindInt(11, po->cc().IsPersistent());
         add_smt.BindInt(
-            11, CookiePriorityToDBCookiePriority(po->cc().Priority()));
+            12, CookiePriorityToDBCookiePriority(po->cc().Priority()));
         if (!add_smt.Run())
           NOTREACHED() << "Could not add a cookie to the DB.";
         break;