nsCryptoRunnable::Run() may operate on dead JSContext

RESOLVED FIXED in mozilla1.8.1beta1

Status

Core Graveyard
Security: UI
P1
critical
RESOLVED FIXED
11 years ago
9 months ago

People

(Reporter: shutdown, Assigned: mrbkap)

Tracking

({fixed1.8.1, verified1.8.0.5})

Trunk
mozilla1.8.1beta1
fixed1.8.1, verified1.8.0.5
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] [kerh-coa], URL)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 221604 [details]
testcase userscript

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

Comment 2

11 years ago
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.
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();
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.5+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:critical]

Updated

11 years ago
Priority: -- → P1
Whiteboard: [sg:critical] → [sg:critical] [kerh-coa]
Target Milestone: --- → mozilla1.8.1

Comment 4

11 years ago
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

11 years ago
Created attachment 225885 [details] [diff] [review]
Patch v1

Would this help?

Comment 6

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

11 years ago
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
Attachment #225885 - Attachment is obsolete: true

Comment 9

11 years ago
assigning to nobody, to make it clear that more help is needed
Assignee: kengert → nobody
Blake, any pointers you can give on where this patch is going wrong (see comment 8)

Comment 11

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



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

Comment 13

11 years ago
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
Attachment #226357 - Attachment is obsolete: true

Comment 14

11 years ago
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
(Assignee)

Comment 15

11 years ago
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.
Assignee: jst → mrbkap
Attachment #226719 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #226890 - Flags: superreview?(jst)
Attachment #226890 - Flags: review?(jst)
(Assignee)

Comment 16

11 years ago
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.
Attachment #226891 - Flags: superreview?(jst)
Attachment #226891 - Flags: review?(jst)
(Assignee)

Comment 17

11 years ago
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).
Attachment #226891 - Attachment is obsolete: true
Attachment #226896 - Flags: superreview?(jst)
Attachment #226896 - Flags: review?(jst)
Attachment #226891 - Flags: superreview?(jst)
Attachment #226891 - Flags: review?(jst)
(Assignee)

Comment 18

11 years ago
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.
Attachment #226896 - Attachment is obsolete: true
Attachment #226897 - Flags: superreview?(jst)
Attachment #226897 - Flags: review?(jst)
Attachment #226896 - Flags: superreview?(jst)
Attachment #226896 - Flags: review?(jst)
Comment on attachment 226890 [details] [diff] [review]
Untested trunk patch

r+sr=jst
Attachment #226890 - Flags: superreview?(jst)
Attachment #226890 - Flags: superreview+
Attachment #226890 - Flags: review?(jst)
Attachment #226890 - Flags: review+
(Assignee)

Comment 20

11 years ago
I checked the trunk patch into the proper place.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #226897 - Flags: approval1.8.1?
Attachment #226897 - Flags: approval1.8.0.5?
Comment on attachment 226897 [details] [diff] [review]
Better branch patch

r+sr=jst
Attachment #226897 - Flags: superreview?(jst)
Attachment #226897 - Flags: superreview+
Attachment #226897 - Flags: review?(jst)
Attachment #226897 - Flags: review+

Comment 22

11 years ago
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 on attachment 226897 [details] [diff] [review]
Better branch patch

approved for 1.8.0 branch, a=dveditz
Attachment #226897 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 24

11 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.5
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.
Attachment #226897 - Flags: approval1.8.1? → approval1.8.1+

Updated

11 years ago
Attachment #226978 - Attachment is obsolete: true

Comment 26

11 years ago
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

11 years ago
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).
Keywords: fixed1.8.0.5 → verified1.8.0.5

Updated

11 years ago
Whiteboard: [sg:critical] [kerh-coa] → [sg:critical] [kerh-coa] [checkin needed]
Is this part of the JS 1.7 uplift? If not, can we get it landed on trunk ASAP?
Target Milestone: mozilla1.8.1 → mozilla1.8.1beta1
(Assignee)

Comment 29

11 years ago
(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.
(Assignee)

Comment 30

11 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1

Updated

11 years ago
Whiteboard: [sg:critical] [kerh-coa] [checkin needed] → [sg:critical] [kerh-coa]
Group: security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.