Encrypt all stored cookies on selected operating systems.

content/browser/net/sqlite_persistent_cookie_store.cc

 // Copyright (c) 2012 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 "content/browser/net/sqlite_persistent_cookie_store.h"
 
 #include <list>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 56 matching lines @@
 // disk on the BG runner every 30 seconds, 512 operations, or call to Flush(),
 // whichever occurs first.
 class SQLitePersistentCookieStore::Backend
     : public base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend> {
  public:
   Backend(
       const base::FilePath& path,
       const scoped_refptr<base::SequencedTaskRunner>& client_task_runner,
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
       bool restore_old_session_cookies,
-      quota::SpecialStoragePolicy* special_storage_policy)
+      quota::SpecialStoragePolicy* special_storage_policy,
+      CookieCryptoDelegate* crypto_delegate)
       : path_(path),
         num_pending_(0),
         force_keep_session_state_(false),
         initialized_(false),
         corruption_detected_(false),
         restore_old_session_cookies_(restore_old_session_cookies),
         special_storage_policy_(special_storage_policy),
         num_cookies_read_(0),
         client_task_runner_(client_task_runner),
         background_task_runner_(background_task_runner),
         num_priority_waiting_(0),
-        total_priority_requests_(0) {}
+        total_priority_requests_(0),
+        crypto_(crypto_delegate) {}
 
   // Creates or loads the SQLite database.
   void Load(const LoadedCallback& loaded_callback);
 
   // Loads cookies for the domain key (eTLD+1).
   void LoadCookiesForKey(const std::string& domain,
       const LoadedCallback& loaded_callback);
 
   // Batch a cookie addition.
   void AddCookie(const net::CanonicalCookie& cc);
@@ skipping 162 matching lines @@
   // starting/completing priority loads or completing the total load).
   base::Lock metrics_lock_;
   int num_priority_waiting_;
   // The total number of priority requests.
   int total_priority_requests_;
   // The time when |num_priority_waiting_| incremented to 1.
   base::Time current_priority_wait_start_;
   // The cumulative duration of time when |num_priority_waiting_| was greater
   // than 1.
   base::TimeDelta priority_wait_duration_;
+  // Class with functions that do cryptographic operations (for protecting
+  // cookies stored persistently).
+  scoped_ptr<CookieCryptoDelegate> crypto_;
 
   DISALLOW_COPY_AND_ASSIGN(Backend);
 };
 
 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.
 //
 // Version 5 adds the columns has_expires and is_persistent, so that the
 // database can store session cookies as well as persistent cookies. Databases
 // of version 5 are incompatible with older versions of code. If a database of
 // version 5 is read by older code, session cookies will be treated as normal
 // cookies. Currently, these fields are written, but not read anymore.
 //
 // In version 4, we migrated the time epoch.  If you open the DB with an older
 // version on Mac or Linux, the times will look wonky, but the file will likely
 // be usable. On Windows version 3 and 4 are the same.
 //
 // 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;

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

This needs to be 7. Some of the cookies will have empty value fields, which
means it's not backwards compatible.

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

If an encrypted value was written, the cookie "value" will just be empty but in
general the system will continue to work even on a down-grade.  Also, cookies
that haven't updated to encrypted versions would still have valid values.  The
system will work at least as well as razing the DB and starting over; I'd call
that "backward compatible".

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

If the database compatible version is too new, I believe we just leave it alone
and go with a clean database which is not persisted.  The user will eventually
catch up and things will start working again.  Not a great outcome, but not
horrible.

Also consider whether older code will write correct data for newer code.  For
instance, as presently defined, older code will fail on inserts because it won't
be providing an encrypted_value column.  As noted below, I think your CREATE
actually needs to change, so that won't be a problem.

OK, so to successfully work you need to have:
 - older code statements can successfully operate on newer schema.
 - older code can successfully interpret the data written by the newer code.
 - older code writes data which works with the newer code without migration.

I think it is possible to have compatible version of 5, but I'm not sure how
much effort I'd put into the issue.  I don't think we have any data on how often
the compatible-version check fails, unfortunately.

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

