Title: [118105] trunk
Revision
118105
Author
[email protected]
Date
2012-05-22 19:49:58 -0700 (Tue, 22 May 2012)

Log Message

[BlackBerry] Possible to clobber httponly cookie.
https://bugs.webkit.org/show_bug.cgi?id=86067

Patch by Jason Liu <[email protected]> on 2012-05-22
Reviewed by Rob Buis.

Source/WebCore:

If a cookie is set by _javascript_ and there is already a same httpOnly cookie in cookieManager,
we should reject it. If it has a httpOnly property, we reject it, too.

Test: http/tests/cookies/js-get-and-set-http-only-cookie.php

* platform/blackberry/CookieJarBlackBerry.cpp:
(WebCore::setCookies):
* platform/blackberry/CookieManager.cpp:
(WebCore::CookieManager::setCookies):
(WebCore::CookieManager::shouldRejectNotHttpCookie):
(WebCore):
* platform/blackberry/CookieManager.h:

LayoutTests:

* http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added.
* http/tests/cookies/js-get-and-set-http-only-cookie.php: Added.
* http/tests/cookies/script-tests/js-get-and-set-http-only-cookie.js: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (118104 => 118105)


--- trunk/LayoutTests/ChangeLog	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/LayoutTests/ChangeLog	2012-05-23 02:49:58 UTC (rev 118105)
@@ -1,3 +1,14 @@
+2012-05-22  Jason Liu  <[email protected]>
+
+        [BlackBerry] Possible to clobber httponly cookie.
+        https://bugs.webkit.org/show_bug.cgi?id=86067
+
+        Reviewed by Rob Buis.
+
+        * http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added.
+        * http/tests/cookies/js-get-and-set-http-only-cookie.php: Added.
+        * http/tests/cookies/script-tests/js-get-and-set-http-only-cookie.js: Added.
+
 2012-05-22  Kangil Han  <[email protected]>
 
         [EFL][DRT] Implement touch event

Added: trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt (0 => 118105)


--- trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt	2012-05-23 02:49:58 UTC (rev 118105)
@@ -0,0 +1,12 @@
+Test for <https://bugs.webkit.org/show_bug.cgi?id=86067> [BlackBerry] Possible to clobber httpOnly cookie.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Check that we can't get or set httpOnly Cookies by _javascript_.
+PASS We can't get httpOnly cookies by _javascript_.
+PASS We can't set httpOnly cookies by _javascript_.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.php (0 => 118105)


--- trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.php	                        (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.php	2012-05-23 02:49:58 UTC (rev 118105)
@@ -0,0 +1,16 @@
+<?php
+    setcookie("httpOnlyCookie", "value", time()+36000000, "", "", 0, 1);
+?>
+<!DCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href=""
+<script src=""
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src=""
+<script src=""
+</body>
+</html>

Added: trunk/LayoutTests/http/tests/cookies/script-tests/js-get-and-set-http-only-cookie.js (0 => 118105)


--- trunk/LayoutTests/http/tests/cookies/script-tests/js-get-and-set-http-only-cookie.js	                        (rev 0)
+++ trunk/LayoutTests/http/tests/cookies/script-tests/js-get-and-set-http-only-cookie.js	2012-05-23 02:49:58 UTC (rev 118105)
@@ -0,0 +1,20 @@
+description(
+'Test for &lt;<a href="" [BlackBerry] Possible to clobber httpOnly cookie.'
+);
+
+debug("Check that we can't get or set httpOnly Cookies by _javascript_.");
+
+if (document.cookie == "httpOnlyCookie=value")
+    testFailed("We shouldn't get httpOnly cookies by _javascript_.");
+else
+    testPassed("We can't get httpOnly cookies by _javascript_.");
+
+document.cookie = "httpOnlyCookie=changedValue";
+if (document.cookie == "httpOnlyCookie=changedValue")
+    testFailed("We shouldn't set httpOnly cookies by _javascript_.");
+else
+    testPassed("We can't set httpOnly cookies by _javascript_.");
+
+clearCookies();
+
+successfullyParsed = true;

Modified: trunk/Source/WebCore/ChangeLog (118104 => 118105)


--- trunk/Source/WebCore/ChangeLog	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/ChangeLog	2012-05-23 02:49:58 UTC (rev 118105)
@@ -1,3 +1,23 @@
+2012-05-22  Jason Liu  <[email protected]>
+
+        [BlackBerry] Possible to clobber httponly cookie.
+        https://bugs.webkit.org/show_bug.cgi?id=86067
+
+        Reviewed by Rob Buis.
+
+        If a cookie is set by _javascript_ and there is already a same httpOnly cookie in cookieManager,
+        we should reject it. If it has a httpOnly property, we reject it, too.
+
+        Test: http/tests/cookies/js-get-and-set-http-only-cookie.php
+
+        * platform/blackberry/CookieJarBlackBerry.cpp:
+        (WebCore::setCookies):
+        * platform/blackberry/CookieManager.cpp:
+        (WebCore::CookieManager::setCookies):
+        (WebCore::CookieManager::shouldRejectNotHttpCookie):
+        (WebCore):
+        * platform/blackberry/CookieManager.h:
+
 2012-05-22  Dana Jansens  <[email protected]>
 
         [chromium] Don't drop children of a RenderSurface from the renderSurfaceLayerList when then position of the surface in its clipRect is not known

Modified: trunk/Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp (118104 => 118105)


--- trunk/Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/platform/blackberry/CookieJarBlackBerry.cpp	2012-05-23 02:49:58 UTC (rev 118105)
@@ -69,7 +69,7 @@
         return;
 
     ASSERT(document && url == document->cookieURL());
-    cookieManager().setCookies(url, value);
+    cookieManager().setCookies(url, value, NoHttpOnlyCookie);
 }
 
 bool cookiesEnabled(Document const*)

