Encrypt all stored cookies on selected operating systems.

chrome/browser/profiles/profile_impl_io_data.cc

Index: chrome/browser/profiles/profile_impl_io_data.cc
diff --git a/chrome/browser/profiles/profile_impl_io_data.cc b/chrome/browser/profiles/profile_impl_io_data.cc
index df566656e3043fbe4256fa0657eabf19f3147a72..616632107ac4696c9b607fddcec698358895e49c 100644
--- a/chrome/browser/profiles/profile_impl_io_data.cc
+++ b/chrome/browser/profiles/profile_impl_io_data.cc
@@ -31,7 +31,9 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "components/webdata/encryptor/encryptor.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/cookie_crypto_delegate.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_context.h"
@@ -49,8 +51,35 @@
 #include "chrome/app/android/chrome_data_reduction_proxy_android.h"
 #endif
 
+// 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) || defined(OS_LINUX)
+#define CRYPT_COOKIES
+#endif
+
 namespace {
 
+#if defined(CRYPT_COOKIES)
+class CookieOSCryptoDelegate : public content::CookieCryptoDelegate {
+ public:
+  virtual bool EncryptString(const std::string& plaintext,
+                             std::string* ciphertext) OVERRIDE;
+  virtual bool DecryptString(const std::string& ciphertext,
+                             std::string* plaintext) OVERRIDE;
+};
+
+bool CookieOSCryptoDelegate::EncryptString(const std::string& plaintext,
+                                           std::string* ciphertext) {
+  return Encryptor::EncryptString(plaintext, ciphertext);
+}
+
+bool CookieOSCryptoDelegate::DecryptString(const std::string& ciphertext,
+                                           std::string* plaintext) {
+  return Encryptor::DecryptString(ciphertext, plaintext);
+}
+#endif  // CRYPT_COOKIES
+
+
 net::BackendType ChooseCacheBackendType() {
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   if (command_line.HasSwitch(switches::kUseSimpleCacheBackend)) {
@@ -394,7 +423,13 @@ void ProfileImplIOData::InitializeInternal(
         lazy_params_->restore_old_session_cookies,
         lazy_params_->special_storage_policy.get(),
         profile_params->cookie_monster_delegate.get(),
-        scoped_refptr<base::SequencedTaskRunner>());
+        scoped_refptr<base::SequencedTaskRunner>(),
+#if defined(CRYPT_COOKIES)
+        new CookieOSCryptoDelegate
+#else
+        NULL
+#endif
+    );

[Scott Hess - ex-Googler, 2013/10/08 17:35:55]

Definitely defer to Erik on this, but ...

Would it make sense to have CookieOSCryptoDelegate::Create(), and have it handle
returning NULL if the OS doesn't support it?

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

All OS support it.  It's only some OS that it's necessary.  And actually, Linux
doesn't support it but we do call it in order to be consistent with how OAuth
tokens are protected.

But the condition can certainly be moved there, perhaps "::IfDesired()" instead
of "::Create()"?

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

It makes me a bit ill, but maybe "MaybeCreate()"?

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

I don't feel strongly about it.  Repeated #ifdef and contorting the call to
accomodate is just a bit of a red flag.  Heaven forbid we ever add an additional
parameter to CreatePersistentCookieStore().

Actually, since it's in the anonymous namespace, it doesn't need to be a static
method, it could just be a helper function like CreateCryptoDelegateIfNeeded(). 
Though IMHO "IfNeeded" implies a level of choice which really isn't present
here, like a given Chrome build might sometimes return one, sometimes not.  It's
more "IfSupported", which seems implied (if you return one, it was supported).

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

It makes sense just to get rid of the repeated #ifdef.

However, I just wanted to note that it is supported on all platforms.  It's just
not necessary on some -- ChromeOS, for example, encrypts the entire profile
directory so also-encrypting the cookies adds no benefit.

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

Except to make the code more convoluted!

I am not questioning your reasoning, but the comment about the CRYPT_COOKIES
definition does not make me think "This is supported everywhere, but is
unnecessary on ChromeOS".  It makes me think "Some platforms support this, some
do not."

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

Comment improved.

     cookie_store->GetCookieMonster()->SetPersistSessionCookies(true);
   }
 
@@ -490,7 +525,13 @@ void ProfileImplIOData::
           lazy_params_->restore_old_session_cookies,
           NULL,
           NULL,
-          scoped_refptr<base::SequencedTaskRunner>());
+          scoped_refptr<base::SequencedTaskRunner>(),
+#if defined(CRYPT_COOKIES)
+          new CookieOSCryptoDelegate
+#else
+          NULL
+#endif
+      );
   // Enable cookies for devtools and extension URLs.
   const char* schemes[] = {chrome::kChromeDevToolsScheme,
                            extensions::kExtensionScheme};
@@ -577,7 +618,13 @@ ProfileImplIOData::InitializeAppRequestContext(
         false,
         NULL,
         NULL,
-        scoped_refptr<base::SequencedTaskRunner>());
+        scoped_refptr<base::SequencedTaskRunner>(),
+#if defined(CRYPT_COOKIES)
+        new CookieOSCryptoDelegate
+#else
+        NULL
+#endif
+    );
   }
 
   // Transfer ownership of the cookies and cache to AppRequestContext.