Closed
Bug 777467
Opened 12 years ago
Closed 12 years ago
Update the same-origin policy for principals to include appid/isinbrowserelement
Categories
(Core :: Security: CAPS, defect, P2)
Core
Security: CAPS
Tracking
()
Tracking | Status | |
---|---|---|
firefox18 | --- | fixed |
People
(Reporter: mounir, Assigned: mounir)
References
Details
(Whiteboard: [LOE:S][qa-])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
Assignee: nobody → mounir
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S]
Updated•12 years ago
|
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P2]
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #669204 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [LOE:S][WebAPI:P2] → [LOE:S][WebAPI:P2][need review]
Assignee | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
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-
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #669204 -
Attachment is obsolete: true
Attachment #669498 -
Flags: review?(bobbyholley+bmo)
Comment 6•12 years ago
|
||
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-
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #669498 -
Attachment is obsolete: true
Attachment #670338 -
Flags: review?(bobbyholley+bmo)
Comment 8•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S][WebAPI:P2][need review] → [LOE:S][WebAPI:P2][has reviewed patch][needs dependency list to be cleared]
Updated•12 years ago
|
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]
Assignee | ||
Comment 9•12 years ago
|
||
Whiteboard: [LOE:S][has reviewed patch][needs dependency list to be cleared] → [LOE:S]
Target Milestone: --- → mozilla19
Comment 10•12 years ago
|
||
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 → ---
Assignee | ||
Comment 11•12 years ago
|
||
That was a stupid typo.
Re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9f725c9eb5
Target Milestone: --- → mozilla19
Comment 12•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 13•12 years ago
|
||
status-firefox18:
--- → fixed
Updated•12 years ago
|
Whiteboard: [LOE:S] → [LOE:S][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•