1145870 - Pwn2Own bug still exploitable in 36.0.3

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mariusz Mlynski]

Attachment: 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.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That check was added in bug 1116977, which is on 37 and 38, but not 36.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Taking per jst's request.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: moz-poc2.html

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: moz-poc2.html

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That bit about ESR belong in bug 1144988, because even the original issue is not fixed there.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: XSS POC variant for ESR31

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.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ESR and 36 pass dbaron's testcase in this bug with the patch.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 13

[Boris Zbarsky [: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 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

For trunk

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for trunk

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8581026 [details] [diff] [review]
Patch for trunk

r=me

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-release/rev/57cc76236bd7
http://hg.mozilla.org/releases/mozilla-esr31/rev/aef3a81c9624

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And on default

http://hg.mozilla.org/releases/mozilla-esr31/rev/854698de78a2
http://hg.mozilla.org/releases/mozilla-esr31/rev/72dd5f988428

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 21

[u279076]

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.

Comment 22

[u279076]

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.

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

Tested Android ARM and Android x86 binaries. Both reproduce on 36.0.3. Verified fixed on 36.0.4.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7eb9be5ed3f

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b7eb9be5ed3f

Comment 26

[Daniel Veditz [:dveditz]]

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.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes.  Not sure why it wasn't marked fixed when it was merged to m-c.

Comment 28

[Phil Ringnalda (:philor)]

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 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8581026 [details] [diff] [review]
Patch for trunk

We need this patch on all branches. Beta+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0725e4cfa3c3
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/acfc2a472b6f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/3ddabfe0bb06

Comment 32

[Christiane Ruetten [INACTIVE]]

Al, what's the Firefox advisory status for this?

Comment 33

[Christiane Ruetten [INACTIVE]]

Found it covered in bug 1146339.