Yes, older code writes data that is correct for newer code in that records with
a "value" but not an "encrypted_value" are handled correctly.

However, you're correct that if the database was created with the new version
and thus has the "not null" clause for "encrypted_value" then in fact old code
will not be able to write to the new database.

V6 of the database has a column "priority integer default 1" when upgraded but
"priority integer not null default 1" when created from scratch.  In the latter
case, wouldn't in be not backward compatible with V5 because of the same "not
null" clause or does the "default" setting make it okay?  If so, I can add a
default to newly created tables with "encrypted_value" to make it work.

[erikwright (departed), 2013/10/08 18:41:52]

Either way, I'm not sure that it is a good idea to have older code reading the
new cookies with (apparently) empty values. This could produce arbitrary,
unexpected results from web apps that will suddenly receive cookies that they
set but without values.

Having the users end up with an in-memory, not persistent cookie store is not
great either, mind you.

[bcwhite, 2013/10/08 20:18:09]

A down-grade would mean sending empty cookie values but would be able to operate
correctly with those cookies that hadn't been updated and thus encrypted.  Six
of one, a half-dozen of the other.

 
 // Possible values for the 'priority' column.
 enum DBCookiePriority {
   kCookiePriorityLow = 0,
   kCookiePriorityMedium = 1,
   kCookiePriorityHigh = 2,
 };
 
 DBCookiePriority CookiePriorityToDBCookiePriority(net::CookiePriority value) {
   switch (value) {
@@ skipping 52 matching lines @@
     std::string stmt(base::StringPrintf(
         "CREATE TABLE cookies ("
             "creation_utc INTEGER NOT NULL UNIQUE PRIMARY KEY,"
             "host_key TEXT NOT NULL,"
             "name TEXT NOT NULL,"
             "value TEXT NOT NULL,"
             "path TEXT NOT NULL,"
             "expires_utc INTEGER NOT NULL,"
             "secure INTEGER NOT NULL,"
             "httponly INTEGER NOT NULL,"
-            "last_access_utc INTEGER NOT NULL, "
-            "has_expires INTEGER NOT NULL DEFAULT 1, "
+            "last_access_utc INTEGER NOT NULL,"
+            "has_expires INTEGER NOT NULL DEFAULT 1,"
             "persistent INTEGER NOT NULL DEFAULT 1,"
-            "priority INTEGER NOT NULL DEFAULT %d)",
+            "priority INTEGER NOT NULL DEFAULT %d,"
+            "encrypted_value BLOB NOT NULL)",

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

This will result in different schema depending on when a user installed Chrome. 
Make this path match the ALTER path.

Also, the whitespace is embedded in the schema in some cases, so I would prefer
not to remove the whitespace.

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

It never will.  The existing upgrades add columns without the "not null" clause
as does the one I've added.  I don't think this can be avoided.
Done.  I guess I just made it internally consistent right here without even
thinking.

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

Unfortunately, I didn't review those, or didn't catch it in review.  So this
case can be better.  [Also, changing things now means that people who came in at
v6 and migrated to v7 will match people who came in at v7.  Earlier upgrades
wouldn't apply.]

The goal of this is that a substantive change like this has the chance of
working differently depending on which upgrade path you came through.  So after
two or three upgrades, it get challenging to reason about the schema any longer.
 Better to just have them match, warts and all, then you have more confidence
that everything works the same.
Periodically I'll be doing a fixup which involves reformatting, and
inconsistencies in spacing become terribly obvious.  It's frustrating to me!

[bcwhite, 2013/10/08 20:18:09]

So... to be sure I understand what you're saying...

You'd rather lose the "not null" (and add "default ''") for "encrypted_value"
and be different here from all other fields in order to have the same definition
of the field regardless of whether it came from a newly created table or an
upgraded table.

Is that correct?

[Scott Hess - ex-Googler, 2013/10/08 21:22:12]

Yes.  I want the semantics of the field you are adding to be the same regardless
of whether it is a fresh database or a migrated database.  The code cannot rely
on SQLite enforcing the NOT NULL, so it should not be implied that the code can
rely on it.

[bcwhite, 2013/10/10 17:19:30]

Done.  I just wanted to be sure I understood your request.

         CookiePriorityToDBCookiePriority(net::COOKIE_PRIORITY_DEFAULT)));
     if (!db->Execute(stmt.c_str()))
       return false;
   }
 
   // Older code created an index on creation_utc, which is already
   // primary key for the table.
   if (!db->Execute("DROP INDEX IF EXISTS cookie_times"))
     return false;
 
@@ skipping 288 matching lines @@
 }
 
 bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
   const std::set<std::string>& domains) {
   DCHECK(background_task_runner_->RunsTasksOnCurrentThread());
 
   sql::Statement smt;
   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_.
     meta_table_.Reset();
     db_.reset();
     return false;
   }
 
   std::vector<net::CanonicalCookie*> cookies;
   std::set<std::string>::const_iterator it = domains.begin();
   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]

