Closed Bug 630716 Opened 9 years ago Closed 9 years ago

overly permissive principals comparison in AccessCheck:isSameOrigin


(Core :: XPConnect, defect)

Not set



Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: gal, Assigned: mrbkap)



(Whiteboard: [sg:moderate][hardblocker][has patch])


(1 file, 1 obsolete file)

No description provided.
blocking2.0: --- → betaN+
Whiteboard: [hardblocker]
QA Contact: xpconnect → mrbkap
Attached patch Proposed fix (obsolete) — Splinter Review
The problem here is that when determining whether two *compartments* are same-origin we need to ignore document domain. Basically, this lets the first page in any compartment determine what domain that compartment is actually in.

bz, I'd appreciate a double-check around my handling of null principals. I'm assuming that the only principal with a null URI is the system principal. We really need a better way to compare two principals' equality :(
Assignee: nobody → mrbkap
Attachment #508937 - Flags: review?(bzbarsky)
Blocks: 629331
Whiteboard: [hardblocker] → [hardblocker][has patch]
The "if (!aprin || !bprin)" you added should be "if (!auri || !buri)" right?
Comment on attachment 508937 [details] [diff] [review]
Proposed fix

This is ok, if you fix the null-checks to check auri and buri, but PLEASE file and fix (for landing after 2.0 ships) a bug on adding an equality compare method on principals that ignores document.domain.  We're going to need that all over the place for HTML5, which uses the "ignoring the domain" origin in various places.

In fact, it might be best to figure out what sort of compares we'll need to do to make the various checks HTML5 calls for, then add the corresponding APIs.
Attachment #508937 - Flags: review?(bzbarsky) → review+
Calling this "moderate" rather than "high" because the two domains in question are evidently cooperative in some cases if they've both set document.domain to the same value.
Whiteboard: [hardblocker][has patch] → [sg:moderate][hardblocker][has patch]
Attached patch Fixed fixSplinter Review
With attachment 508937 [details] [diff] [review] applied, facebook had some apparently minor problems with the instant messaging stuff. A bunch of debugging later showed that the problem was that location objects weren't respecting document.domain with the old patch. This fixes location objects as well as the original bug (and the aprin/bprin -> auri/buri typo). bz, I'm carrying forward your review on this and looking for gal's review on the location object fix.
Attachment #508937 - Attachment is obsolete: true
Attachment #509558 - Flags: review?(gal)
Oh, and the latter patch passes try.
Attachment #509558 - Flags: review?(gal) → review+
Fix landed:
Closed: 9 years ago
Resolution: --- → FIXED
Are there any STR for this issue?
I can't think of anything that would be visible outside a debugger.
Not accessible to reporter
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.