Bug 1201747 - Don't inspect the subject principal in StorageAllowedForPrincipal. r=mystor, a=al
authorBobby Holley <bobbyholley@gmail.com>
Fri, 18 Sep 2015 18:19:33 -0700
changeset 296173 0fbd79c7d54183cb25d9a7bc0873a317d869af07
parent 296172 ea609f3830eacc917728d3e1994aa481c00d7f2f
child 296174 8a9b291e31b70644bd8b7313b82032ad410311e7
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmystor, al
bugs1201747
milestone43.0a2
Bug 1201747 - Don't inspect the subject principal in StorageAllowedForPrincipal. r=mystor, a=al
dom/base/nsContentUtils.cpp
dom/tests/mochitest/general/frameStorageChrome.html
dom/tests/mochitest/general/storagePermissionsUtils.js
dom/tests/mochitest/general/test_storagePermissionsAccept.html
dom/tests/mochitest/general/test_storagePermissionsLimitForeign.html
dom/tests/mochitest/general/test_storagePermissionsReject.html
dom/tests/mochitest/general/test_storagePermissionsRejectForeign.html
--- a/dom/base/nsContentUtils.cpp
+++ b/dom/base/nsContentUtils.cpp
@@ -8128,38 +8128,27 @@ nsContentUtils::InternalStorageAllowedFo
   // Check if we should only allow storage for the session, and record that fact
   if (sCookiesLifetimePolicy == nsICookieService::ACCEPT_SESSION) {
     // Storage could be StorageAccess::ePrivateBrowsing or StorageAccess::eAllow
     // so perform a std::min comparison to make sure we preserve ePrivateBrowsing
     // if it has been set.
     access = std::min(StorageAccess::eSessionScoped, access);
   }
 
-  // If the caller is chrome privileged, then it is allowed to access any
-  // storage it likes, no matter whether the storage for that window/principal
-  // would normally be permitted.
-  if (IsSystemPrincipal(SubjectPrincipal())) {
-    return access;
-  }
-
-  if (!SubjectPrincipal()->Subsumes(aPrincipal)) {
-    NS_WARNING("A principal is attempting to access storage for a principal "
-               "which it doesn't subsume!");
-    return StorageAccess::eDeny;
-  }
-
   // About URIs are allowed to access storage, even if they don't have chrome
   // privileges. If this is not desired, than the consumer will have to
   // implement their own restriction functionality.
   nsCOMPtr<nsIURI> uri;
-  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(aPrincipal->GetURI(getter_AddRefs(uri))));
-  bool isAbout = false;
-  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(uri->SchemeIs("about", &isAbout)));
-  if (isAbout) {
-    return access;
+  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
+  if (NS_SUCCEEDED(rv) && uri) {
+    bool isAbout = false;
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(uri->SchemeIs("about", &isAbout)));
+    if (isAbout) {
+      return access;
+    }
   }
 
   nsCOMPtr<nsIPermissionManager> permissionManager =
     services::GetPermissionManager();
   if (!permissionManager) {
     return StorageAccess::eDeny;
   }
 
--- a/dom/tests/mochitest/general/frameStorageChrome.html
+++ b/dom/tests/mochitest/general/frameStorageChrome.html
@@ -2,17 +2,18 @@
 <head>
 <title>frame for storage allowed test</title>
 
 <script type="text/javascript" src="https://example.com/tests/dom/tests/mochitest/general/storagePermissionsUtils.js"></script>
 <script type="text/javascript">
 
   task(function* () {
     // We just check if we can access storage as chrome using special powers!
-    yield chromePower();
+    var params = new URLSearchParams(location.search.substr(1));
+    yield chromePower(params.get('allowed') == 'yes', params.get('blockSessionStorage') == 'yes');
   });
 
 </script>
 
 </head>
 
 <body>
 </body>
--- a/dom/tests/mochitest/general/storagePermissionsUtils.js
+++ b/dom/tests/mochitest/general/storagePermissionsUtils.js
@@ -57,54 +57,59 @@ function runWorker(url) {
         return;
       }
 
       ok(!e.data.match(/^FAILURE/), e.data + " (WORKER = " + url + ")");
     });
   });
 }
 