How about accessing smt.ColumnString(3) only if encrypted_value is not empty?

[bcwhite, 2013/10/08 20:18:09]

Done.

+      std::string encrypted_value = smt.ColumnString(4);
+      if (!encrypted_value.empty() && crypto_.get())

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

I think it's a serious error condition if there is an encrypted value and
crypto_ is NULL.

Should probably DCHECK. In production, the least harmful thing to do would be to
read it into memory (with a blank value) but never send it, but I think that
reading it into memory (with a blank value) and sending it is much easier and
not that much worse.

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

Done.

+        crypto_->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())]++;
       cookies.push_back(cc.release());
       ++num_cookies_read_;
     }
     smt.Reset(true);
   }
   {
     base::AutoLock locked(lock_);
@@ skipping 102 matching lines @@
     }
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
     meta_table_.SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
     transaction.Commit();
     UMA_HISTOGRAM_TIMES("Cookie.TimeDatabaseMigrationToV6",
                         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 ''"));

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

IIUC the migration path makes this nullable with default '' while the new path
makes it not null? Perhaps after creating the column and giving all rows a value
we could alter it to make it not nullable?

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

The other upgrade blocks don't include "not null" when specifying the now field.
 I assume this is because all the new entries would have to be NULL.  And like
the other upgrades, there's never any explicit update to all rows to assign a
value.  We'd never know when that condition was reached.

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

ALTER...ADD changes the schema info, but does not modify the table data.  SQLite
implicitly assumes NULL for items where the query ran off the end of the present
data.  Then DEFAULT is interpreted on top of that.  ALTER knows all of this
implementation detail, you cannot use ALTER...ADD to add a column with a NOT
NULL.

Unfortunately, ALTER cannot change the definition of an existing column.  So
it's not possible to later change it to be NOT NULL.  If desired you could
probably deny new NULL data using a TRIGGER, with DEFAULT handling existing rows
which don't contain the data.

I think it might also be reasonable to just allow the NULL, and use
ColumnType(i) == sql::COLUMN_TYPE_NULL to test it.  Is it guaranteed that this
column could never be a valid empty value?  Though I guess in that case it would
fallback to the other column, which would also be empty.  If that scenario is
valid, it might be worth having a test to prevent problems if someone refactors
later.

+    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) {
     UMA_HISTOGRAM_COUNTS_100("Cookie.CorruptMetaTable", 1);
 
     meta_table_.Reset();
     db_.reset(new sql::Connection);
     if (!base::DeleteFile(path_, false) ||
         !db_->Open(path_) ||
         !meta_table_.Init(
@@ skipping 64 matching lines @@
     base::AutoLock locked(lock_);
     pending_.swap(ops);
     num_pending_ = 0;
   }
 
   // Maybe an old timer fired or we are already Close()'ed.
   if (!db_.get() || ops.empty())
     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;
 
   sql::Statement update_access_smt(db_->GetCachedStatement(SQL_FROM_HERE,
       "UPDATE cookies SET last_access_utc=? WHERE creation_utc=?"));
   if (!update_access_smt.is_valid())
     return;
 
   sql::Statement del_smt(db_->GetCachedStatement(SQL_FROM_HERE,
                          "DELETE FROM cookies WHERE creation_utc=?"));
   if (!del_smt.is_valid())
     return;
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return;
 
   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:
         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());
+        if (crypto_.get()) {
+          std::string encrypted_value;
+          add_smt.BindString(3, std::string());  // value
+          crypto_->EncryptString(po->cc().Value(), &encrypted_value);
+          add_smt.BindBlob(4, encrypted_value.data(),

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

This looks like it's refering to memory that will be subsequently freed when
encrypted_value goes off the stack. If that's not the case (because BindBlob
copies the value) please add a comment to that effect.

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

Excellent catch!  I missed that when I moved the variable inside the "for" loop
at Scott's suggestion.

Done.

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

BindBlob() uses SQLITE_TRANSIENT, which means SQLite will make a copy of the
data.

+                           static_cast<int>(encrypted_value.length()));
+        } else {
+          add_smt.BindString(3, po->cc().Value());
+          add_smt.BindBlob(4, "", 0);  // encrypted_value

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

Presumably NULL could be used in place of '\0' since it is a 0-length blob?

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

Doing this will try to set the value of the field to NULL but the create
statement explicitly says "not null".  A "check failure" results.

+        }
+        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;
 
       case PendingOperation::COOKIE_UPDATEACCESS:
         update_access_smt.Reset(true);
         update_access_smt.BindInt64(0,
             po->cc().LastAccessDate().ToInternalValue());
         update_access_smt.BindInt64(1,
             po->cc().CreationDate().ToInternalValue());
