Bug 1146339 (CVE-2015-0801)

A variant of Bug 1144988 lets one bypass same-origin policy

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
4 years ago
2 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({sec-high})

36 Branch
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox36+ wontfix, firefox37+ verified, firefox38+ verified, firefox39+ verified, firefox-esr3137+ 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)

Details

(Whiteboard: [adv-main37+][adv-esr31.6+][bg2-adv-main2.2+])

Attachments

(3 attachments, 1 obsolete attachment)

Posted file 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.
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.
[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...
Flags: needinfo?(lmandel)
(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.
> This makes us open pdf in a new tab which is rather odd.

Really?  Even if you cancel the channel?
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?
(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.
(In reply to Not doing reviews right now from comment #5) 
> Really?  Even if you cancel the channel?
And which channel actually?
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
Flags: needinfo?(lmandel) → needinfo?(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.
> 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.
Flags: needinfo?(bzbarsky)
(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.
> Sorry, I meant "outside of this bug" :)

Ah.  In that case, not that we know of.
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.
Keywords: sec-high
Posted patch approach 1Splinter Review
Attachment #8581618 - Attachment is obsolete: true
Looks like Olli is taking this one.
Assignee: nobody → bugs
Attachment #8581768 - Flags: review?(bzbarsky)
Attachment #8581853 - Flags: review?(bzbarsky)
Comment on attachment 8581768 [details] [diff] [review]
approach 1

r=me
Attachment #8581768 - Flags: review?(bzbarsky) → review+
Comment on attachment 8581853 [details] [diff] [review]
for esr31

r=me
Attachment #8581853 - Flags: review?(bzbarsky) → review+
Do you want to nominate this for uplift to Aurora/Beta/ESR?
Flags: needinfo?(bugs)
Hmm, I need still beta patch. I think we want this there.
(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.)
The patch is ready, just compiling and testing.
Posted patch for betaSplinter Review
Flags: needinfo?(bugs)
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
Attachment #8581768 - Flags: sec-approval?
Attachment #8581768 - Flags: approval-mozilla-aurora?
Comment on attachment 8581853 [details] [diff] [review]
for esr31

[Approval Request Comment]
See comment 26.
Attachment #8581853 - Flags: approval-mozilla-esr31?
Comment on attachment 8582069 [details] [diff] [review]
for beta

Approval Request Comment
See comment 26.
Attachment #8582069 - Flags: approval-mozilla-beta?
Comment on attachment 8581768 [details] [diff] [review]
approach 1

sec-approval+ for trunk.
Attachment #8581768 - Flags: sec-approval? → sec-approval+
Comment on attachment 8582069 [details] [diff] [review]
for beta

We want this fix on all branches.
Attachment #8582069 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8581768 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8581853 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
I really can't deal with landing tonight. 3am+ here.
https://hg.mozilla.org/mozilla-central/rev/7ee9c3fce092
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Whiteboard: [adv-main37+][adv-esr31.6+]
Alias: CVE-2015-0801
verified - Firefox for Android 37 beta8 w/ pdf.js extension installed. No alert appears.
Summary: A variant of Bug 1144988 lets one to bypass same-origin policy → A variant of Bug 1144988 lets one bypass same-origin policy
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.
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).
Whiteboard: [adv-main37+][adv-esr31.6+] → [adv-main37+][adv-esr31.6+][bg2-adv-main2.2+]
Depends on: 1180382

Updated

4 years ago
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.