Bug 1440462 - Send httponly cookie names to content processes. r=jdm
authorNicholas Hurley <hurley@mozilla.com>
Wed, 26 Sep 2018 15:39:33 +0000
changeset 496762 4b87a1cf5afe90be21b815f15380cbae184343b8
parent 496761 650083b7ab814e858766d1c61728aaa7c1d3e39c
child 496763 b91faf6f965b23ea06fd208dceb571fc473a7953
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
bugs1440462
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1440462 - Send httponly cookie names to content processes. r=jdm Previously, if script tried to set a cookie that matched a cookie we had received via Set-Cookie that was labeled httponly, script would think that cookie was properly set (even though it wasn't). This ensures that script knows just enough about httponly cookies to prevent this inconsistent view while avoiding leakages of the potentially-sensitive cookie values. Differential Revision: https://phabricator.services.mozilla.com/D5700
netwerk/cookie/CookieServiceChild.cpp
netwerk/cookie/CookieServiceParent.cpp
--- a/netwerk/cookie/CookieServiceChild.cpp
+++ b/netwerk/cookie/CookieServiceChild.cpp
@@ -386,16 +386,21 @@ CookieServiceChild::GetCookieStringFromC
 
   cookiesList->Sort(CompareCookiesForSending());
   for (uint32_t i = 0; i < cookiesList->Length(); i++) {
     nsCookie *cookie = cookiesList->ElementAt(i);
     // check the host, since the base domain lookup is conservative.
     if (!nsCookieService::DomainMatches(cookie, hostFromURI))
       continue;
 
+    // We don't show HttpOnly cookies in content processes.
+    if (cookie->IsHttpOnly()) {
+      continue;
+    }
+
     // if the cookie is secure and the host scheme isn't, we can't send it
     if (cookie->IsSecure() && !isSecure)
       continue;
 
     int32_t sameSiteAttr = 0;
     cookie->GetSameSite(&sameSiteAttr);
     if (aIsSameSiteForeign && nsCookieService::IsSameSiteEnabled()) {
       // it if's a cross origin request and the cookie is same site only (strict)
@@ -516,19 +521,17 @@ CookieServiceChild::RecordDocumentCookie
     }
   }
 
   int64_t currentTime = PR_Now() / PR_USEC_PER_SEC;
   if (aCookie->Expiry() <= currentTime) {
     return;
   }
 
-  if (!aCookie->IsHttpOnly()) {
-    cookiesList->AppendElement(aCookie);
-  }
+  cookiesList->AppendElement(aCookie);
 }
 
 nsresult
 CookieServiceChild::GetCookieStringInternal(nsIURI *aHostURI,
                                             nsIChannel *aChannel,
                                             char **aCookieString)
 {
   NS_ENSURE_ARG(aHostURI);
@@ -672,29 +675,48 @@ CookieServiceChild::SetCookieStringInter
                                 firstPartyStorageAccessGranted, aCookieString,
                                 CountCookiesFromHashTable(baseDomain, attrs),
                                 attrs, nullptr);
 
   if (cookieStatus != STATUS_ACCEPTED && cookieStatus != STATUS_ACCEPT_SESSION) {
     return NS_OK;
   }
 
