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.

This works on fx30-fx33. But, a form element does not work with a physical keyboard on fx24.6.0esr.

This works on fx24.6.0esr, fx30-fx33.

This works on fx32 and fx33.

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.

I filed bug 1031303 for the null pointer crash.

Spoofing part looks like a Firefox UI bug.

needinfo-ping, Gavin can you help find someone to take this bug?

Will add it to our backlog.

(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. :-)

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.

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.

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?

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.

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?

[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.

(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).

(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.

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?

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?

(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.

Sound arguments. Fire when ready.

Comment on attachment 8643086 [details] [diff] [review] Patch with test fixes Let's take it, it should be in esr 38.3.0.

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.

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.

(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.