Closed Bug 1145870 Opened 5 years ago Closed 5 years ago
Pwn2Own bug still exploitable in 36
2.44 KB, patch
|Details | Diff | Splinter Review|
1.04 KB, patch
|Details | Diff | Splinter Review|
36.0.3 you're about to ship is still exploitable. In the release channel, nsDocShell::OnLinkClick* methods don't check for IsNavigationAllowed, so the fix for bug 1144988 can be bypassed.
That check was added in bug 1116977, which is on 37 and 38, but not 36.
Taking per jst's request.
Assignee: nobody → khuey
(In reply to Kyle Huey [:khuey] (email@example.com) from comment #4) > Created attachment 8580977 [details] > moz-poc2.html As far as mozilla-central: this shows an alert in yesterday's nightly but does not show an alert in today's nightly, so this is fixed on mozilla-central within the range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf1060d8ce9f&tochange=4d2d97b3ba34 which I believe is as expected.
I tested the new exploit on 36.0.3 and it reproduces. After applying the patch from bug 1116977 it does not. Two questions worth thinking about: 1. Is there any regression risk from that patch? We're about to ship it in a week anyways, so ... 2. Are there other codepaths that also need this check?
(In reply to Kyle Huey [:khuey] (firstname.lastname@example.org) from comment #6) > 2. Are there other codepaths that also need this check? One data point here is that we both looked over the codepaths that lead to InternalLoad, and the only one that doesn't go through IsNavigationAllowed is LoadErrorPage. Is that OK?
khuey points out that ESR31 may also need bug 1046022, which introduced another IsNavigationAllowed check. I double-checked all the InternalLoad callers on both mozilla-release and mozilla-esr31, and didn't find any others that were missing IsNavigationAllowed checks (other than LoadErrorPage above, bug 1046022 needed for ESR31, and the ones fixed by bug 1116977 needed for 36 and ESR31).
That bit about ESR belong in bug 1144988, because even the original issue is not fixed there.
Since khuey pointed out the MessageChannel test isn't valid in ESR31, here's a variant of the PoC here that just uses sync XHR to "steal" the source code to pdf.js. I believe this shouldn't work, but it does alert the source code for me against ESR31 tip.
ESR and 36 pass dbaron's testcase in this bug with the patch.
Discussed this with Kyle and dbaron. We're going to add explicit checks for this new boolean on all branches (but not full IsNavigationAllowed() calls) at the places where trunk calls IsNavigationAllowed(). We're also going to check this boolean in InternalLoad, just in case; that should alleviate worries about LoadErrorPage. All loads do go through InternalLoad, so that fallback check should cover everything, in a pinch.
Comment on attachment 8581021 [details] [diff] [review] Patch r=me if we also have a trunk patch with just the InternalLoad change and a third patch with the additional ESR change you mentioned.
Attachment #8581021 - Flags: review?(bzbarsky) → review+
Comment on attachment 8581026 [details] [diff] [review] Patch for trunk r=me
Attachment #8581026 - Flags: review?(bzbarsky) → review+
For QA, the failure case for dbaron's testcase is an alert that says "The page at resource://pdf.js says:" and has some pdf.js source code in it. The success case is no alert. The testcase should fail before the changes in this bug and pass afterwards on release and esr31.
Here are my results from testing this on the new candidate builds: * Firefox 31.5.3esr en-US shows no alert * Firefox 31.5.2esr en-US shows an alert with some pdf.js code * Firefox 36.0.4 en-US shows no alert * Firefox 36.0.3 en-US show an alert with some pdf.js code * Firefox 36.0.1 en-US show an alert with some pdf.js code Based on these results I'm marking this verified fixed for 36 and 31esr.
Lawrence asked me to confirm this as well on our test branches. * Firefox Nightly 39.0a1 2015-03-21 shows no alert * Firefox Aurora 38.0a2 2015-03-21 shows no alert * Firefox Beta 37.0b7 shows no alert I'm leaving those branches marked unaffected based on these results.
Tested Android ARM and Android x86 binaries. Both reproduce on 36.0.3. Verified fixed on 36.0.4.
Attachment #8581026 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Should this bug be "fixed" now that it's landed on branches and m-c? I don't see any comments indicating we're working on a "better" replacement fix.
Yes. Not sure why it wasn't marked fixed when it was merged to m-c.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Because it's marked "status-firefox39: unaffected" so the merge to m-c was a no-op, fixed would have been when you landed it on 36.
Comment on attachment 8581026 [details] [diff] [review] Patch for trunk We need this patch on all branches. Beta+
Attachment #8581026 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b615089f60c4 https://hg.mozilla.org/releases/mozilla-beta/rev/0725e4cfa3c3 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/acfc2a472b6f https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3ddabfe0bb06 https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/bfcc1ebed71e
Target Milestone: --- → mozilla39
Al, what's the Firefox advisory status for this?
Found it covered in bug 1146339.
Whiteboard: [b2g-adv-main2.2?] → [b2g-adv-main2.2-]
You need to log in before you can comment on or make changes to this bug.