@@ skipping 165 matching lines @@
     LOG(WARNING) << "Failed to post task from " << origin.ToString()
                  << " to client_task_runner_.";
   }
 }
 
 SQLitePersistentCookieStore::SQLitePersistentCookieStore(
     const base::FilePath& path,
     const scoped_refptr<base::SequencedTaskRunner>& client_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
     bool restore_old_session_cookies,
-    quota::SpecialStoragePolicy* special_storage_policy)
+    quota::SpecialStoragePolicy* special_storage_policy,
+    CookieCryptoDelegate* crypto_delegate)
     : backend_(new Backend(path,
                            client_task_runner,
                            background_task_runner,
                            restore_old_session_cookies,
-                           special_storage_policy)) {
+                           special_storage_policy,
+                           crypto_delegate)) {
 }
 
 void SQLitePersistentCookieStore::Load(const LoadedCallback& loaded_callback) {
   backend_->Load(loaded_callback);
 }
 
 void SQLitePersistentCookieStore::LoadCookiesForKey(
     const std::string& key,
     const LoadedCallback& loaded_callback) {
   backend_->LoadCookiesForKey(key, loaded_callback);
@@ skipping 24 matching lines @@
   backend_->Close();
   // We release our reference to the Backend, though it will probably still have
   // a reference if the background runner has not run Close() yet.
 }
 
 net::CookieStore* CreatePersistentCookieStore(
     const base::FilePath& path,
     bool restore_old_session_cookies,
     quota::SpecialStoragePolicy* storage_policy,
     net::CookieMonster::Delegate* cookie_monster_delegate,
-    const scoped_refptr<base::SequencedTaskRunner>& background_task_runner) {
+    const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
+    CookieCryptoDelegate* crypto_delegate) {
   SQLitePersistentCookieStore* persistent_store =
       new SQLitePersistentCookieStore(
           path,
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
           background_task_runner.get() ? background_task_runner :
               BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
                   BrowserThread::GetBlockingPool()->GetSequenceToken()),
           restore_old_session_cookies,
-          storage_policy);
+          storage_policy,
+          crypto_delegate);
   net::CookieMonster* cookie_monster =
       new net::CookieMonster(persistent_store, cookie_monster_delegate);
 
   const std::string cookie_priority_experiment_group =
       base::FieldTrialList::FindFullName("CookieRetentionPriorityStudy");
   cookie_monster->SetPriorityAwareGarbageCollection(
       cookie_priority_experiment_group == "ExperimentOn");
 
   return cookie_monster;
 }
 
 }  // namespace content