1146339 - (CVE-2015-0801) A variant of Bug 1144988 lets one bypass same-origin policy

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: moz-poc1.html

Bz got this idea
1) Start a new load.
2) Do an anchor scroll (this does not cancel the load, iirc).
3) Use the mutation event to wait until the load we started completes. 



There are few options to fix this, at least
* Use the original patch for bug 1144988
* Add some more checks to doShortCircuitedLoad so that we just bail out early
  if anchor scrolling changes docshell's state to something unexpected.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or:

* Have docshell fail out attemts to Embed() or something higher up the callstack (basically kill the load if it tries to change what we have loaded) while we're in the "block loads" state.  This should only matter in cases when someone spins the event loop in that state, and I'm comfortable treating that as an attack.

In terms of simplicity and provable correctness, I think this is way better than your second bullet point.  Not sure how it compares to the original patch for bug 1144988.

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Need to decide which releases we want to get this fixed for.

The other thing we need to decide is whether this warrants more chemspills or whether we can just fix this in 37 and 31.6 in the normal course of events.  I seem to recall those shipping pretty darned soon...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Not doing reviews right now from comment #1)
> * Have docshell fail out attemts to Embed() or something higher up the
> callstack (basically kill the load if it tries to change what we have
> loaded) while we're in the "block loads" state.  This should only matter in
> cases when someone spins the event loop in that state, and I'm comfortable
> treating that as an attack.
This makes us open pdf in a new tab which is rather odd.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 2 (check the state after anchor scroll)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> This makes us open pdf in a new tab which is rather odd.

Really?  Even if you cancel the channel?

Comment 6

[Boris Zbarsky [:bzbarsky]]

My problem with approach 2 is that it seems like it's adding complexity and making things harder to reason about.  For example, what happens in the caller once we take that early return?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Not doing reviews right now from comment #5)
> Really?  Even if you cancel the channel?
Didn't cancel. Just threw failure


The right fix is IMO the original patch for bug 1144988, but that is regression prone, so should land to trunk only.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Not doing reviews right now from comment #5) 
> Really?  Even if you cancel the channel?
And which channel actually?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Bz, is there a proof of concept of this security issue or you "just" had the idea following the pwn2own bug?

37 and 31.6 will arrive soon. We, release management, would really like to avoid a new chemspill (our users can be upset and, moreover, this adds a lot of pressure on our teams). So, if we can wait a week for this, we would be super happy!

Thanks

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tracking on all releases anyway.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Didn't cancel. Just threw failure

OK.  What you want to do is in nsDSURIContentListener::DoContent set *aAbortProcess to true (this will prevent the "new tab" bit), cancel the request, and return NS_OK.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Bz, is there a proof of concept of this security issue

Sure.  It's attached right in this bug.  We had the idea on Saturday; it just took a bit of time to produce the POC.

My gut feeling is that waiting a few days extra (less than a week, because it'd take us a day or two to do the chemspill anyway) is find here.  The really big open question is whether someone else will have the same idea once they see the advisories/patches for the thing we're shipping in 36.0.3/4 and company, and how long it takes them to produce a POC after that, and whether they can start using it on our users before we ship 37/31.6.

I'm assuming we _definitely_ want to fix this in 37 and 31.6, of course.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

(In reply to Not doing reviews right now from comment #12)
> > Bz, is there a proof of concept of this security issue
> 
> Sure.  It's attached right in this bug.  We had the idea on Saturday; it
> just took a bit of time to produce the POC.
>
Sorry, I meant "outside of this bug" :)

So, I propose we take the chance to wait until next week and 37.
If we see an exploit published, we could chemspill again.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Sorry, I meant "outside of this bug" :)

Ah.  In that case, not that we know of.

Comment 15

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

The 37 RC is building today. We preferably need a fix today or tomorrow at the latest to avoid potential impact on the relasese date. We will spin another RC to pick this up if it isn't ready today.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: approach 1

Comment 17

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

Looks like Olli is taking this one.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for esr31

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8581768 [details] [diff] [review]
approach 1

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8581853 [details] [diff] [review]
for esr31

r=me

Comment 21

[Benjamin Kerensa [:bkerensa]]

Do you want to nominate this for uplift to Aurora/Beta/ESR?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I need still beta patch. I think we want this there.

Comment 23

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

(In reply to Olli Pettay [:smaug] from comment #22)
> Hmm, I need still beta patch. I think we want this there.

Absolutely. If this is done today, we should be able to still get our mobile beta8 built as scheduled. If not, we'll build tomorrow instead. (Not a huge deal to build tomorrow. I'd rather take a more thought through patch if you need more time.)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

The patch is ready, just compiling and testing.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for beta

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8581768 [details] [diff] [review]
approach 1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I wouldn't say it is easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message should be
"Bug 1146339, do anchor scrolling right before dispatching popstate/hashchange, r=bz"

...which shouldn't bulls-eye on the issue


Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The backports just deal with different indentation and some extra mBlockNavigation checks.

How likely is this patch to cause regressions; how much testing does it need?
This is docshell, so somewhat risky. This is one of the areas where no specification defines exactly how browsers should behave.
But in principle the patch makes the relevant code easier to maintain and less error prone.


[String/UUID change made/needed]:
NA

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8581853 [details] [diff] [review]
for esr31

[Approval Request Comment]
See comment 26.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8582069 [details] [diff] [review]
for beta

Approval Request Comment
See comment 26.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8581768 [details] [diff] [review]
approach 1

sec-approval+ for trunk.

Comment 30

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

Comment on attachment 8582069 [details] [diff] [review]
for beta

We want this fix on all branches.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

I really can't deal with landing tonight. 3am+ here.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee9c3fce092

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04dc5ff4b2a9
https://hg.mozilla.org/releases/mozilla-beta/rev/4d306a83ae1b
https://hg.mozilla.org/releases/mozilla-esr31/rev/d8d56cdaa221

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/4d306a83ae1b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4d306a83ae1b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2a05cd42088b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/2a05cd42088b
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/543c2325d667
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/543c2325d667
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d648d0af04c9

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ee9c3fce092

Comment 36

[Kevin Brosnan [Ex-Mozilla]]

verified - Firefox for Android 37 beta8 w/ pdf.js extension installed. No alert appears.

Comment 37

[Bogdan Maris, Desktop QA]

Verified on Firefox 37.0 RC build 2, latest Aurora and latest Nightly across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 64-bit) that the alert is no longer there plus the pdf is displayed inframe.

Comment 38

[Bogdan Maris, Desktop QA]

Verified on Firefox 31.6.0 ESR as well across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04-64bit).