777467 - Update the same-origin policy for principals to include appid/isinbrowserelement

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 2

[Mounir Lamouri (:mounir)]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=1788ef111e28
https://tbpl.mozilla.org/?tree=Try&rev=44fc7960130b (fixes the M-oth)

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 669204 [details] [diff] [review]
Patch

Can we just write a helper function that returns true if and only if the app id, app status, and mozBrowser bit are exactly equal between two principals? Or do the two checks implemented in this patch differ in some more subtle way?

In general, this patch needs a _big_ comment explaining how these new attributes affect the security relationship between principals.

The test should use the pushPrefEnv/popPrefEnv API, I'd think.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Bobby Holley (:bholley) from comment #3)
> Comment on attachment 669204 [details] [diff] [review]
> Patch
> 
> Can we just write a helper function that returns true if and only if the app
> id, app status, and mozBrowser bit are exactly equal between two principals?
> Or do the two checks implemented in this patch differ in some more subtle
> way?

No, they do not really differ. I actually though of writing such a helper but I've been lazy... :(

> In general, this patch needs a _big_ comment explaining how these new
> attributes affect the security relationship between principals.

Ok, will do that.

> The test should use the pushPrefEnv/popPrefEnv API, I'd think.

This API is broken as soon as you get null values. I wrote a patch to fix it but it broke some tests. Never found the time to fix it after that.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 669498 [details] [diff] [review]
Patch


>+    /**
>+     * Returns true if the two principals share the same "app origin".

I'm not a fan of this name, because this isn't really an origin at all. Let's call them app attributes?

>+     *
>+     * An app origin is defined based on the appId and the inBrowserElement
>+     * flag. Two principals have the same app origin if those information are
>+     * equals.

"If those attributes are equal".

That means that principals with UNKNOWN_APP_ID is only in the
>+     * same "app origin" with a principal if it it's appId is also
>+     * UNKNOWN_APP_ID.
>+     */

I'm looking for a bigger comment here. Specifically one that describes the scenarios and use cases you told me about over IRC, which explain why we need to do these checks. I pored over the B2G wiki pages for over an hour while trying to review this patch and a lot of the subtleties were still lost on me.

>+    static bool
>+    CheckSameAppOriginPrincipal(nsIPrincipal* aSubject,
>+                                nsIPrincipal* aObject);

Let's call this AppAttributesEqual(nsIPrincipal* aFirst, nsIPrincipal* aSecond);

>+  if (!aOther) {
>+    NS_WARNING("Need a principal to compare this to!");
>     return NS_OK;

I'd prefer to just replace this with a MOZ_ASSERT(aOther), or at least throw. It might turn something orange though, so if you're in a hurry you don't have to though.

>diff --git a/caps/src/nsScriptSecurityManager.cpp b/caps/src/nsScriptSecurityManager.cpp

>+    bool subjectUnknownAppId;
>+    aSubject->GetUnknownAppId(&subjectUnknownAppId);
>+
>+    bool objectUnknownAppId;
>+    aObject->GetUnknownAppId(&objectUnknownAppId);
>+
>+    if (subjectUnknownAppId != objectUnknownAppId) {
>+        return NS_ERROR_DOM_PROP_ACCESS_DENIED;
>+    }
>+
>+    uint32_t subjectAppId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
>+    if (!subjectUnknownAppId) {
>+        aSubject->GetAppId(&subjectAppId);
>+    }
>+
>+    uint32_t objectAppId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
>+    if (!objectUnknownAppId) {
>+        aObject->GetAppId(&objectAppId);
>+    }
>+
>+    if (subjectAppId != objectAppId) {
>+        return NS_ERROR_DOM_PROP_ACCESS_DENIED;
>+    }
>+
>+    bool subjectBrowserElementFlag;
>+    aSubject->GetIsInBrowserElement(&subjectBrowserElementFlag);
>+
>+    bool objectBrowserElementFlag;
>+    aObject->GetIsInBrowserElement(&objectBrowserElementFlag);
>+
>+    if (subjectBrowserElementFlag != objectBrowserElementFlag) {
>+        return NS_ERROR_DOM_PROP_ACCESS_DENIED;
>+    }
>+

I believe you want to call the helper here.


>+/* static */ bool
>+nsScriptSecurityManager::CheckSameAppOriginPrincipal(nsIPrincipal* aSubject,
>+                                                     nsIPrincipal* aObject)
>+{
>+    MOZ_ASSERT(aSubject && aObject, "Don't pass null pointers!");
>+
>+    bool subjectUnknownAppId;
>+    aSubject->GetUnknownAppId(&subjectUnknownAppId);
>+
>+    bool objectUnknownAppId;
>+    aObject->GetUnknownAppId(&objectUnknownAppId);
>+
>+    if (subjectUnknownAppId != objectUnknownAppId) {
>+        return false;
>+    }
>+
>+    uint32_t subjectAppId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
>+    if (!subjectUnknownAppId) {
>+        aSubject->GetAppId(&subjectAppId);
>+    }
>+
>+    uint32_t objectAppId = nsIScriptSecurityManager::UNKNOWN_APP_ID;
>+    if (!objectUnknownAppId) {
>+        aObject->GetAppId(&objectAppId);
>+    }
>+    if (!subjectUnknownAppId) {
>+        aSubject->GetAppId(&subjectAppId);
>+    }

This is seriously gross. Can we just add a [notxpcom] getter for the appID on nsIPrincipal that avoids the XPCOM gunk and doesn't assert for UNKNOWN_APP_ID? In fact, if we also add one for IsInBrowserElement, this whole operation would be a one-liner and we could probably skip creating a helper function entirely.


>+addLoadEvent(function() {
>+  // Test the witness frame (we can access same-origin frame).
>+  is(canAccessDocument(frames[0]), true,
>+     "should be able to access the first frame");

is(foo, true) === ok(foo) btw.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 670338 [details] [diff] [review]
Patch

I'm still unhappy about a number of things in this setup, but I don't feel like quibbling about it right now. r=bholley

Comment 9

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb62d8b9800a

Comment 10

[Matt Brubeck (:mbrubeck)]

Sorry, backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eda62c9a13d

because debug builds are crashing in browser_bug592338 at nsPrincipal::GetAppId:
https://tbpl.mozilla.org/php/getParsedLog.php?id=16345716&full=1&branch=mozilla-inbound

Comment 11

[Mounir Lamouri (:mounir)]

That was a stupid typo.
Re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9f725c9eb5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e9f725c9eb5

Comment 13

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/602505260ffb