Closed Bug 984467 Opened 6 years ago Closed 5 years ago

Should nsGlobalWindow::CallerInnerWindow distinguish between "not nsISupports" and "not nsGlobalWindow"?

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- wontfix
b2g-v1.4 --- unaffected
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: peterv, Assigned: bholley, Mentored)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main35-][b2g-adv-main2.2-])

Attachments

(1 file, 3 obsolete files)

Currently nsGlobalWindow::CallerInnerWindow returns null if the scope doesn't hold an nsISupports and it returns GetCurrentInnerWindowInternal() if the scope holds an nsISupports that's not a nsGlobalWindow. It's not clear why we want to treat these two cases differently. We should figure out if we can just treat them the same.
The difference was introduced in bug 414743, but it's not really clear why that didn't just always return GetCurrentInnerWindowInternal().
Flags: needinfo?(mrbkap)
After doing some investigation, I think this was an oversight on my part (also, I wonder why I was trying to minimize the patch size in that bug). We probably should return GetCurrentInnerWindowInternal in both cases.
Flags: needinfo?(mrbkap)
OK.  Then we should be able to just xpc::WindowOrNull() and if null use the current inner.  See bug 982114 comment 1.
Whiteboard: [mentor=bzbarsky@mit.edu][lang=c++]
I'm willing to work on this bug. Could you provide me few pointers?
Flags: needinfo?(bzbarsky)
Sure.  If you look in nsGlobalWindow.cpp, the CallerInnerWindow function returns null if !notive but if the native is not an nsPIDOMWindow it returns the current inner.

This bug is about getting rid of that difference, as described in comment 2.  And in particular, if all we care about is whether we have a window we can replace the GetNativeOfWrapper + QI bit with xpc::WindowOrNull(scope).

Does that help?
Flags: needinfo?(bzbarsky) → needinfo?(anujagarwal464)
(In reply to Boris Zbarsky [:bz] from comment #5)

> This bug is about getting rid of that difference, as described in comment 2.
> And in particular, if all we care about is whether we have a window we can
> replace the GetNativeOfWrapper + QI bit with xpc::WindowOrNull(scope).
> 
> Does that help?

So in both the cases I should return GetCurrentInnerWindowInternal().
Flags: needinfo?(anujagarwal464) → needinfo?(bzbarsky)
Yes.
Flags: needinfo?(bzbarsky)
Attached patch bug984467.diff (obsolete) — Splinter Review
Assignee: nobody → anujagarwal464
Attachment #8408413 - Flags: feedback?(bzbarsky)
Comment on attachment 8408413 [details] [diff] [review]
bug984467.diff

Please do simplify the code per comment 5?
Attachment #8408413 - Flags: feedback?(bzbarsky) → feedback+
Er, the link in comment 10 is unrelated to this bug.  ;)
Attached patch bug984467.diff -clean (obsolete) — Splinter Review
Attachment #8408767 - Flags: review?(bzbarsky)
Comment on attachment 8408767 [details] [diff] [review]
bug984467.diff -clean

>+  if(!win)

Please reinstate the space before '(' there.

r=me with that fixed.
Attachment #8408767 - Flags: review?(bzbarsky) → review+
Attached patch bug984467.diff (obsolete) — Splinter Review
Attachment #8408413 - Attachment is obsolete: true
Attachment #8408767 - Attachment is obsolete: true
Attachment #8408779 - Flags: review?(bzbarsky)
Comment on attachment 8408779 [details] [diff] [review]
bug984467.diff

r=me
Attachment #8408779 - Flags: review?(bzbarsky) → review+
Bobby, does that test need adjusting, or do we really need to distinguish between "nsISupports but not window" and "not nsISupports" here?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] from comment #18)
> Bobby, does that test need adjusting, or do we really need to distinguish
> between "nsISupports but not window" and "not nsISupports" here?

Probably not, but IMO we shouldn't be falling back to GetCurrentInnerWindowInternal at all. That seems like a total security bug, right?

Marking this bug as protected while we sort this out.
Group: core-security
Flags: needinfo?(bobbyholley)
Well, you need a fallback if you want to allow a JS component to postMessage or setTimeout on on content window.  That's why this fallback was added, back in bug 414743.

