871368 - (CVE-2013-1710) Arbitrary code execution using crypto.generateCRMFRequest

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

nsCrypto::GenerateCRMFRequest uses JS_GetGlobalObject, so a callback script can be executed with a global object that is unrelated to the caller.  This allows for a XSS attack or arbitrary code execution.

Bug 871301 seems to fix this.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - XSS

This triggers an assertion failure in debug builds.

This tries to get cookies for blog.mozilla.org.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - Arbitrary code execution

This triggers an assertion failure in debug builds.

Comment 3

[Bobby Holley (:bholley)]

Sigh. That bug is thankfully pretty innocuous-looking, so let's not mark the dependency. Let's get that landed.

Comment 4

[Bobby Holley (:bholley)]

dchan, this API seems super old and not well-understood, but is nonetheless web exposed. Can you make sure we do some kind of cursory audit of this stuff sometime?

Comment 5

[Bobby Holley (:bholley)]

I realized that the cx pushing in that bug is also going to cause a compartment mismatch with the current patch, because it pushes _after_ entering a compartment. I fixed it up in a hopefully-nonchalant fashion.

Comment 6

[Bobby Holley (:bholley)]

(also note that we appear to have zero test coverage of this function apart from one crashtest :-( )

Comment 7

[Jesse Ruderman]

Assertion failure: cx->compartment->principals == options.principals, at ../../../js/src/jsapi.cpp:5641

Comment 8

[Jesse Ruderman]

cf bug 327126

Comment 9

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #7)
> Assertion failure: cx->compartment->principals == options.principals, at
> ../../../js/src/jsapi.cpp:5641

Jesse, is this the assertion that we hit before bug 871301 landed (which I would expect)? Or does it happen on trunk?

Comment 10

[Jesse Ruderman]

That's on mozilla-central, which your fix hasn't hit yet.

Comment 11

[Jesse Ruderman]

I made some additions to my DOM fuzzer to help find bugs like this (a6e1e4bf9951, d2a2418aeaac).

Comment 12

[Daniel Veditz [:dveditz]]

bug 871301 is now fixed on m-c.

Matt: another bug to try in Fx22 and ESR-17 to see if we need to back-port fixes.

Comment 13

[Bobby Holley (:bholley)]

We should uplift bug 871368 to all branches relatively close to the next branch. Al, when shall we do it?

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Crashes FF22, but does not seem to crash FF17.0.6esr.

Comment 15

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

?

How do we upload a bug with no patch? Do you mean uplift bug 871301?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #15)
> ?
> 
> How do we upload a bug with no patch? Do you mean uplift bug 871301?

Yes.

Comment 17

[moz_bug_r_a4]

testcase 1 no longer works because now blog.mozilla.org sends X-Frame-Options: SAMEORIGIN.  I'll attach a new testcase.

Comment 18

[moz_bug_r_a4]

Attachment: testcase 3 - XSS

This tries to get cookies for www.mozilla.org.

Comment 19

[moz_bug_r_a4]

I can reproduce testcase 2 (a Components.stack alert dialog appears) and testcase 3 (a XSS alert dialog appears) on fx23, 22, 21 and 17.  (I've tested only opt builds.)

Comment 20

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

I want to loop in Release Management and see when they want to take this.

Comment 21

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

Marking B2G as affected unless someone wants to correct me.

Comment 22

[Alex Keybl [:akeybl]]

Is bug 871301 something we'd still like to take into FF23? If so, please nominate for Aurora approval. It's too late in Beta to take a change of this nature.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Placeholder for bug 871301 patch

We probably need sec-approval.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Somewhat easily. If someone looks at the function we're patching, it's pretty obvious that the API evaluates arbitrary strings, which is a pretty strong hint at how to start exploiting it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

The last decade. :-(

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Don't have backports, but they should be relatively straightforward.

How likely is this patch to cause regressions; how much testing does it need?

We're changing behavior in certain cases, but this is a mozilla-proprietary API. I'm more concerned about exploits than I am about regressions.

Comment 24

[Bobby Holley (:bholley)]

Also, it really sucks that this fell through the cracks for this cycle. I think the ideal time to land it would have been a month ago. :-( Where did we slip up here?

Comment 25

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

Comment on attachment 761051 [details]
Placeholder for bug 871301 patch

Sec-approval+ for trunk.

As to how it was missed...well, it wasn't really. It wasn't approved for branches, which is the problem here, and nothing got a sec-approval request for trunk. We get a lot of sec issues that we have to ping people on directly to get traction on patches a lot of the time.

Comment 26

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #25)
> Comment on attachment 761051 [details]
> Sec-approval+ for trunk.

It's already on trunk. The whole question of this bug since comment 13 is when we check it in on branches, thereby revealing that it's a security issue.

> As to how it was missed...well, it wasn't really. It wasn't approved for
> branches, which is the problem here, and nothing got a sec-approval request
> for trunk. We get a lot of sec issues that we have to ping people on
> directly to get traction on patches a lot of the time.

It seems like comment 20 sought to answer the question of "when do we want to check this in on branches." I'm wondering what caused that to go unanswered for so many weeks, and if we can build a more robust mechanism for handling these things in the future.

Comment 27

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

I don't get to answer that question for branches. That requires release management input. From now on, I'm going to needinfo? them or just have people mark the patches ? for approval for beta and/or aurora. I gather that these show up in queries.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #26)
> It seems like comment 20 sought to answer the question of "when do we want
> to check this in on branches." I'm wondering what caused that to go
> unanswered for so many weeks, and if we can build a more robust mechanism
> for handling these things in the future.

You should generally get that question answered by just requesting approval for aurora/beta/esr, especially for bugs already tracked.

Comment 29

[Bobby Holley (:bholley)]

> (In reply to Bobby Holley (:bholley) from comment #26)
> You should generally get that question answered by just requesting approval
> for aurora/beta/esr, especially for bugs already tracked.

Ok.

(In reply to Al Billings [:abillings] from comment #27)
> I don't get to answer that question for branches. That requires release
> management input. From now on, I'm going to needinfo? them or just have
> people mark the patches ? for approval for beta and/or aurora. I gather that
> these show up in queries.

That sounds good to me.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #29)
> That sounds good to me.

Though I guess my concerns is that the release drivers are considering things from a regression risk perspective, but not necessarily from a zero-day perspective. I guess I can ask for that analysis to be done explicitly in my request.

I'll add a reminder for myself to nominate this for all branches on June 26th.

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 761051 [details]
Placeholder for bug 871301 patch

Now that the merge has happened, I'm flagging this for branch uplift. Please let me know when we want to land this timing-wise.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since forever.
User impact if declined: sec-critial
Testing completed (on m-c, etc.): Baked on m-c for many weeks.
Risk to taking this patch (and alternatives if risky): Low risk. No alternative.
String or IDL/UUID changes made by this patch: None.

Comment 32

[Bobby Holley (:bholley)]

Do we want to land this now? Or wait a bit into the cycle to decrease the attack window? There is currently no public-facing indication that the patch in bug 871301 fixes a security issue.

Comment 33

[David Chan [:dchan]]

Oh wow. Sorry Bobby. I think my bugzilla filters are incorrect and I didn't get the needinfo from this a month ago. I'll add the crypto stuff to my list and talk to Yvan about it.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Bholley - let's put this in, via an obfuscated comment in the week of 7/22 - I will put this in my reminder list to check back on.

Comment 35

[Bobby Holley (:bholley)]

I've written up a bogus approval request in bug 871301.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

b2g18 is closed at the moment, but it's in my queue to land there once it reopens.

https://hg.mozilla.org/releases/mozilla-beta/rev/59c478137f1e
https://hg.mozilla.org/releases/mozilla-esr17/rev/206dba739a7b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ea8e6808127f
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/ea8e6808127f

Comment 38

[Bobby Holley (:bholley)]

Hi Bogdan,

This is the real issue behind bug 871301. Please do any verification work here and keep the dependency secret. Do not comment further in bug 871301.

Comment 39

[Bogdan Maris, Desktop QA]

Thx Bobby for giving me access to this bug.

Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 5.1; rv:24.0) Gecko/20130805 Firefox/24.0
Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20100101 Firefox/17.0

Verified as fixed on Firefox 23 RC (buildID: 20130730113002), Firefox 17.0.8esr (buildID: 20130729214632) latest Aurora (buildID: 20130805004006) and latest Nightly (buildID: 20130804030207).