Modified: trunk/Source/WebCore/platform/blackberry/CookieManager.cpp (118104 => 118105)


--- trunk/Source/WebCore/platform/blackberry/CookieManager.cpp	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/platform/blackberry/CookieManager.cpp	2012-05-23 02:49:58 UTC (rev 118105)
@@ -131,7 +131,7 @@
     return protocol == "file" || protocol == "local";
 }
 
-void CookieManager::setCookies(const KURL& url, const String& value)
+void CookieManager::setCookies(const KURL& url, const String& value, CookieFilter filter)
 {
     CookieLog("CookieManager - Setting cookies");
     CookieParser parser(url);
@@ -139,7 +139,7 @@
 
     for (size_t i = 0; i < cookies.size(); ++i) {
         BackingStoreRemovalPolicy treatment = m_privateMode ? DoNotRemoveFromBackingStore : RemoveFromBackingStore;
-        checkAndTreatCookie(cookies[i], treatment);
+        checkAndTreatCookie(cookies[i], treatment, filter);
     }
 }
 
@@ -306,10 +306,16 @@
     m_cookieBackingStore->open(m_cookieJarFileName);
 }
 
-void CookieManager::checkAndTreatCookie(ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore)
+void CookieManager::checkAndTreatCookie(ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore, CookieFilter filter)
 {
     CookieLog("CookieManager - checkAndTreatCookie - processing url with domain - %s & protocol %s\n", candidateCookie->domain().utf8().data(), candidateCookie->protocol().utf8().data());
 
+    // A cookie which is not from http shouldn't have a httpOnly property.
+    if (filter == NoHttpOnlyCookie && candidateCookie->isHttpOnly()) {
+        delete candidateCookie;
+        return;
+    }
+
     const bool ignoreDomain = shouldIgnoreDomain(candidateCookie->protocol());
 
     // Determine which protocol tree to add the cookie to. Create one if necessary.
@@ -356,7 +362,7 @@
             m_cookieBackingStore->remove(candidateCookie);
         else if (curMap) {
             // RemoveCookie will return 0 if the cookie doesn't exist.
-            ParsedCookie* expired = curMap->removeCookie(candidateCookie);
+            ParsedCookie* expired = curMap->removeCookie(candidateCookie, filter);
             // Cookie is useless, Remove the cookie from the backingstore if it exists.
             // Backup check for BackingStoreCookieEntry incase someone incorrectly uses this enum.
             if (expired && postToBackingStore != BackingStoreCookieEntry && !expired->isSession()) {
@@ -369,15 +375,24 @@
             delete candidateCookie;
     } else {
         ASSERT(curMap);
-        addCookieToMap(curMap, candidateCookie, postToBackingStore);
+        addCookieToMap(curMap, candidateCookie, postToBackingStore, filter);
     }
 }
 
-void CookieManager::addCookieToMap(CookieMap* targetMap, ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore)
+void CookieManager::addCookieToMap(CookieMap* targetMap, ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore, CookieFilter filter)
 {
-    ParsedCookie* prevCookie = targetMap->addOrReplaceCookie(candidateCookie);
-    if (prevCookie) {
+    ParsedCookie* replacedCookie = 0;
 
+    if (!targetMap->addOrReplaceCookie(candidateCookie, &replacedCookie, filter)) {
+
+        CookieLog("CookieManager - rejecting new cookie - %s.\n", candidateCookie->toString().utf8().data());
+
+        delete candidateCookie;
+        return;
+    }
+ 
+    if (replacedCookie) {
+
         CookieLog("CookieManager - updating new cookie - %s.\n", candidateCookie->toString().utf8().data());
 
         // A cookie was replaced in targetMap.
@@ -385,7 +400,7 @@
         // If new cookie is non-session and old one is, we have to add it to backingstore
         // If both sessions are non-session, then we update it in the backingstore
         bool newIsSession = candidateCookie->isSession();
-        bool oldIsSession = prevCookie->isSession();
+        bool oldIsSession = replacedCookie->isSession();
 
         if (postToBackingStore == RemoveFromBackingStore) {
             if (!newIsSession && !oldIsSession)
@@ -394,7 +409,7 @@
                 // Must manually decrease the counter because it was not counted when
                 // the cookie was removed in cookieVector.
                 removedCookie();
-                m_cookieBackingStore->remove(prevCookie);
+                m_cookieBackingStore->remove(replacedCookie);
             } else if (!newIsSession && oldIsSession) {
                 // Must manually increase the counter because it was not counted when
                 // the cookie was added in cookieVector.
@@ -402,7 +417,7 @@
                 m_cookieBackingStore->insert(candidateCookie);
             }
         }
-        delete prevCookie;
+        delete replacedCookie;
         return;
     }
 
@@ -456,7 +471,7 @@
     }
 }
 
