Last Comment Bug 871368 - (CVE-2013-1710) Arbitrary code execution using crypto.generateCRMFRequest
(CVE-2013-1710)
: Arbitrary code execution using crypto.generateCRMFRequest
Status: RESOLVED FIXED
[adv-main23+][adv-esr1708+][fixed by ...
: csectype-priv-escalation, sec-critical
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla24
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks: 1055189
  Show dependency treegraph
 
Reported: 2013-05-12 19:52 PDT by moz_bug_r_a4
Modified: 2014-11-19 20:03 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
verified
+
verified
23+
verified
fixed
wontfix
wontfix
fixed


Attachments
Placeholder for bug 871301 patch (34 bytes, text/plain)
2013-06-11 11:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
lukasblakk+bugs: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details

Description moz_bug_r_a4 2013-05-12 19:52:44 PDT
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 2013-05-12 19:54:27 PDT
Created attachment 748621 [details]
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 2013-05-12 19:56:06 PDT
Created attachment 748622 [details]
testcase 2 - Arbitrary code execution

This triggers an assertion failure in debug builds.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-05-13 10:07:56 PDT
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) (busy with Stylo) 2013-05-13 11:13:52 PDT
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) (busy with Stylo) 2013-05-13 11:19:14 PDT
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) (busy with Stylo) 2013-05-13 11:19:57 PDT
(also note that we appear to have zero test coverage of this function apart from one crashtest :-( )
Comment 7 Jesse Ruderman 2013-05-15 07:47:39 PDT
Assertion failure: cx->compartment->principals == options.principals, at ../../../js/src/jsapi.cpp:5641
Comment 8 Jesse Ruderman 2013-05-15 07:47:50 PDT
cf bug 327126
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2013-05-15 08:03:56 PDT
(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 2013-05-15 08:15:22 PDT
That's on mozilla-central, which your fix hasn't hit yet.
Comment 11 Jesse Ruderman 2013-05-15 08:36:54 PDT
I made some additions to my DOM fuzzer to help find bugs like this (a6e1e4bf9951, d2a2418aeaac).
Comment 12 Daniel Veditz [:dveditz] 2013-05-16 13:33:53 PDT
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) (busy with Stylo) 2013-05-20 13:16:51 PDT
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:] 2013-05-20 16:55:40 PDT
Crashes FF22, but does not seem to crash FF17.0.6esr.
Comment 15 Al Billings [:abillings] 2013-05-20 16:55:58 PDT
?

How do we upload a bug with no patch? Do you mean uplift bug 871301?
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2013-05-20 20:37:21 PDT
(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 2013-05-20 22:04:13 PDT
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 2013-05-20 22:05:54 PDT
Created attachment 752019 [details]
testcase 3 - XSS

This tries to get cookies for www.mozilla.org.
Comment 19 moz_bug_r_a4 2013-05-20 22:11:50 PDT
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] 2013-05-21 11:02:31 PDT
I want to loop in Release Management and see when they want to take this.
Comment 21 Al Billings [:abillings] 2013-05-21 11:03:19 PDT
Marking B2G as affected unless someone wants to correct me.
Comment 22 Alex Keybl [:akeybl] 2013-06-11 11:15:30 PDT
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) (busy with Stylo) 2013-06-11 11:24:47 PDT
Created attachment 761051 [details]
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) (busy with Stylo) 2013-06-11 11:26:30 PDT
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] 2013-06-11 11:43:54 PDT
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) (busy with Stylo) 2013-06-11 11:51:03 PDT
(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] 2013-06-11 17:49:28 PDT
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: gavin@gavinsharp.com] 2013-06-11 19:21:21 PDT
(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) (busy with Stylo) 2013-06-11 20:55:07 PDT
> (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) (busy with Stylo) 2013-06-11 20:59:04 PDT
(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) (busy with Stylo) 2013-06-26 09:26:52 PDT
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) (busy with Stylo) 2013-06-26 12:54:26 PDT
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] 2013-06-26 17:22:41 PDT
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 2013-07-03 11:33:21 PDT
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) (busy with Stylo) 2013-07-16 20:54:46 PDT
I've written up a bogus approval request in bug 871301.
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-07-22 14:31:43 PDT
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 38 Bobby Holley (:bholley) (busy with Stylo) 2013-08-02 10:43:17 PDT
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, QA [:bogdan_maris] 2013-08-05 07:43:48 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.