Closed Bug 777467 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Whiteboard: [LOE:S][qa-])

Attachments

(1 file, 2 obsolete files)

No description provided.
blocking-basecamp: ? → +
Assignee: nobody → mounir
Whiteboard: [LOE:S]
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P2]
Depends on: 789224
Depends on: 799152
Attached patch Patch (obsolete) — Splinter Review
Attachment #669204 - Flags: review?(bobbyholley+bmo)
Status: NEW → ASSIGNED
Whiteboard: [LOE:S][WebAPI:P2] → [LOE:S][WebAPI:P2][need review]
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.
Attachment #669204 - Flags: review?(bobbyholley+bmo) → review-
(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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #669204 - Attachment is obsolete: true
Attachment #669498 - Flags: review?(bobbyholley+bmo)
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.
Attachment #669498 - Flags: review?(bobbyholley+bmo) → review-
Attached patch PatchSplinter Review
Attachment #669498 - Attachment is obsolete: true
Attachment #670338 - Flags: review?(bobbyholley+bmo)
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
Attachment #670338 - Flags: review?(bobbyholley+bmo) → review+
Whiteboard: [LOE:S][WebAPI:P2][need review] → [LOE:S][WebAPI:P2][has reviewed patch][needs dependency list to be cleared]
Priority: -- → P2
Whiteboard: [LOE:S][WebAPI:P2][has reviewed patch][needs dependency list to be cleared] → [LOE:S][has reviewed patch][needs dependency list to be cleared]
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb62d8b9800a
Whiteboard: [LOE:S][has reviewed patch][needs dependency list to be cleared] → [LOE:S]
Target Milestone: --- → mozilla19
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
Target Milestone: mozilla19 → ---
That was a stupid typo.
Re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9f725c9eb5
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/5e9f725c9eb5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [LOE:S] → [LOE:S][qa-]
You need to log in before you can comment on or make changes to this bug.