prevent creating, modifying, and deleting HttpOnly cookies from web content, and add unit tests to that effect. b=383181, r+sr=dveditz
authordwitte@stanford.edu
Tue, 26 Jun 2007 01:36:50 -0700
changeset 2793 9329432b7e1c1a22be0d6c3a56d0bc8c4d2218e5
parent 2792 a9ad957a60c3c228ae90d483e6a103390e43d533
child 2794 6a9b35e26b2124b7c405751463a5240048098432
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs383181
milestone1.9a6pre
prevent creating, modifying, and deleting HttpOnly cookies from web content, and add unit tests to that effect. b=383181, r+sr=dveditz
netwerk/cookie/src/nsCookieService.cpp
netwerk/cookie/src/nsCookieService.h
netwerk/test/TestCookie.cpp
--- a/netwerk/cookie/src/nsCookieService.cpp
+++ b/netwerk/cookie/src/nsCookieService.cpp
@@ -850,17 +850,17 @@ nsCookieService::SetCookieValue(nsIURI *
   attributes.path = aPath;
   attributes.expiryTime = aExpiry;
   attributes.creationID = PR_Now();
   attributes.isSession = aIsSession;
 
   attributes.isSecure = PR_FALSE;
   aHostURI->SchemeIs("https", &attributes.isSecure);
 
-  CheckAndAdd(aHostURI, aChannel, attributes, EmptyCString());
+  CheckAndAdd(aHostURI, aChannel, attributes, EmptyCString(), PR_FALSE);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCookieService::SetCookieString(nsIURI     *aHostURI,
                                  nsIPrompt  *aPrompt,
                                  const char *aCookieHeader,
                                  nsIChannel *aChannel)
@@ -869,27 +869,39 @@ nsCookieService::SetCookieString(nsIURI 
   nsCOMPtr<nsIURI> firstURI;
 
   if (aChannel) {
     nsCOMPtr<nsIHttpChannelInternal> httpInternal = do_QueryInterface(aChannel);
     if (httpInternal)
       httpInternal->GetDocumentURI(getter_AddRefs(firstURI));
   }
 
-  return SetCookieStringFromHttp(aHostURI, firstURI, aPrompt, aCookieHeader, nsnull, aChannel);
+  return SetCookieStringInternal(aHostURI, firstURI, aPrompt, aCookieHeader, nsnull, aChannel, PR_FALSE);
 }
 
 NS_IMETHODIMP
 nsCookieService::SetCookieStringFromHttp(nsIURI     *aHostURI,
                                          nsIURI     *aFirstURI,
                                          nsIPrompt  *aPrompt,
                                          const char *aCookieHeader,
                                          const char *aServerTime,
                                          nsIChannel *aChannel) 
 {
+  return SetCookieStringInternal(aHostURI, aFirstURI, aPrompt, aCookieHeader, aServerTime, aChannel, PR_TRUE);
+}
+
+nsresult
+nsCookieService::SetCookieStringInternal(nsIURI     *aHostURI,
+                                         nsIURI     *aFirstURI,
+                                         nsIPrompt  *aPrompt,
+                                         const char *aCookieHeader,
+                                         const char *aServerTime,
+                                         nsIChannel *aChannel,
+                                         PRBool      aFromHttp) 
+{
   if (!aHostURI) {
     COOKIE_LOGFAILURE(SET_COOKIE, nsnull, aCookieHeader, "host URI is null");
     return NS_OK;
   }
 
   // check default prefs
   PRUint32 cookieStatus = CheckPrefs(aHostURI, aFirstURI, aChannel, aCookieHeader);
   // fire a notification if cookie was rejected (but not if there was an error)
@@ -914,17 +926,17 @@ nsCookieService::SetCookieStringFromHttp
   }
 
   // start a transaction on the storage db, to optimize insertions.
   // transaction will automically commit on completion
   mozStorageTransaction transaction(mDBConn, PR_TRUE);
  
   // switch to a nice string type now, and process each cookie in the header
   nsDependentCString cookieHeader(aCookieHeader);
-  while (SetCookieInternal(aHostURI, aChannel, cookieHeader, serverTime));
+  while (SetCookieInternal(aHostURI, aChannel, cookieHeader, serverTime, aFromHttp));
 
   return NS_OK;
 }
 
 // notify observers that a cookie was rejected due to the users' prefs.
 void
 nsCookieService::NotifyRejected(nsIURI *aHostURI)
 {
@@ -1025,17 +1037,17 @@ nsCookieService::Add(const nsACString &a
                      currentTimeInUsec,
                      aIsSession,
                      aIsSecure,
                      aIsHttpOnly);
   if (!cookie) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
-  AddInternal(cookie, currentTimeInUsec / PR_USEC_PER_SEC, nsnull, nsnull);
+  AddInternal(cookie, currentTimeInUsec / PR_USEC_PER_SEC, nsnull, nsnull, PR_TRUE);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCookieService::Remove(const nsACString &aHost,
                         const nsACString &aName,
                         const nsACString &aPath,
                         PRBool           aBlocked)
@@ -1279,17 +1291,18 @@ nsCookieService::ImportCookies()
  ******************************************************************************/
 
 // processes a single cookie, and returns PR_TRUE if there are more cookies
 // to be processed
 PRBool
 nsCookieService::SetCookieInternal(nsIURI             *aHostURI,
                                    nsIChannel         *aChannel,
                                    nsDependentCString &aCookieHeader,
-                                   PRInt64             aServerTime)
+                                   PRInt64             aServerTime,
+                                   PRBool              aFromHttp)
 {
   // create a stack-based nsCookieAttributes, to store all the
   // attributes parsed from the cookie
   nsCookieAttributes cookieAttributes;
 
   // init expiryTime such that session cookies won't prematurely expire
   cookieAttributes.expiryTime = LL_MAXINT;
 
@@ -1304,26 +1317,27 @@ nsCookieService::SetCookieInternal(nsIUR
   // generate a creation id for the cookie
   PRInt64 currentTimeInUsec = PR_Now();
   cookieAttributes.creationID = currentTimeInUsec;
 
   // calculate expiry time of cookie.
   cookieAttributes.isSession = GetExpiry(cookieAttributes, aServerTime,
                                          currentTimeInUsec / PR_USEC_PER_SEC);
 
-  CheckAndAdd(aHostURI, aChannel, cookieAttributes, savedCookieHeader);
+  CheckAndAdd(aHostURI, aChannel, cookieAttributes, savedCookieHeader, aFromHttp);
 
   return newCookie;
 }
 
 void
 nsCookieService::CheckAndAdd(nsIURI               *aHostURI,
                              nsIChannel           *aChannel,
                              nsCookieAttributes   &aAttributes,
-                             const nsAFlatCString &aCookieHeader)
+                             const nsAFlatCString &aCookieHeader,
+                             PRBool                aFromHttp)
 {
   // reject cookie if it's over the size limit, per RFC2109
   if ((aAttributes.name.Length() + aAttributes.value.Length()) > kMaxBytesPerCookie) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "cookie too big (> 4kb)");
     return;
   }
 
   if (aAttributes.name.FindChar('\t') != kNotFound) {
@@ -1376,58 +1390,72 @@ nsCookieService::CheckAndAdd(nsIURI     
 
     // update isSession and expiry attributes, in case they changed
     cookie->SetIsSession(aAttributes.isSession);
     cookie->SetExpiry(aAttributes.expiryTime);
   }
 
   // add the cookie to the list. AddInternal() takes care of logging.
   // we get the current time again here, since it may have changed during prompting
-  AddInternal(cookie, PR_Now() / PR_USEC_PER_SEC, aHostURI, aCookieHeader.get());
+  AddInternal(cookie, PR_Now() / PR_USEC_PER_SEC, aHostURI, aCookieHeader.get(), aFromHttp);
 }
 
 // this is a backend function for adding a cookie to the list, via SetCookie.
 // also used in the cookie manager, for profile migration from IE.
 // it either replaces an existing cookie; or adds the cookie to the hashtable,
 // and deletes a cookie (if maximum number of cookies has been
 // reached). also performs list maintenance by removing expired cookies.
 void
 nsCookieService::AddInternal(nsCookie   *aCookie,
                              PRInt64     aCurrentTime,
                              nsIURI     *aHostURI,
-                             const char *aCookieHeader)
+                             const char *aCookieHeader,
+                             PRBool      aFromHttp)
 {
   // start a transaction on the storage db, to optimize deletions/insertions.
   // transaction will automically commit on completion. if we already have a
   // transaction (e.g. from SetCookie*()), this will have no effect. 
   mozStorageTransaction transaction(mDBConn, PR_TRUE);
 
   nsListIter matchIter;
   const PRBool foundCookie =
     FindCookie(aCookie->Host(), aCookie->Name(), aCookie->Path(), matchIter);
 
   nsRefPtr<nsCookie> oldCookie;
   if (foundCookie) {
     oldCookie = matchIter.current;
+
+    // if the old cookie is httponly, make sure we're not coming from script
+    if (!aFromHttp && oldCookie->IsHttpOnly()) {
+      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie is httponly; coming from script");
+      return;
+    }
+
     RemoveCookieFromList(matchIter);
 
     // check if the cookie has expired
     if (aCookie->Expiry() <= aCurrentTime) {
       COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "previously stored cookie was deleted");
       NotifyChanged(oldCookie, NS_LITERAL_STRING("deleted").get());
       return;
     }
 
   } else {
     // check if cookie has already expired
     if (aCookie->Expiry() <= aCurrentTime) {
       COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "cookie has already expired");
       return;
     }
 
