Enforce httponly on cookies coming from the renderer. This prevents javascript from setting a new httponly cookie, and more importantly from overwriting httponly cookies.

Patch from Marius Schilder.
Review URL: http://codereview.chromium.org/11275

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5700 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 132 insertions, 36 deletions

diff --git a/net/base/cookie_monster.cc b/net/base/cookie_monster.cc
index adfd19b..5b5f572 100644
--- a/net/base/cookie_monster.cc
+++ b/net/base/cookie_monster.cc
@@ -379,14 +379,36 @@ static bool HasCookieableScheme(const GURL& url) {
 
 bool CookieMonster::SetCookie(const GURL& url,
                               const std::string& cookie_line) {
+  CookieOptions options;
+  return SetCookieWithOptions(url, cookie_line, options);
+}
+
+bool CookieMonster::SetCookieWithOptions(const GURL& url,
+                                         const std::string& cookie_line,
+                                         const CookieOptions& options) {
   Time creation_date = CurrentTime();
   last_time_seen_ = creation_date;
-  return SetCookieWithCreationTime(url, cookie_line, creation_date);
+  return SetCookieWithCreationTimeWithOptions(url,
+                                              cookie_line,
+                                              creation_date,
+                                              options);
 }
 
 bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
                                               const std::string& cookie_line,
                                               const Time& creation_time) {
+  CookieOptions options;
+  return SetCookieWithCreationTimeWithOptions(url,
+                                              cookie_line,
+                                              creation_time,
+                                              options);
+}
+
+bool CookieMonster::SetCookieWithCreationTimeWithOptions(
+                                              const GURL& url,
+                                              const std::string& cookie_line,
+                                              const Time& creation_time,
+                                              const CookieOptions& options) {
   DCHECK(!creation_time.is_null());
 
   if (!HasCookieableScheme(url)) {
@@ -407,6 +429,11 @@ bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
     return false;
   }
 
+  if (options.exclude_httponly() && pc.IsHttpOnly()) {
+    COOKIE_DLOG(INFO) << "SetCookie() not setting httponly cookie";
+    return false;
+  }
+
   std::string cookie_domain;
   if (!GetCookieDomainKey(url, pc, &cookie_domain)) {
     return false;
@@ -427,7 +454,12 @@ bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
     return false;
   }
 
-  DeleteAnyEquivalentCookie(cookie_domain, *cc);
+  if (DeleteAnyEquivalentCookie(cookie_domain,
+                                *cc,
+                                options.exclude_httponly())) {
+    COOKIE_DLOG(INFO) << "SetCookie() not clobbering httponly cookie";
+    return false;
+  }
 
   COOKIE_DLOG(INFO) << "SetCookie() cc: " << cc->DebugString();
 
@@ -448,9 +480,17 @@ bool CookieMonster::SetCookieWithCreationTime(const GURL& url,
 
 void CookieMonster::SetCookies(const GURL& url,
                                const std::vector<std::string>& cookies) {
+  CookieOptions options;
+  SetCookiesWithOptions(url, cookies, options);
+}
+
+void CookieMonster::SetCookiesWithOptions(
+    const GURL& url,
+    const std::vector<std::string>& cookies,
+    const CookieOptions& options) {
   for (std::vector<std::string>::const_iterator iter = cookies.begin();
        iter != cookies.end(); ++iter)
-    SetCookie(url, *iter);
+    SetCookieWithOptions(url, *iter, options);
 }
 
 void CookieMonster::InternalInsertCookie(const std::string& key,
@@ -485,9 +525,11 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
   delete cc;
 }
 
-void CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
-                                              const CanonicalCookie& ecc) {
+bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
+                                              const CanonicalCookie& ecc,
+                                              bool skip_httponly) {
   bool found_equivalent_cookie = false;
+  bool skipped_httponly = false;
   for (CookieMapItPair its = cookies_.equal_range(key);
        its.first != its.second; ) {
     CookieMap::iterator curit = its.first;
@@ -499,7 +541,11 @@ void CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
       // overwrite each other.
       DCHECK(!found_equivalent_cookie) <<
           "Duplicate equivalent cookies found, cookie store is corrupted.";
-      InternalDeleteCookie(curit, true);
+      if (skip_httponly && cc->IsHttpOnly()) {
+        skipped_httponly = true;
+      } else {
+        InternalDeleteCookie(curit, true);
+      }
       found_equivalent_cookie = true;
 #ifdef NDEBUG
       // Speed optimization: No point looping through the rest of the cookies
@@ -508,6 +554,7 @@ void CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
 #endif
     }
   }
+  return skipped_httponly;
 }
 
 int CookieMonster::GarbageCollect(const Time& current,
@@ -659,10 +706,6 @@ static bool CookieSorter(CookieMonster::CanonicalCookie* cc1,
   return cc1->Path().length() > cc2->Path().length();
 }
 
-std::string CookieMonster::GetCookies(const GURL& url) {
-  return GetCookiesWithOptions(url, NORMAL);
-}
-
 // Currently our cookie datastructure is based on Mozilla's approach.  We have a
 // hash keyed on the cookie's domain, and for any query we walk down the domain
 // components and probe for cookies until we reach the TLD, where we stop.
@@ -675,8 +718,13 @@ std::string CookieMonster::GetCookies(const GURL& url) {
 // search/prefix trie, where we reverse the hostname and query for all
 // keys that are a prefix of our hostname.  I think the hash probing
 // should be fast and simple enough for now.
+std::string CookieMonster::GetCookies(const GURL& url) {
+  CookieOptions options;
+  return GetCookiesWithOptions(url, options);
+}
+
 std::string CookieMonster::GetCookiesWithOptions(const GURL& url,
-                                                 CookieOptions options) {
+                                                 const CookieOptions& options) {
   if (!HasCookieableScheme(url)) {
     DLOG(WARNING) << "Unsupported cookie scheme: " << url.scheme();
     return std::string();
@@ -730,7 +778,7 @@ CookieMonster::CookieList CookieMonster::GetAllCookies() {
 
 void CookieMonster::FindCookiesForHostAndDomain(
     const GURL& url,
-    CookieOptions options,
+    const CookieOptions& options,
     std::vector<CanonicalCookie*>* cookies) {
   AutoLock autolock(lock_);
   InitIfNecessary();
@@ -765,7 +813,7 @@ void CookieMonster::FindCookiesForHostAndDomain(
 void CookieMonster::FindCookiesForKey(
     const std::string& key,
     const GURL& url,
-    CookieOptions options,
+    const CookieOptions& options,
     const Time& current,
     std::vector<CanonicalCookie*>* cookies) {
   bool secure = url.SchemeIsSecure();
@@ -782,8 +830,8 @@ void CookieMonster::FindCookiesForKey(
       continue;
     }
 
-    // Filter out HttpOnly cookies unless they where explicitly requested.
-    if ((options & INCLUDE_HTTPONLY) == 0 && cc->IsHttpOnly())
+    // Filter out HttpOnly cookies, per options.
+    if (options.exclude_httponly() && cc->IsHttpOnly())
       continue;
 
     // Filter out secure cookies unless we're https.
diff --git a/net/base/cookie_monster.h b/net/base/cookie_monster.h
index 3a94b9b..fef0e8f 100644
--- a/net/base/cookie_monster.h
+++ b/net/base/cookie_monster.h
@@ -49,12 +49,17 @@ class CookieMonster {
   typedef std::pair<std::string, CanonicalCookie> CookieListPair;
   typedef std::vector<CookieListPair> CookieList;
 
-  enum CookieOptions {
-    // Normal cookie behavior, decides which cookies to return based on
-    // the URL and whether it's https, etc.  Never returns HttpOnly cookies
-    NORMAL = 0,
-    // Include HttpOnly cookies
-    INCLUDE_HTTPONLY = 1,
+  class CookieOptions {
+   public:
+    // Default is to exclude httponly, which means:
+    // - reading operations will not return httponly cookies.
+    // - writing operations will not write httponly cookies. 
+    CookieOptions() : exclude_httponly_(true) {}
+    void set_exclude_httponly() { exclude_httponly_ = true; }
+    void set_include_httponly() { exclude_httponly_ = false; }
+    bool exclude_httponly() const { return exclude_httponly_; }
+   private:
+    bool exclude_httponly_;
   };
 
   CookieMonster();
@@ -81,21 +86,33 @@ class CookieMonster {
 
   // Set a single cookie.  Expects a cookie line, like "a=1; domain=b.com".
   bool SetCookie(const GURL& url, const std::string& cookie_line);
+  bool SetCookieWithOptions(const GURL& url,
+                            const std::string& cookie_line,
+                            const CookieOptions& options);
   // Sets a single cookie with a specific creation date. To set a cookie with
   // a creation date of Now() use SetCookie() instead (it calls this function
   // internally).
   bool SetCookieWithCreationTime(const GURL& url,
                                  const std::string& cookie_line,
                                  const base::Time& creation_time);
+  bool SetCookieWithCreationTimeWithOptions(
+                                 const GURL& url,
+                                 const std::string& cookie_line,
+                                 const base::Time& creation_time,
+                                 const CookieOptions& options);
   // Set a vector of response cookie values for the same URL.
   void SetCookies(const GURL& url, const std::vector<std::string>& cookies);
+  void SetCookiesWithOptions(const GURL& url,
+                             const std::vector<std::string>& cookies,
+                             const CookieOptions& options);
 
   // TODO what if the total size of all the cookies >4k, can we have a header
   // that big or do we need multiple Cookie: headers?
   // Simple interface, get a cookie string "a=b; c=d" for the given URL.
-  // It will _not_ return httponly cookies, see GetCookiesWithOptions
+  // It will _not_ return httponly cookies, see CookieOptions.
   std::string GetCookies(const GURL& url);
-  std::string GetCookiesWithOptions(const GURL& url, CookieOptions options);
+  std::string GetCookiesWithOptions(const GURL& url,
+                                    const CookieOptions& options);
   // Returns all the cookies, for use in management UI, etc.  This does not mark
   // the cookies as having been accessed.
   CookieList GetAllCookies();
@@ -140,17 +157,22 @@ class CookieMonster {
   void InitStore();
 
   void FindCookiesForHostAndDomain(const GURL& url,
-                                   CookieOptions options,
+                                   const CookieOptions& options,
                                    std::vector<CanonicalCookie*>* cookies);
 
   void FindCookiesForKey(const std::string& key,
                          const GURL& url,
-                         CookieOptions options,
+                         const CookieOptions& options,
                          const base::Time& current,
                          std::vector<CanonicalCookie*>* cookies);
 
-  void DeleteAnyEquivalentCookie(const std::string& key,
-                                 const CanonicalCookie& ecc);
+  // Delete any cookies that are equivalent to |ecc| (same path, key, etc).
+  // If |skip_httponly| is true, httponly cookies will not be deleted.  The
+  // return value with be true if |skip_httponly| skipped an httponly cookie.
+  // NOTE: There should never be more than a single matching equivalent cookie.
+  bool DeleteAnyEquivalentCookie(const std::string& key,
+                                 const CanonicalCookie& ecc,
+                                 bool skip_httponly);
 
   void InternalInsertCookie(const std::string& key,
                             CanonicalCookie* cc,
diff --git a/net/base/cookie_monster_unittest.cc b/net/base/cookie_monster_unittest.cc
index c3c64f3..fcf8dbc 100644
--- a/net/base/cookie_monster_unittest.cc
+++ b/net/base/cookie_monster_unittest.cc
@@ -468,10 +468,29 @@ TEST(CookieMonsterTest, PathTest) {
 TEST(CookieMonsterTest, HttpOnlyTest) {
   GURL url_google(kUrlGoogle);
   net::CookieMonster cm;
-  EXPECT_TRUE(cm.SetCookie(url_google, "A=B; httponly"));
+  net::CookieMonster::CookieOptions options;
+  options.set_include_httponly();
+
+  // Create a httponly cookie.
+  EXPECT_TRUE(cm.SetCookieWithOptions(url_google, "A=B; httponly", options));
+  
+  // Check httponly read protection.
+  EXPECT_EQ("", cm.GetCookies(url_google));
+  EXPECT_EQ("A=B", cm.GetCookiesWithOptions(url_google, options));
+
+  // Check httponly overwrite protection.
+  EXPECT_FALSE(cm.SetCookie(url_google, "A=C"));
   EXPECT_EQ("", cm.GetCookies(url_google));
-  EXPECT_EQ("A=B", cm.GetCookiesWithOptions(url_google,
-      net::CookieMonster::INCLUDE_HTTPONLY));
+  EXPECT_EQ("A=B", cm.GetCookiesWithOptions(url_google, options));
+  EXPECT_TRUE(cm.SetCookieWithOptions(url_google, "A=C", options));
+  EXPECT_EQ("A=C", cm.GetCookies(url_google));
+
+  // Check httponly create protection.
+  EXPECT_FALSE(cm.SetCookie(url_google, "B=A; httponly"));
+  EXPECT_EQ("A=C", cm.GetCookiesWithOptions(url_google, options));
+  EXPECT_TRUE(cm.SetCookieWithOptions(url_google, "B=A; httponly", options));
+  EXPECT_EQ("A=C; B=A", cm.GetCookiesWithOptions(url_google, options));
+  EXPECT_EQ("A=C", cm.GetCookies(url_google));
 }
 
 namespace {
@@ -614,15 +633,17 @@ TEST(CookieMonsterTest, TestCookieDeletion) {
 TEST(CookieMonsterTest, TestCookieDeleteAll) {
   GURL url_google(kUrlGoogle);
   net::CookieMonster cm;
+  net::CookieMonster::CookieOptions options;
+  options.set_include_httponly();
 
   EXPECT_TRUE(cm.SetCookie(url_google, kValidCookieLine));
   EXPECT_EQ("A=B", cm.GetCookies(url_google));
 
-  EXPECT_TRUE(cm.SetCookie(url_google, "C=D"));
-  EXPECT_EQ("A=B; C=D", cm.GetCookies(url_google));
+  EXPECT_TRUE(cm.SetCookieWithOptions(url_google, "C=D; httponly", options));
+  EXPECT_EQ("A=B; C=D", cm.GetCookiesWithOptions(url_google, options));
 
   EXPECT_EQ(2, cm.DeleteAll(false));
-  EXPECT_EQ("", cm.GetCookies(url_google));
+  EXPECT_EQ("", cm.GetCookiesWithOptions(url_google, options));
 }
 
 TEST(CookieMonsterTest, TestCookieDeleteAllCreatedAfterTimestamp) {
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 69f7bb5..eac02e1 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -414,7 +414,11 @@ void URLRequestHttpJob::NotifyHeadersComplete() {
         ctx->cookie_policy()->CanSetCookie(request_->url(),
                                            request_->policy_url())) {
       FetchResponseCookies();
-      ctx->cookie_store()->SetCookies(request_->url(), response_cookies_);
+      net::CookieMonster::CookieOptions options;
+      options.set_include_httponly();
+      ctx->cookie_store()->SetCookiesWithOptions(request_->url(),
+                                                 response_cookies_,
+                                                 options);
     }
   }
 
@@ -517,9 +521,10 @@ void URLRequestHttpJob::AddExtraHeaders() {
     if (context->cookie_store() &&
         context->cookie_policy()->CanGetCookies(request_->url(),
                                                request_->policy_url())) {
+      net::CookieMonster::CookieOptions options;
+      options.set_include_httponly();
       std::string cookies = request_->context()->cookie_store()->
-          GetCookiesWithOptions(request_->url(),
-                                net::CookieMonster::INCLUDE_HTTPONLY);
+          GetCookiesWithOptions(request_->url(), options);
       if (!cookies.empty())
         request_info_.extra_headers += "Cookie: " + cookies + "\r\n";
     }