-function chromePower() {
+function chromePower(allowed, blockSessionStorage) {
+
+  // localStorage is affected by storage policy.
   try {
     SpecialPowers.wrap(window).localStorage.getItem("X");
-    ok(true, "getting localStorage didn't throw");
+    ok(allowed, "getting localStorage from chrome didn't throw");
   } catch (e) {
-    ok(false, "getting localStorage should not throw");
+    ok(!allowed, "getting localStorage from chrome threw");
   }
 
+
+  // sessionStorage is not. See bug 1183968.
   try {
     SpecialPowers.wrap(window).sessionStorage.getItem("X");
-    ok(true, "getting sessionStorage didn't throw");
+    ok(!blockSessionStorage, "getting sessionStorage from chrome didn't throw");
   } catch (e) {
-    ok(false, "getting sessionStorage should not throw");
+    ok(blockSessionStorage, "getting sessionStorage from chrome threw");
   }
 
+  // indexedDB is affected by storage policy.
   try {
     SpecialPowers.wrap(window).indexedDB;
-    ok(true, "getting indexedDB didn't throw");
+    ok(allowed, "getting indexedDB from chrome didn't throw");
   } catch (e) {
-    ok(false, "getting indexedDB should not throw");
+    ok(!allowed, "getting indexedDB from chrome threw");
   }
 
+  // Same with caches, along with the additional https-only requirement.
   try {
+    var shouldResolve = allowed && location.protocol == "https:";
     var promise = SpecialPowers.wrap(window).caches.keys();
-    ok(true, "getting caches didn't throw");
-
+    ok(true, "getting caches from chrome should never throw");
     return new Promise((resolve, reject) => {
       promise.then(function() {
-        ok(location.protocol == "https:", "The promise was not rejected");
+        ok(shouldResolve, "The promise was resolved for chrome");
         resolve();
       }, function(e) {
-        ok(location.protocol != "https:", "The promise should not have been rejected: " + e);
+        ok(!shouldResolve, "The promise was rejected for chrome: " + e);
         resolve();
       });
     });
   } catch (e) {
-    ok(false, "getting caches should not have thrown");
-    return Promise.resolve();
+    ok(false, "getting caches from chrome threw");
   }
 }
 
 function storageAllowed() {
   try {
     localStorage.getItem("X");
     ok(true, "getting localStorage didn't throw");
   } catch (e) {
--- a/dom/tests/mochitest/general/test_storagePermissionsAccept.html
+++ b/dom/tests/mochitest/general/test_storagePermissionsAccept.html
@@ -15,28 +15,28 @@ task(function* () {
   yield setCookieBehavior(BEHAVIOR_ACCEPT);
 
   // We should be able to access storage
   yield storageAllowed();
 
   // Same origin iframes should be allowed, unless they redirect to a URI with the null principal
   yield runIFrame("frameStorageAllowed.html");
   yield runIFrame("frameStorageNullprincipal.sjs");
-  yield runIFrame("frameStorageChrome.html");
+  yield runIFrame("frameStorageChrome.html?allowed=yes");
 
   // Sandboxed iframes should have the null principal, and thus can't access storage
   document.querySelector('iframe').setAttribute('sandbox', 'allow-scripts');
   yield runIFrame("frameStoragePrevented.html#nullprincipal");
   yield runIFrame("frameStorageNullprincipal.sjs");
   document.querySelector('iframe').removeAttribute('sandbox');
 
   // Thirdparty iframes should be allowed, unless they redirect to a URI with the null principal
   yield runIFrame(thirdparty + "frameStorageAllowed.html");
   yield runIFrame(thirdparty + "frameStorageNullprincipal.sjs");
-  yield runIFrame(thirdparty + "frameStorageChrome.html");
+  yield runIFrame(thirdparty + "frameStorageChrome.html?allowed=yes");
 
   // Workers should be able to access storage
   yield runWorker("workerStorageAllowed.js");
 });
 
     </script>
   </body>
 </html>
--- a/dom/tests/mochitest/general/test_storagePermissionsLimitForeign.html
+++ b/dom/tests/mochitest/general/test_storagePermissionsLimitForeign.html
@@ -12,31 +12,33 @@
     <script type="text/javascript">
 
 task(function* () {
   yield setCookieBehavior(BEHAVIOR_LIMIT_FOREIGN);
 
   // We should be able to access storage
   yield storageAllowed();
 
-  // Same origin iframes should be prevented, unless they have chrome privileges
+  // Same origin iframes should be allowed.
   yield runIFrame("frameStorageAllowed.html");
+  yield runIFrame("frameStorageChrome.html?allowed=yes");
+
+  // Null principal iframes should not.
   yield runIFrame("frameStorageNullprincipal.sjs");
-  yield runIFrame("frameStorageChrome.html");
 
   // Sandboxed iframes should have the null principal, and thus can't access storage
   document.querySelector('iframe').setAttribute('sandbox', 'allow-scripts');
   yield runIFrame("frameStoragePrevented.html#nullprincipal");
   yield runIFrame("frameStorageNullprincipal.sjs");
   document.querySelector('iframe').removeAttribute('sandbox');
 
-  // Thirdparty iframes should be blocked, unless they have chrome privileges
+  // Thirdparty iframes should be blocked, even when accessed from chrome over Xrays.
   yield runIFrame(thirdparty + "frameStoragePrevented.html#thirdparty");
   yield runIFrame(thirdparty + "frameStorageNullprincipal.sjs");
-  yield runIFrame(thirdparty + "frameStorageChrome.html");
+  yield runIFrame(thirdparty + "frameStorageChrome.html?allowed=no");
 
   // Workers should be unable to access storage
   yield runWorker("workerStorageAllowed.js");
 });
 
     </script>
   </body>
 </html>
--- a/dom/tests/mochitest/general/test_storagePermissionsReject.html
+++ b/dom/tests/mochitest/general/test_storagePermissionsReject.html
@@ -12,30 +12,31 @@
     <script type="text/javascript">
 
 task(function* () {
   yield setCookieBehavior(BEHAVIOR_REJECT);
 
   // We should be unable to access storage
   yield storagePrevented();
 
-  // Same origin iframes should be prevented, unless they have chrome privileges
+  // Same origin iframes should be blocked.
   yield runIFrame("frameStoragePrevented.html");
   yield runIFrame("frameStorageNullprincipal.sjs");
-  yield runIFrame("frameStorageChrome.html");
+  yield runIFrame("frameStorageChrome.html?allowed=no&blockSessionStorage=yes");
 
   // Sandboxed iframes should have the null principal, and thus can't access storage
   document.querySelector('iframe').setAttribute('sandbox', 'allow-scripts');
   yield runIFrame("frameStoragePrevented.html#nullprincipal");
   yield runIFrame("frameStorageNullprincipal.sjs");
   document.querySelector('iframe').removeAttribute('sandbox');
 
-  // thirdparty iframes should be blocked, unless they have chrome privileges
+  // thirdparty iframes should be blocked.
   yield runIFrame(thirdparty + "frameStoragePrevented.html");
   yield runIFrame(thirdparty + "frameStorageNullprincipal.sjs");
+  yield runIFrame(thirdparty + "frameStorageChrome.html?allowed=no&blockSessionStorage=yes");
 
   // Workers should be unable to access storage
   yield runWorker("workerStoragePrevented.js");
 });
 
     </script>
   </body>
 </html>
--- a/dom/tests/mochitest/general/test_storagePermissionsRejectForeign.html
+++ b/dom/tests/mochitest/general/test_storagePermissionsRejectForeign.html
@@ -15,28 +15,28 @@ task(function* () {
   yield setCookieBehavior(BEHAVIOR_REJECT_FOREIGN);
 
   // We should be able to access storage
   yield storageAllowed();
 
   // Same origin iframes should be allowed, unless they redirect to a URI with the null principal
   yield runIFrame("frameStorageAllowed.html");
   yield runIFrame("frameStorageNullprincipal.sjs");
-  yield runIFrame("frameStorageChrome.html");
+  yield runIFrame("frameStorageChrome.html?allowed=yes");
 
   // Sandboxed iframes should have the null principal, and thus can't access storage
   document.querySelector('iframe').setAttribute('sandbox', 'allow-scripts');
   yield runIFrame("frameStoragePrevented.html#nullprincipal");
   yield runIFrame("frameStorageNullprincipal.sjs");
   document.querySelector('iframe').removeAttribute('sandbox');
 
-  // thirdparty iframes should be blocked, unless they have chrome privileges
+  // thirdparty iframes should be blocked.
   yield runIFrame(thirdparty + "frameStoragePrevented.html#thirdparty");
   yield runIFrame(thirdparty + "frameStorageNullprincipal.sjs");
-  yield runIFrame(thirdparty + "frameStorageChrome.html");
+  yield runIFrame(thirdparty + "frameStorageChrome.html?allowed=no");
 
   // Workers should be able to access storage
   yield runWorker("workerStorageAllowed.js");
 });
 
     </script>
   </body>
 </html>