+    // if the new cookie is httponly, make sure we're not coming from script
+    if (!aFromHttp && aCookie->IsHttpOnly()) {
+      COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader, "cookie is httponly; coming from script");
+      return;
+    }
+
     // check if we have to delete an old cookie.
     nsEnumerationData data(aCurrentTime, LL_MAXINT);
     if (CountCookiesFromHostInternal(aCookie->RawHost(), data) >= mMaxCookiesPerHost) {
       // remove the oldest cookie from host
       oldCookie = data.iter.current;
       RemoveCookieFromList(data.iter);
 
     } else if (mCookieCount >= mMaxNumberOfCookies) {
--- a/netwerk/cookie/src/nsCookieService.h
+++ b/netwerk/cookie/src/nsCookieService.h
@@ -166,19 +166,20 @@ class nsCookieService : public nsICookie
   protected:
     void                          PrefChanged(nsIPrefBranch *aPrefBranch);
     nsresult                      InitDB();
     nsresult                      CreateTable();
     nsresult                      ImportCookies();
     nsresult                      Read();
     void                          GetCookieList(nsIURI *aHostURI, nsIURI *aFirstURI, nsIChannel *aChannel, const nsACString *aName, PRBool isHttpBound, nsAutoVoidArray &aResult);
     char*                         CookieStringFromArray(const nsAutoVoidArray& aCookieList, nsIURI *aHostURI);
-    PRBool                        SetCookieInternal(nsIURI *aHostURI, nsIChannel *aChannel, nsDependentCString &aCookieHeader, PRInt64 aServerTime);
-    void                          CheckAndAdd(nsIURI *aHostURI, nsIChannel *aChannel, nsCookieAttributes &aAttributes, const nsAFlatCString &aCookieHeader);
-    void                          AddInternal(nsCookie *aCookie, PRInt64 aCurrentTime, nsIURI *aHostURI, const char *aCookieHeader);
+    nsresult                      SetCookieStringInternal(nsIURI *aHostURI, nsIURI *aFirstURI, nsIPrompt *aPrompt, const char *aCookieHeader, const char *aServerTime, nsIChannel *aChannel, PRBool aFromHttp);
+    PRBool                        SetCookieInternal(nsIURI *aHostURI, nsIChannel *aChannel, nsDependentCString &aCookieHeader, PRInt64 aServerTime, PRBool aFromHttp);
+    void                          CheckAndAdd(nsIURI *aHostURI, nsIChannel *aChannel, nsCookieAttributes &aAttributes, const nsAFlatCString &aCookieHeader, PRBool aFromHttp);
+    void                          AddInternal(nsCookie *aCookie, PRInt64 aCurrentTime, nsIURI *aHostURI, const char *aCookieHeader, PRBool aFromHttp);
     void                          RemoveCookieFromList(nsListIter &aIter);
     PRBool                        AddCookieToList(nsCookie *aCookie, PRBool aWriteToDB = PR_TRUE);
     static PRBool                 GetTokenValue(nsASingleFragmentCString::const_char_iterator &aIter, nsASingleFragmentCString::const_char_iterator &aEndIter, nsDependentCSubstring &aTokenString, nsDependentCSubstring &aTokenValue, PRBool &aEqualsFound);
     static PRBool                 ParseAttributes(nsDependentCString &aCookieHeader, nsCookieAttributes &aCookie);
     static PRBool                 IsIPAddress(const nsAFlatCString &aHost);
     static PRBool                 IsInDomain(const nsACString &aDomain, const nsACString &aHost, PRBool aIsDomain = PR_TRUE);
     static PRBool                 IsForeign(nsIURI *aHostURI, nsIURI *aFirstURI);
     PRUint32                      CheckPrefs(nsIURI *aHostURI, nsIURI *aFirstURI, nsIChannel *aChannel, const char *aCookieHeader);
--- a/netwerk/test/TestCookie.cpp
+++ b/netwerk/test/TestCookie.cpp
@@ -637,29 +637,57 @@ main(PRInt32 argc, char *argv[])
       SetACookie(cookieService, "http://multi.path.tests/", nsnull, "test1=path; path=/one/two/three", nsnull);
       SetACookie(cookieService, "http://multi.path.tests/", nsnull, "test2=path; path=/one \n test3=path; path=/one/two/three/four \n test4=path; path=/one/two \n test5=path; path=/one/two/", nsnull);
       SetACookie(cookieService, "http://multi.path.tests/one/two/three/four/five/", nsnull, "test6=path", nsnull);
       SetACookie(cookieService, "http://multi.path.tests/one/two/three/four/five/six/", nsnull, "test7=path; path=", nsnull);
       SetACookie(cookieService, "http://multi.path.tests/", nsnull, "test8=path; path=/", nsnull);
       GetACookie(cookieService, "http://multi.path.tests/one/two/three/four/five/six/", nsnull, getter_Copies(cookie));
       rv[0] = CheckResult(cookie.get(), MUST_EQUAL, "test7=path; test6=path; test3=path; test1=path; test5=path; test4=path; test2=path; test8=path");
 
+      allTestsPassed = PrintResult(rv, 1) && allTestsPassed;
+
 
       // *** httponly tests 
       printf("*** Beginning httponly tests...\n");
+
+      // Since this cookie is NOT set via http, setting it fails
+      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=httponly; httponly");
+      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
+      rv[0] = CheckResult(cookie.get(), MUST_BE_NULL);
       // Since this cookie is set via http, it can be retrieved
       SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly", nsnull);
       GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
-      rv[0] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");
-      // Since this cookie is NOT set via http, it can NOT be retrieved
-      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=httponly; httponly");
+      rv[1] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");
+      // ... but not by web content
+      GetACookieNoHttp(cookieService, "http://httponly.test/", getter_Copies(cookie));
+      rv[2] = CheckResult(cookie.get(), MUST_BE_NULL);
+      // Non-Http cookies should not replace HttpOnly cookies
+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly", nsnull);
+      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=not-httponly");
+      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
+      rv[3] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");
+      // ... and, if an HttpOnly cookie already exists, should not be set at all
       GetACookieNoHttp(cookieService, "http://httponly.test/", getter_Copies(cookie));
-      rv[1] = CheckResult(cookie.get(), MUST_NOT_EQUAL, "test=httponly");
+      rv[4] = CheckResult(cookie.get(), MUST_BE_NULL);
+      // Non-Http cookies should not delete HttpOnly cookies
+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly", nsnull);
+      SetACookieNoHttp(cookieService, "http://httponly.test/", "test=httponly; max-age=-1");
+      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
+      rv[5] = CheckResult(cookie.get(), MUST_EQUAL, "test=httponly");
+      // ... but HttpOnly cookies should
+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly; max-age=-1", nsnull);
+      GetACookie(cookieService, "http://httponly.test/", nsnull, getter_Copies(cookie));
+      rv[6] = CheckResult(cookie.get(), MUST_BE_NULL);
+      // Non-Httponly cookies can replace HttpOnly cookies when set over http
+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=httponly; httponly", nsnull);
+      SetACookie(cookieService, "http://httponly.test/", nsnull, "test=not-httponly", nsnull);
+      GetACookieNoHttp(cookieService, "http://httponly.test/", getter_Copies(cookie));
+      rv[7] = CheckResult(cookie.get(), MUST_EQUAL, "test=not-httponly");
 
-      allTestsPassed = PrintResult(rv, 2) && allTestsPassed;
+      allTestsPassed = PrintResult(rv, 8) && allTestsPassed;
 
 
       // *** nsICookieManager{2} interface tests
       printf("*** Beginning nsICookieManager{2} interface tests...\n");
       nsCOMPtr<nsICookieManager> cookieMgr = do_GetService(NS_COOKIEMANAGER_CONTRACTID, &rv0);
       if (NS_FAILED(rv0)) return -1;
       nsCOMPtr<nsICookieManager2> cookieMgr2 = do_QueryInterface(cookieMgr);
       if (!cookieMgr2) return -1;