-void CookieManager::setPrivateMode(const bool mode)
+void CookieManager::setPrivateMode(bool mode)
 {
     if (m_privateMode == mode)
         return;
@@ -500,7 +515,6 @@
     return curMap;
 }
 
-
 void CookieManager::removeCookieWithName(const KURL& url, const String& cookieName)
 {
     // We get all cookies from all domains that domain matches the request domain

Modified: trunk/Source/WebCore/platform/blackberry/CookieManager.h (118104 => 118105)


--- trunk/Source/WebCore/platform/blackberry/CookieManager.h	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/platform/blackberry/CookieManager.h	2012-05-23 02:49:58 UTC (rev 118105)
@@ -49,11 +49,6 @@
     DoNotRemoveFromBackingStore
 };
 
-enum CookieFilter {
-    NoHttpOnlyCookie,
-    WithHttpOnlyCookies,
-};
-
 enum CookieStorageAcceptPolicy {
     CookieStorageAcceptPolicyAlways,
     CookieStorageAcceptPolicyNever,
@@ -79,7 +74,7 @@
     bool canLocalAccessAllCookies() const { return m_shouldDumpAllCookies; }
     void setCanLocalAccessAllCookies(bool enabled) { m_shouldDumpAllCookies = enabled; }
 
-    void setCookies(const KURL&, const String& value);
+    void setCookies(const KURL&, const String& value, CookieFilter = WithHttpOnlyCookies);
 
     void removeAllCookies(BackingStoreRemovalPolicy);
     void removeCookieWithName(const KURL&, const String& cookieName);
@@ -101,7 +96,7 @@
 
     void setCookiePolicy(CookieStorageAcceptPolicy policy) { m_policy = policy; }
     CookieStorageAcceptPolicy cookiePolicy() const { return m_policy; }
-    void setPrivateMode(const bool);
+    void setPrivateMode(bool);
 
     String generateHtmlFragmentForCookies();
     String getCookie(const KURL& requestURL, CookieFilter) const;
@@ -117,9 +112,9 @@
     CookieManager();
     virtual ~CookieManager();
 
-    void checkAndTreatCookie(ParsedCookie*, BackingStoreRemovalPolicy);
+    void checkAndTreatCookie(ParsedCookie*, BackingStoreRemovalPolicy, CookieFilter = WithHttpOnlyCookies);
 
-    void addCookieToMap(CookieMap* targetMap, ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore);
+    void addCookieToMap(CookieMap* targetMap, ParsedCookie* candidateCookie, BackingStoreRemovalPolicy postToBackingStore, CookieFilter = WithHttpOnlyCookies);
 
     CookieMap* findOrCreateCookieMap(CookieMap* protocolMap, const String& domain, bool findOnly);
 

Modified: trunk/Source/WebCore/platform/blackberry/CookieMap.cpp (118104 => 118105)


--- trunk/Source/WebCore/platform/blackberry/CookieMap.cpp	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/platform/blackberry/CookieMap.cpp	2012-05-23 02:49:58 UTC (rev 118105)
@@ -54,28 +54,31 @@
     deleteAllCookiesAndDomains();
 }
 
