Closed
Bug 984467
Opened 10 years ago
Closed 10 years ago
Should nsGlobalWindow::CallerInnerWindow distinguish between "not nsISupports" and "not nsGlobalWindow"?
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: peterv, Assigned: bholley, Mentored)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main35-][b2g-adv-main2.2-])
Attachments
(1 file, 3 obsolete files)
4.59 KB,
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta-
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
The difference was introduced in bug 414743, but it's not really clear why that didn't just always return GetCurrentInnerWindowInternal().
Updated•10 years ago
|
Flags: needinfo?(mrbkap)
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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++]
Comment 4•10 years ago
|
||
I'm willing to work on this bug. Could you provide me few pointers?
Flags: needinfo?(bzbarsky)
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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)
Comment 8•10 years ago
|
||
Assignee: nobody → anujagarwal464
Attachment #8408413 -
Flags: feedback?(bzbarsky)
Comment 9•10 years ago
|
||
Comment on attachment 8408413 [details] [diff] [review] bug984467.diff Please do simplify the code per comment 5?
Attachment #8408413 -
Flags: feedback?(bzbarsky) → feedback+
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=84a9a3ae8068
Comment 11•10 years ago
|
||
Er, the link in comment 10 is unrelated to this bug. ;)
Comment 12•10 years ago
|
||
Attachment #8408767 -
Flags: review?(bzbarsky)
Comment 13•10 years ago
|
||
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+
Comment 14•10 years ago
|
||
Attachment #8408413 -
Attachment is obsolete: true
Attachment #8408767 -
Attachment is obsolete: true
Attachment #8408779 -
Flags: review?(bzbarsky)
Comment 15•10 years ago
|
||
Comment on attachment 8408779 [details] [diff] [review] bug984467.diff r=me
Attachment #8408779 -
Flags: review?(bzbarsky) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/df8e7a4b86cf
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Backed out for mochitest-other failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/d69cced6e242 https://tbpl.mozilla.org/php/getParsedLog.php?id=38093479&tree=Mozilla-Inbound
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
(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)
Comment 20•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
(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.
Assignee | ||
Comment 22•10 years ago
|
||
(And in particular, if we _do_ use such a fallback, it should only be in the IsCallerChrome() case).
Comment 23•10 years ago
|
||
> 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.
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Comment 26•10 years ago
|
||
Bobby, what sort of security rating should this get?
Flags: needinfo?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bobbyholley)
Keywords: sec-moderate
Updated•10 years ago
|
Mentor: bzbarsky
Whiteboard: [mentor=bzbarsky@mit.edu][lang=c++] → [lang=c++]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [lang=c++]
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8519193 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 28•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ff97aad4385d
Assignee | ||
Updated•10 years ago
|
Attachment #8408779 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: anujagarwal464 → bobbyholley
Comment 29•10 years ago
|
||
Comment on attachment 8519193 [details] [diff] [review] Remove current inner fallback in CallerInnerWindow. v1 r=me
Attachment #8519193 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ec124fcdbb
Assignee | ||
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
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-
Comment 33•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/55ec124fcdbb
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox36:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 34•10 years ago
|
||
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+
Comment 35•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9d115d5d0a37
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
status-firefox34:
--- → wontfix
status-firefox35:
--- → fixed
status-firefox-esr31:
--- → wontfix
Comment 36•10 years ago
|
||
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)
Assignee | ||
Comment 37•10 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8519193 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 39•9 years ago
|
||
Bustage follow-up: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/55b024bb551d
Updated•9 years ago
|
Whiteboard: [adv-main35-]
Updated•9 years ago
|
Whiteboard: [adv-main35-] → [adv-main35-][b2g-adv-main2.2-]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•