Last Comment Bug 337462 - nsCryptoRunnable::Run() may operate on dead JSContext
: nsCryptoRunnable::Run() may operate on dead JSContext
Status: RESOLVED FIXED
[sg:critical] [kerh-coa]
: fixed1.8.1, verified1.8.0.5
Product: Core Graveyard
Classification: Graveyard
Component: Security: UI (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8.1beta1
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-10 10:43 PDT by shutdown
Modified: 2016-09-27 13:03 PDT (History)
11 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.5+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase userscript (409 bytes, text/javascript)
2006-05-10 10:48 PDT, shutdown
no flags Details
Patch v1 (2.33 KB, patch)
2006-06-16 10:26 PDT, Kai Engert (:kaie) (on vacation)
no flags Details | Diff | Splinter Review
Patch v2 (3.86 KB, patch)
2006-06-20 11:27 PDT, Kai Engert (:kaie) (on vacation)
no flags Details | Diff | Splinter Review
Patch v3 (3.08 KB, patch)
2006-06-22 15:44 PDT, Kai Engert (:kaie) (on vacation)
no flags Details | Diff | Splinter Review
Untested trunk patch (6.55 KB, patch)
2006-06-23 19:09 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Tested branch patch (7.68 KB, patch)
2006-06-23 19:11 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
New branch patch (8.30 KB, patch)
2006-06-23 20:51 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Better branch patch (8.42 KB, patch)
2006-06-23 21:24 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
dveditz: approval1.8.0.5+
dbaron: approval1.8.1+
Details | Diff | Splinter Review
first attempt to backport blakes patch to 1.0.x branch. (4.49 KB, patch)
2006-06-25 06:20 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
better patch for aviary branch (1.97 KB, patch)
2006-06-26 15:40 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description shutdown 2006-05-10 10:43:21 PDT
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 2006-05-10 10:48:39 PDT
Created attachment 221604 [details]
testcase userscript

steps:
1. install GreaseMonkey.
2. install attached userscript.
3. visit http://www.mozilla.org/hackme
Comment 2 shutdown 2006-05-10 12:50:57 PDT
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] 2006-05-10 17:55:37 PDT
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) (on vacation) 2006-06-14 10:48:27 PDT
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) (on vacation) 2006-06-16 10:26:16 PDT
Created attachment 225885 [details] [diff] [review]
Patch v1

Would this help?
Comment 6 Kai Engert (:kaie) (on vacation) 2006-06-16 10:55:21 PDT
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 2006-06-19 16:47:58 PDT
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) (on vacation) 2006-06-20 11:27:17 PDT
Created attachment 226357 [details] [diff] [review]
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) (on vacation) 2006-06-21 10:10:08 PDT
assigning to nobody, to make it clear that more help is needed
Comment 10 Daniel Veditz [:dveditz] 2006-06-21 15:35:21 PDT
Blake, any pointers you can give on where this patch is going wrong (see comment 8)
Comment 11 Alexander Sack 2006-06-21 20:34:40 PDT
> 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] 2006-06-22 14:50:00 PDT
jst: is this something you could help with (see comment 7)
Comment 13 Kai Engert (:kaie) (on vacation) 2006-06-22 15:44:04 PDT
Created attachment 226719 [details] [diff] [review]
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) (on vacation) 2006-06-22 16:19:47 PDT
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) 2006-06-23 19:09:55 PDT
Created attachment 226890 [details] [diff] [review]
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) 2006-06-23 19:11:43 PDT
Created attachment 226891 [details] [diff] [review]
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) 2006-06-23 20:51:49 PDT
Created attachment 226896 [details] [diff] [review]
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) 2006-06-23 21:24:13 PDT
Created attachment 226897 [details] [diff] [review]
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, jst@mozilla.com) 2006-06-23 21:29:08 PDT
Comment on attachment 226890 [details] [diff] [review]
Untested trunk patch

r+sr=jst
Comment 20 Blake Kaplan (:mrbkap) 2006-06-23 21:35:28 PDT
I checked the trunk patch into the proper place.
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2006-06-24 11:20:20 PDT
Comment on attachment 226897 [details] [diff] [review]
Better branch patch

r+sr=jst
Comment 22 Alexander Sack 2006-06-25 06:20:09 PDT
Created attachment 226978 [details] [diff] [review]
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] 2006-06-26 01:15:59 PDT
Comment on attachment 226897 [details] [diff] [review]
Better branch patch

approved for 1.8.0 branch, a=dveditz
Comment 24 Blake Kaplan (:mrbkap) 2006-06-26 01:19:15 PDT
Fix checked into the 1.8.0 branch.
Comment 25 David Baron :dbaron: ⌚️UTC-8 2006-06-26 10:20:04 PDT
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 2006-06-26 15:40:31 PDT
Created attachment 227156 [details] [diff] [review]
better patch for aviary branch

With some hints from Blake, I crafted this more minimal patch for aviary branch.
Comment 27 Jay Patel [:jay] 2006-06-26 16:46:13 PDT
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] 2006-07-06 11:19:19 PDT
Is this part of the JS 1.7 uplift? If not, can we get it landed on trunk ASAP?
Comment 29 Blake Kaplan (:mrbkap) 2006-07-06 11:41:35 PDT
(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) 2006-07-06 12:37:25 PDT
Fix checked into the 1.8 branch.

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