+  nsCookieKey key(baseDomain, attrs);
+  CookiesList *cookies = mCookiesMap.Get(key);
+
   nsCString serverTimeString(aServerTime);
   int64_t serverTime = nsCookieService::ParseServerTime(serverTimeString);
   bool moreCookies;
   do {
     nsCookieAttributes cookieAttributes;
     bool canSetCookie = false;
-    nsCookieKey key(baseDomain, attrs);
     moreCookies = nsCookieService::CanSetCookie(aHostURI, key, cookieAttributes,
                                                 requireHostMatch, cookieStatus,
                                                 cookieString, serverTime, aFromHttp,
                                                 aChannel, mLeaveSecureAlone,
                                                 canSetCookie, mThirdPartyUtil);
 
+    // We need to see if the cookie we're setting would overwrite an httponly
+    // one. This would not affect anything we send over the net (those come from
+    // the parent, which already checks this), but script could see an
+    // inconsistent view of things.
+    if (cookies && canSetCookie && !aFromHttp) {
+      for (uint32_t i = 0; i < cookies->Length(); ++i) {
+        RefPtr<nsCookie> cookie = cookies->ElementAt(i);
+        if (cookie->Name().Equals(cookieAttributes.name) &&
+            cookie->Host().Equals(cookieAttributes.host) &&
+            cookie->Path().Equals(cookieAttributes.path) &&
+            cookie->IsHttpOnly()) {
+          // Can't overwrite an httponly cookie from a script context.
+          canSetCookie = false;
+        }
+      }
+    }
+
     if (canSetCookie) {
       SetCookieInternal(cookieAttributes, attrs, aChannel,
                         aFromHttp, permissionService);
     }
 
     // document.cookie can only set one cookie at a time.
     if (!aFromHttp) {
       break;
--- a/netwerk/cookie/CookieServiceParent.cpp
+++ b/netwerk/cookie/CookieServiceParent.cpp
@@ -95,20 +95,22 @@ CookieServiceParent::RemoveBatchDeletedC
   CookieStruct cookieStruct;
   nsTArray<CookieStruct> cookieStructList;
   nsTArray<OriginAttributes> attrsList;
   for (uint32_t i = 0; i < len; i++) {
     nsCOMPtr<nsICookie> xpcCookie = do_QueryElementAt(aCookieList, i);
     auto cookie = static_cast<nsCookie*>(xpcCookie.get());
     attrs = cookie->OriginAttributesRef();
     GetInfoFromCookie(cookie, cookieStruct);
-    if (!cookie->IsHttpOnly()) {
-      cookieStructList.AppendElement(cookieStruct);
-      attrsList.AppendElement(attrs);
+    if (cookie->IsHttpOnly()) {
+      // Child only needs to exist if an HttpOnly cookie exists, not its value
+      cookieStruct.value() = "";
     }
+    cookieStructList.AppendElement(cookieStruct);
+    attrsList.AppendElement(attrs);
   }
   Unused << SendRemoveBatchDeletedCookies(cookieStructList, attrsList);
 }
 
 void
 CookieServiceParent::RemoveAll()
 {
   Unused << SendRemoveAll();
@@ -116,28 +118,32 @@ CookieServiceParent::RemoveAll()
 
 void
 CookieServiceParent::RemoveCookie(nsICookie *aCookie)
 {
   auto cookie = static_cast<nsCookie*>(aCookie);
   OriginAttributes attrs = cookie->OriginAttributesRef();
   CookieStruct cookieStruct;
   GetInfoFromCookie(cookie, cookieStruct);
-  if (!cookie->IsHttpOnly()) {
-    Unused << SendRemoveCookie(cookieStruct, attrs);
+  if (cookie->IsHttpOnly()) {
+    cookieStruct.value() = "";
   }
+  Unused << SendRemoveCookie(cookieStruct, attrs);
 }
 
 void
 CookieServiceParent::AddCookie(nsICookie *aCookie)
 {
   auto cookie = static_cast<nsCookie*>(aCookie);
   OriginAttributes attrs = cookie->OriginAttributesRef();
   CookieStruct cookieStruct;
   GetInfoFromCookie(cookie, cookieStruct);
+  if (cookie->IsHttpOnly()) {
+    cookieStruct.value() = "";
+  }
   Unused << SendAddCookie(cookieStruct, attrs);
 }
 
 void
 CookieServiceParent::TrackCookieLoad(nsIChannel *aChannel)
 {
   nsCOMPtr<nsIURI> uri;
   aChannel->GetURI(getter_AddRefs(uri));
@@ -186,17 +192,19 @@ void
 CookieServiceParent::SerialializeCookieList(const nsTArray<nsCookie*> &aFoundCookieList,
                                             nsTArray<CookieStruct>    &aCookiesList,
                                             nsIURI                    *aHostURI)
 {
   for (uint32_t i = 0; i < aFoundCookieList.Length(); i++) {
     nsCookie *cookie = aFoundCookieList.ElementAt(i);
     CookieStruct* cookieStruct = aCookiesList.AppendElement();
     cookieStruct->name() = cookie->Name();
-    cookieStruct->value() = cookie->Value();
+    if (!cookie->IsHttpOnly()) {
+      cookieStruct->value() = cookie->Value();
+    }
     cookieStruct->host() = cookie->Host();
     cookieStruct->path() = cookie->Path();
     cookieStruct->expiry() = cookie->Expiry();
     cookieStruct->lastAccessed() = cookie->LastAccessed();
     cookieStruct->creationTime() = cookie->CreationTime();
     cookieStruct->isSession() = cookie->IsSession();
     cookieStruct->isSecure() = cookie->IsSecure();
     cookieStruct->sameSite() = cookie->SameSite();