I don't see why this is a security bug, as long as untrusted code doesn't get to execute in scopes with no window.
(In reply to Boris Zbarsky [:bz] from comment #20)
> Well, you need a fallback if you want to allow a JS component to postMessage
> or setTimeout on on content window.  That's why this fallback was added,
> back in bug 414743.

Why do we need a fallback? We have code that handles the null case, both in nsGlobalWindow::PostMessageMoz and in JS. It seems like assuming that the target was the caller is strictly worse than returning null.

> I don't see why this is a security bug, as long as untrusted code doesn't
> get to execute in scopes with no window.

This happens all the time with Sandboxes.
(And in particular, if we _do_ use such a fallback, it should only be in the IsCallerChrome() case).
> Why do we need a fallback?

Ah, I see PostMessage handles a null return.  But nsGlobalWindow::InnerForSetTimeoutOrInterval and nsGlobalWindow::SetTimeoutOrInterval certainly do not.

> This happens all the time with Sandboxes.

Hmm.  Ok, that's bad.

> it should only be in the IsCallerChrome() case

_That_ I can buy.
That said, in the postMessage case the difference in behavior is what the origin is in the event the page gets (the page itself vs empty) right?  So maybe the fallback should only be in the timeout code.
(In reply to Boris Zbarsky [:bz] from comment #24)
> That said, in the postMessage case the difference in behavior is what the
> origin is in the event the page gets (the page itself vs empty) right?  So
> maybe the fallback should only be in the timeout code.

The code in InnerForSetTimeoutOrInterval seems pretty insane to me, but maybe I don't understand it. Anyway, that seems strictly better, yes.
Bobby, what sort of security rating should this get?
Flags: needinfo?(bobbyholley)
Flags: needinfo?(bobbyholley)
Keywords: sec-moderate
Mentor: bzbarsky
Whiteboard: [mentor=bzbarsky@mit.edu][lang=c++] → [lang=c++]
Whiteboard: [lang=c++]
Attachment #8408779 - Attachment is obsolete: true
Assignee: anujagarwal464 → bobbyholley
Comment on attachment 8519193 [details] [diff] [review]
Remove current inner fallback in CallerInnerWindow. v1

r=me
Attachment #8519193 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519193 [details] [diff] [review]
Remove current inner fallback in CallerInnerWindow. v1

Nominating this for aurora and beta along with bug 1092388, because I fixed and landed them together. Fixing this on esr31 is likely to be more work because WebIDL window changed all of this code in FF32.
Attachment #8519193 - Flags: approval-mozilla-beta?
Attachment #8519193 - Flags: approval-mozilla-aurora?
Comment on attachment 8519193 [details] [diff] [review]
Remove current inner fallback in CallerInnerWindow. v1

I think we should take this fix and that for bug 1092388 in 35. It's simply too late to take anything less than a critical fix in 34. The description of the exposure in bug 1092388 is "so-so". Please let me know if you think I've missed something here and you think we really need to take this fix in 34. Beta-
Attachment #8519193 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/55ec124fcdbb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8519193 [details] [diff] [review]
Remove current inner fallback in CallerInnerWindow. v1

Let's get this on Aurora.
Attachment #8519193 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Bobby, is this something we're going to want on the B2G release branches as well once this is approved for esr31? As a sec-moderate, it'll need explicit b2g32 and b2g34 approval to do so. FWIW, it appears to apply to both branches without issue.
Flags: needinfo?(bobbyholley)
Comment on attachment 8519193 [details] [diff] [review]
Remove current inner fallback in CallerInnerWindow. v1

(In reply to [Away 18-Nov to 23-Nov] Ryan VanderMeulen [:RyanVM UTC-5] from comment #36)
> Bobby, is this something we're going to want on the B2G release branches as
> well once this is approved for esr31? As a sec-moderate, it'll need explicit
> b2g32 and b2g34 approval to do so. FWIW, it appears to apply to both
> branches without issue.

I think we should probably take it anywhere we take the full fixes for bug 1092388 - so that sounds like b2g34.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding
User impact if declined: potential security bugs
Testing completed: Landed along with bug 1092388 on various branches.
Risk to taking this patch (and alternatives if risky): If we take bug 1092388, I think the lowest risk thing to do is to take this bug as well. We could also just not take this bug, which would be ok too.
String or UUID changes made by this patch: None.
Flags: needinfo?(bobbyholley)
Attachment #8519193 - Flags: approval-mozilla-b2g34?
Attachment #8519193 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Whiteboard: [adv-main35-]
Whiteboard: [adv-main35-] → [adv-main35-][b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.