337462 - nsCryptoRunnable::Run() may operate on dead JSContext

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[shutdown]

1768 nsCryptoRunnable::Run()
1772   JSContext *cx = m_args->m_cx;
1785   if (JS_EvaluateScriptForPrincipals(cx, m_args->m_scope, principals,

The JSContext referenced here may already have been destroyed,
because m_args->m_cx is just a weak reference and
nsCryptoRunnable::Run() is called asynchronously.

Comment 1

[shutdown]

Attachment: testcase userscript

steps:
1. install GreaseMonkey.
2. install attached userscript.
3. visit http://www.mozilla.org/hackme

Comment 2

[shutdown]

TB18535506H
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2) Gecko/20060510 BonEcho/2.0a2

FIREFOX caused an invalid page fault in
module JS3250.DLL at 017f:600ecebc.
Registers:
EAX=02c427c0 CS=017f EIP=600ecebc EFLGS=00010212
EBX=50505050 SS=0187 ESP=00d5f7bc EBP=00d5f7d4
ECX=00000000 DS=0187 ESI=000000b0 FS=3a57
EDX=00000000 ES=0187 EDI=000000ae GS=0000
Bytes at CS:EIP:
ff b3 08 24 00 00 ff 15 e4 60 12 60 80 bb 85 01
Stack dump:
000000ae 000000b0 02c427d0 00000000 00d5fb10 7800db11 00d5f7ec 60118016
02c427c0 00000003 00000008 00000002 00d5f828 60114911 02c427c0 02c427d0

EBX=50505050 shows that JSContext has been overwritten
by the script-supplied data. possibly exploitable.

Comment 3

[Daniel Veditz [:dveditz]]

I couldn't get ff1.5.0.3 to crash, but in a debug build it crashed nicely trying to reference the deleted (0xdddddddd) context. No doubt a slightly different testcase could be devised to exploit 1.5.0.3 as well.

Greasemonkey does not appear to be required, it's just the mechanism used here to get a deleted javascript context. It works just as well (in a debug build) to do:

url="data:text/html,<script%20src='https://bugzilla.mozilla.org/attachment.cgi?id=221604'></script>";
x=open(url);
x.close();

Comment 4

[Kai Engert (:KaiE:)]

sorry, I don't crashwith 1.8.0 branch on Linux, not with greasemonkey, nor with Dan's code in a debug build. I propose I do a patch without being able to reproduce.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Would this help?

Comment 6

[Kai Engert (:KaiE:)]

hmm, I get a console output that this object does not support weak refereneces?

I'd appreciate help, as I don't have much insight in JS engine code.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You need to add support to the XPCContext for weak references, which should be fairly easy.

Jst: would that be ok to do?

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch v2

I implemented the proposal, see this new patch, but when playing with my testcase attempt at http://kuix.de/misc/bug337462-bla-6543-hidden/ I crash with this message:

JS API usage error: the address passed to JS_AddNamedRoot currently holds an
invalid jsval.  This is usually caused by a missing call to JS_RemoveRoot.
The root's name is "nsCryptoRunnable::mScope".
Assertion failure: root_points_to_gcArenaPool, at /home/kaie/moz/18/mozilla/js/src/jsgc.c:1494

Comment 9

[Kai Engert (:KaiE:)]

assigning to nobody, to make it clear that more help is needed

Comment 10

[Daniel Veditz [:dveditz]]

Blake, any pointers you can give on where this patch is going wrong (see comment 8)

Comment 11

[Alexander Sack]

> JS API usage error: the address passed to JS_AddNamedRoot currently holds an
> invalid jsval.  This is usually caused by a missing call to JS_RemoveRoot.
> The root's name is "nsCryptoRunnable::mScope".
> Assertion failure: root_points_to_gcArenaPool, at
> /home/kaie/moz/18/mozilla/js/src/jsgc.c:1494
> 


(In reply to comment #10)
> Blake, any pointers you can give on where this patch is going wrong (see
> comment 8)

AFAICT, with patch v2 we don't call RemoveRoot at all if JSContext is destroyed in the time between constructor and destructor. In consequence, the root is not freed and calling JS_AddNamedRoot a second time will yield the error above.

Comment 12

[Daniel Veditz [:dveditz]]

jst: is this something you could help with (see comment 7)

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v3

the previous comments inspired me to hack this additional patch attempt, maybe we need to keep references to both XPCOM objects around, but I haven't tried it yet

Comment 14

[Kai Engert (:KaiE:)]

no, this patch crashes

###!!! ASSERTION: Someone is holding a bad reference to a XPCCallContext: 'mRefCnt == 0', file /home/kaie/moz/18/mozilla/js/src/xpconnect/src/xpccallcontext.cpp, line 294
Break: at file /home/kaie/moz/18/mozilla/js/src/xpconnect/src/xpccallcontext.cpp, line 294

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Untested trunk patch

I couldn't get GreaseMonkey to work at all in any of my trunk builds, so here's the untested version of a patch that I tested on the branch (albeit with a few changes). The idea is to make the security code hold a strong reference to an object that keeps the context that it's holding alive. In shutdown's case, this is an evalInSandbox sandbox. The normal case (where this code is called from a browser window) will get the nsIScriptContext implementation from the context, and that holds onto its context.

Note that the scripts in evalInSandbox *can* (as shutdown's script does) hang the browser. It may be worth opening a new bug on that.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Tested branch patch

I've tested to make sure that this version of the patch really does work. The main changes are the changes to make sure that exceptions are propagated.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: New branch patch

jst requested on IRC that I not remove the SandboxErrorReporter (to minimize the patch), so here's the patch that doesn't do that. Note that once the evalInSandbox finishes, we don't care about reporting exceptions that occur on the sandcx back to the original cx (since it has presumably moved on as well).

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better branch patch

I realized *just* after attaching the last patch that I needed to clear the script error reporter in DidEval to avoid crashes if the CRMF request generated an error.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 226890 [details] [diff] [review]
Untested trunk patch

r+sr=jst

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

I checked the trunk patch into the proper place.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 226897 [details] [diff] [review]
Better branch patch

r+sr=jst

Comment 22

[Alexander Sack]

Attachment: first attempt to backport blakes patch to 1.0.x branch.

I noticed, that on branch the context is not pushed to thread data in evalInSandbox. Is it safe to keep it that way?

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 226897 [details] [diff] [review]
Better branch patch

approved for 1.8.0 branch, a=dveditz

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 25

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

Comment on attachment 226897 [details] [diff] [review]
Better branch patch

a=dbaron on behalf of drivers.  Please check in to the MOZILLA_1_8_BRANCH and mark the bug fixed1.8.1 once you have.

Comment 26

[Alexander Sack]

Attachment: better patch for aviary branch

With some hints from Blake, I crafted this more minimal patch for aviary branch.

Comment 27

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crash with testcases (kai's test url or dveditz's js).

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this part of the JS 1.7 uplift? If not, can we get it landed on trunk ASAP?

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #28)
> Is this part of the JS 1.7 uplift? If not, can we get it landed on trunk ASAP?

Did you mean 1.8 branch there? I meant to check it in earlier, but forgot. I'll do that now.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.