Closed
Bug 1031145
Opened 10 years ago
Closed 9 years ago
URL Spoofing using onbeforeunload dialog
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
RESOLVED
FIXED
Firefox 41
People
(Reporter: moz_bug_r_a4, Assigned: Gijs)
References
Details
(Keywords: sec-high)
Attachments
(4 files, 1 obsolete file)
3.07 KB,
text/html
|
Details | |
3.76 KB,
text/html
|
Details | |
710 bytes,
text/html
|
Details | |
11.33 KB,
patch
|
mconley
:
review+
Sylvestre
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
This is almost the same as bug 700080, but this uses an onbeforeunload dialog instead of an alert dialog in order to circumvent the fix.
Also, this bug can cause a crash on fx32 and fx33.
Reporter | ||
Comment 1•10 years ago
|
||
This works on fx30-fx33. But, a form element does not work with a physical keyboard on fx24.6.0esr.
Reporter | ||
Comment 2•10 years ago
|
||
This works on fx24.6.0esr, fx30-fx33.
Reporter | ||
Comment 3•10 years ago
|
||
This works on fx32 and fx33.
Comment 4•10 years ago
|
||
The crash is a null pointer crash.
http://mxr.mozilla.org/mozilla-aurora/source/dom/base/nsGlobalWindow.cpp?rev=042efa075740&mark=2406-2406#2400 mContext is null.
Comment 5•10 years ago
|
||
I filed bug 1031303 for the null pointer crash.
Comment 6•10 years ago
|
||
Spoofing part looks like a Firefox UI bug.
Component: Security → Tabbed Browser
Product: Core → Firefox
Comment 7•10 years ago
|
||
needinfo-ping, Gavin can you help find someone to take this bug?
Flags: needinfo?(gavin.sharp)
Comment 8•10 years ago
|
||
Will add it to our backlog.
Flags: needinfo?(gavin.sharp) → firefox-backlog+
Comment 9•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #8)
> Will add it to our backlog.
Is there a timeline on this backlog because making a backlog+ flag scares me. :-)
Comment 10•10 years ago
|
||
No, there's no timeline guarantee. Given that it was internally reported, has no evidence of use in the wild, requires some user interaction, shows an unexpected dialog, and is tricky to fix, it's having a hard time float to the top of our priority list. Arguably sec-high overstates its impact. I want to fix it, but I want to fix a lot of other things too.
Updated•10 years ago
|
Group: firefox-core-security
Comment 11•9 years ago
|
||
Dave, is there somebody who can work on this sec-high bug? It is more than a year old with no sign of anybody having worked on it. Thanks.
Flags: needinfo?(dcamp)
Assignee | ||
Comment 12•9 years ago
|
||
I can't reproduce this on current fx40. Not the crash or the spoofing. The new page that opens just closes.
I can see oddness (specifically: tabs that don't show up in the tabbar but are somehow still there in the tab count -- but no crash!) with the crash testcase on fx38.
Am I missing something?
Flags: needinfo?(moz_bug_r_a4)
Flags: needinfo?(continuation)
Assignee | ||
Comment 13•9 years ago
|
||
Ah, and I can reproduce the spoofing on 38. I expect some of the other beforeunload work that we did fixed this... the question will be "what", specifically. I'll do a bisection later today.
Flags: needinfo?(dcamp) → needinfo?(gijskruitbosch+bugs)
Updated•9 years ago
|
Flags: needinfo?(continuation)
Assignee | ||
Comment 14•9 years ago
|
||
Fixed by bug 1162860, looks like. (m-c pushlog was https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0f1d42a6745a&tochange=1a8343f8ed83 )
I will see if I can get a patch for esr38 if that seems necessary?
Assignee | ||
Updated•9 years ago
|
status-firefox39:
--- → fixed
status-firefox40:
--- → fixed
status-firefox41:
--- → fixed
status-firefox42:
--- → fixed
status-firefox-esr31:
--- → wontfix
status-firefox-esr38:
--- → affected
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 15•9 years ago
|
||
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: n/a - sec-high
User impact if declined: spoofing, sadness, etc.
Fix Landed on Version: 40, uplifted to 39, has baked for a long time now
Risk to taking this patch (and alternatives if risky): very low considering baking time and lack of reported regressions
String or UUID changes made by this patch: no.
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(moz_bug_r_a4)
Attachment #8642948 -
Flags: review+
Attachment #8642948 -
Flags: approval-mozilla-esr38?
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #15)
> Fix Landed on Version: 40, uplifted to 39, has baked for a long time now
Err, correction, landed on 41, uplifted to 40 and 39 (but pretty early on in 39's beta cycle, so I'm still pretty happy with bake time here).
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Fix Landed on Version: 40, uplifted to 39, has baked for a long time now
>
> Err, correction, landed on 41, uplifted to 40 and 39 (but pretty early on in
> 39's beta cycle, so I'm still pretty happy with bake time here).
Sigh. And the test in this patch is breaking on esr38 because even more of BrowserTestUtils is missing on esr38. That is, it was uplifted, but the related code changes weren't, and so there are no errors but it ends up listening for events (like TabSwitchDone) that the tabbrowser on 38 never sends.
We can still take this patch on esr38 with no risk impact or anything - but we shouldn't take the automated test because it will just fail.
I can try to make that existing test work on esr38 but I don't know that that's a good use of my time right now. Let me know if people feel differently and/or I'm missing something that would change the importance of that work.
I've verified separately that the patch fixes the PoCs that are attached to this bug when applied on esr38.
Assignee | ||
Comment 18•9 years ago
|
||
I changed my mind because of how long esr38 is going to be around, and wanting to make it easier to uplift stuff... Mike, does this look righteous to you?
Attachment #8642948 -
Attachment is obsolete: true
Attachment #8642948 -
Flags: approval-mozilla-esr38?
Attachment #8643086 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Attachment #8643086 -
Flags: approval-mozilla-esr38?
Comment 19•9 years ago
|
||
Comment on attachment 8643086 [details] [diff] [review]
Patch with test fixes
Review of attachment 8643086 [details] [diff] [review]:
-----------------------------------------------------------------
r=me given a green try run, though I'm curious about some of the changes to BrowserTestUtils.
::: browser/base/content/tabbrowser.xml
@@ +5114,1 @@
> <field name="closing">false</field>
Shouldn't we just remove this instead of commenting it out?
::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +137,5 @@
> * @resolves When a load event is triggered for the browser.
> */
> browserLoaded(browser, includeSubFrames=false) {
> return new Promise(resolve => {
> + let mm = browser.ownerDocument.defaultView.messageManager;
I don't fully understand this change. You've changed from listening from a particular browser, to listening to the entire window for the browser-test-utils:loadEvent message. I understand that you're filtering on msg.target == browser, but why is this necessary?
@@ +335,5 @@
> */
> synthesizeMouseAtPoint(offsetX, offsetY, event, browser)
> {
> return BrowserTestUtils.synthesizeMouse(null, offsetX, offsetY, event, browser);
> },
I guess no other tests on 38 rely on this?
Attachment #8643086 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19)
> Comment on attachment 8643086 [details] [diff] [review]
> Patch with test fixes
>
> Review of attachment 8643086 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me given a green try run, though I'm curious about some of the changes to
> BrowserTestUtils.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35756ab3989
> ::: browser/base/content/tabbrowser.xml
> @@ +5114,1 @@
> > <field name="closing">false</field>
>
> Shouldn't we just remove this instead of commenting it out?
Note that this is already on 39+, I'm just backporting. I wanted to leave it so we don't end up re-adding it and re-breaking stuff.
> ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
> @@ +137,5 @@
> > * @resolves When a load event is triggered for the browser.
> > */
> > browserLoaded(browser, includeSubFrames=false) {
> > return new Promise(resolve => {
> > + let mm = browser.ownerDocument.defaultView.messageManager;
>
> I don't fully understand this change. You've changed from listening from a
> particular browser, to listening to the entire window for the
> browser-test-utils:loadEvent message. I understand that you're filtering on
> msg.target == browser, but why is this necessary?
This is to match current trunk. I expect there are cases where it raced with the content being created.
> @@ +335,5 @@
> > */
> > synthesizeMouseAtPoint(offsetX, offsetY, event, browser)
> > {
> > return BrowserTestUtils.synthesizeMouse(null, offsetX, offsetY, event, browser);
> > },
>
> I guess no other tests on 38 rely on this?
I'm assuming you mean the removeTab removal? Yeah, that is also backported from the original beta branch patch, and the trypush is green, so presumably nobody used it. In any case the implementation is broken because it relies on session store stuff that isn't there on 38.
Comment 21•9 years ago
|
||
Sound arguments. Fire when ready.
Comment 22•9 years ago
|
||
Comment on attachment 8643086 [details] [diff] [review]
Patch with test fixes
Let's take it, it should be in esr 38.3.0.
Attachment #8643086 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8643086 [details] [diff] [review]
Patch with test fixes
Al, how do you want me to go about landing this? Quick summary:
this patch landed on 41, was uplifted to 39 and 40 as a non-security correctness fix for bug 1162860. Then we found that it fixed this security bug, and so now I wrote a backport to 38. Should I wait with landing this and get sec-approval, or just land it as a plain uplift of 1162860 and pretend nothing else is going on, or...?
Sec-approval form just in case:
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The code fix is tiny and innocuous. The comment and the test change probably give a bunch away though.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes and no - yes, see above. No, because we landed this as a non-security fix. But then, if we're uplifting to ESR, people will presumably (rightly) assume that the "wrong decisions" that the comment talks about have serious consequences.
Which older supported branches are affected by this flaw?
Just 38 at this point.
If not all supported branches, which bug introduced the flaw?
n/a
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
n/a
How likely is this patch to cause regressions; how much testing does it need?
Pretty unlikely, considering it's now baked for months and been through 1 release without serious problems that I know of, and has automated tests.
Flags: needinfo?(abillings)
Attachment #8643086 -
Flags: sec-approval?
Comment 24•9 years ago
|
||
Comment on attachment 8643086 [details] [diff] [review]
Patch with test fixes
Just land it on ESR38. I think we're fine.
The only thing I might hold off on is the test but if you've already landed the test, then just land the whole thing.
Flags: needinfo?(abillings)
Attachment #8643086 -
Flags: sec-approval? → sec-approval+
Comment 25•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
fixed → ---
tracking-firefox-esr38:
--- → ?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Kent James (:rkent) from bug 1184009 comment #29)
> I'm trying to decide whether to take this bug, and also bug 1031145 (which I
> am not authorized to view) in Thunderbird 38.2.0 Normally we would to match
> our code as closely as possible to the Firefox esr38.2.0 release, which has
> already shipped.
>
> Is there any expectation of doing a FF 38.2.1 due to this or the other bug?
> Thoughts on whether this might be important for Thunderbird?
I don't expect this bug is important for Thunderbird, but I've CC'd you. From what I've read, I don't think we'll be spinning 38.2.1 for it, but I could be wrong.
Updated•9 years ago
|
Group: firefox-core-security
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Updated•9 years ago
|
tracking-firefox-esr38:
41+ → ---
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•