-ParsedCookie* CookieMap::addOrReplaceCookie(ParsedCookie* cookie)
+bool CookieMap::addOrReplaceCookie(ParsedCookie* candidateCookie, ParsedCookie** replacedCookie, CookieFilter filter)
 {
     CookieLog("CookieMap - Attempting to add cookie - %s", cookie->name().utf8().data());
 
-    ParsedCookie* prevCookie = 0;
     size_t cookieCount = m_cookieVector.size();
     for (size_t i = 0; i < cookieCount; i++) {
-        if (m_cookieVector[i]->name() == cookie->name() && m_cookieVector[i]->path() == cookie->path()) {
-            prevCookie = m_cookieVector[i];
-            m_cookieVector[i] = cookie;
-            if (prevCookie == m_oldestCookie)
+        if (m_cookieVector[i]->name() == candidateCookie->name() && m_cookieVector[i]->path() == candidateCookie->path()) {
+
+            if (filter == NoHttpOnlyCookie && m_cookieVector[i]->isHttpOnly())
+                return false;
+
+            *replacedCookie = m_cookieVector[i];
+            m_cookieVector[i] = candidateCookie;
+            if (*replacedCookie == m_oldestCookie)
                 updateOldestCookie();
-            return prevCookie;
+            return true;
         }
     }
 
-    m_cookieVector.append(cookie);
-    if (!cookie->isSession())
+    m_cookieVector.append(candidateCookie);
+    if (!candidateCookie->isSession())
         cookieManager().addedCookie();
-    if (!m_oldestCookie || m_oldestCookie->lastAccessed() > cookie->lastAccessed())
-        m_oldestCookie = cookie;
-    return 0;
+    if (!m_oldestCookie || m_oldestCookie->lastAccessed() > candidateCookie->lastAccessed())
+        m_oldestCookie = candidateCookie;
+    return true;
 }
 
 ParsedCookie* CookieMap::removeCookieAtIndex(int position, const ParsedCookie* cookie)
@@ -99,12 +102,15 @@
     return prevCookie;
 }
 
-ParsedCookie* CookieMap::removeCookie(const ParsedCookie* cookie)
+ParsedCookie* CookieMap::removeCookie(const ParsedCookie* cookie, CookieFilter filter)
 {
     size_t cookieCount = m_cookieVector.size();
     for (size_t position = 0; position < cookieCount; ++position) {
-        if (m_cookieVector[position]->name() == cookie->name() && m_cookieVector[position]->path() == cookie->path())
+        if (m_cookieVector[position]->name() == cookie->name() && m_cookieVector[position]->path() == cookie->path()) {
+            if (filter == NoHttpOnlyCookie && m_cookieVector[position]->isHttpOnly())
+                return 0;
             return removeCookieAtIndex(position, cookie);
+        }
     }
     return 0;
 }

Modified: trunk/Source/WebCore/platform/blackberry/CookieMap.h (118104 => 118105)


--- trunk/Source/WebCore/platform/blackberry/CookieMap.h	2012-05-23 02:48:00 UTC (rev 118104)
+++ trunk/Source/WebCore/platform/blackberry/CookieMap.h	2012-05-23 02:49:58 UTC (rev 118105)
@@ -34,6 +34,11 @@
 
 namespace WebCore {
 
+enum CookieFilter {
+    NoHttpOnlyCookie,
+    WithHttpOnlyCookies,
+};
+
 class ParsedCookie;
 
 /* A cookie map is a node in the tree held by CookieManager that represents
@@ -54,11 +59,11 @@
     unsigned int count() const { return m_cookieVector.size(); }
     const String& getName() const { return m_name; }
 
-    // Returning the original cookie object so manager can keep a reference to the updates in the database queue.
-    ParsedCookie* addOrReplaceCookie(ParsedCookie*);
+    // Return false if the candidateCookie is rejected.
+    bool addOrReplaceCookie(ParsedCookie* candidateCookie, ParsedCookie** replacedCookie, CookieFilter = WithHttpOnlyCookies);
 
     // Need to return the reference to the removed cookie so manager can deal with it (garbage collect).
-    ParsedCookie* removeCookie(const ParsedCookie*);
+    ParsedCookie* removeCookie(const ParsedCookie*, CookieFilter = WithHttpOnlyCookies);
 
     // Returns a map with that given subdomain.
     CookieMap* getSubdomainMap(const String&);
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to