Closed Bug 1145870 Opened 5 years ago Closed 5 years ago

Pwn2Own bug still exploitable in 36.0.3

Categories

(Core :: DOM: Navigation, defect)

36 Branch
All
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 + verified
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: marius.mlynski, Assigned: khuey)

References

Details

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

Attachments

(2 files, 2 obsolete files)

Attached file common.js
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] (khuey@mozilla.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.
Flags: sec-bounty?
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #8580983 - Flags: review?(bzbarsky)
Attachment #8580983 - Flags: approval-mozilla-release?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Attached patch PatchSplinter Review
Attachment #8580983 - Attachment is obsolete: true
Attachment #8580983 - Flags: review?(bzbarsky)
Attachment #8580983 - Flags: approval-mozilla-release?
Attachment #8580983 - Flags: approval-mozilla-esr31?
Attachment #8581021 - Flags: review?(bzbarsky)
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+
Attached patch Patch (obsolete) — Splinter Review
For trunk
Attachment #8581024 - Flags: review?(bzbarsky)
Attachment #8581021 - Flags: approval-mozilla-release?
Attachment #8581021 - Flags: approval-mozilla-esr31?
Attachment #8581024 - Attachment is obsolete: true
Attachment #8581024 - Flags: review?(bzbarsky)
Comment on attachment 8581026 [details] [diff] [review]
Patch for trunk

r=me
Attachment #8581026 - Flags: review?(bzbarsky) → review+
Attachment #8581021 - Flags: approval-mozilla-release?
Attachment #8581021 - Flags: approval-mozilla-release+
Attachment #8581021 - Flags: approval-mozilla-esr31?
Attachment #8581021 - Flags: approval-mozilla-esr31+
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-beta?
Attachment #8581026 - Flags: approval-mozilla-aurora?
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+
Flags: sec-bounty? → sec-bounty+
Al, what's the Firefox advisory status for this?
Flags: needinfo?(abillings)
Whiteboard: [b2g-adv-main2.2?]
Found it covered in bug 1146339.
Flags: needinfo?(abillings)
Whiteboard: [b2g-adv-main2.2?] → [b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.