290162 - crash in InstallTrigger.install.call

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050408 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050408 Firefox/1.0.3

InstallTrigger.install.call(document,"a","a");
crashes in a strange way, jumping to strange places.
crashes at different $eip which shows it may be exploitable.

last crash:

0x08b9e161 in ?? ()
(gdb) x/i $eip
0x8b9e161:      in     $0xaf,%eax
(gdb) 


Reproducible: Always

Steps to Reproduce:
will attach a testacase

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: testcase

Comment 2

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050413
Firefox/1.0+

I also crash out with the testcase. TBID TB5058239M

Comment 3

[Matthias Versen [:Matti]]

Attachment: win2k3 stack trace from 20050404 (optimized with symbols)

Comment 4

[Matthias Versen [:Matti]]

This is core (trying JS Engine)

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: winxpsp2 stack from debug seamonkey 20050412

completely different stack

Comment 6

[georgi - hopefully not receiving bugspam]

bugzilla overwrote some entries on reload, feel free to correct.

Comment 7

[Brendan Eich [:brendan]]

http://lxr.mozilla.org/mozilla/source/xpinstall/src/nsJSInstallTriggerGlobal.cpp#158

nsJSInstallTriggerGlobal.cpp needs to be scoured of all JS_GetPrivate calls in
methods that can be invoked on unrelated |this| object types.  The right API has
always been JS_GetInstancePrivate.  Not sure how exploitable this is, but it's
an easy bug to fix.

/be

Comment 8

[Brendan Eich [:brendan]]

Seven JS_GetPrivate calls in nsJSInstallTriggerGlobal.cpp.  Six of them need to
be changed to JS_GetInstancePrivate.  The call from the finalizer is fine, it
should not be changed.

Any other ancient 4.x-era hand-rolled JS API code lying around?

/be

Comment 9

[georgi - hopefully not receiving bugspam]

	alert(InstallVersion.toString());
	alert(InstallVersion.toString.call(document,"a","a"));
	

Comment 10

[georgi - hopefully not receiving bugspam]

	alert(InstallVersion.toString.call(0xbabe,"a","a"));

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1086057216 (LWP 14155)]
0x4010fbe1 in nsSubstring::Replace () from ./local/firefox103/libxpcom.so

(gdb) x/i $eip
0x4010fbe1 <_ZN11nsSubstring7ReplaceEjjPKtj+281>:       cmpw   $0x0,(%eax)
(gdb) p $eax
$4 = 95612

Comment 11

[Brendan Eich [:brendan]]

http://lxr.mozilla.org/mozilla/search?string=JS_GetPrivate%28

Look at all xpinstall uses.  Any from a JS native function (a function with the
JSNative signature:

JSBool
(*JSNative)(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)

should be changed to call JS_GetInstancePrivate, passing argv as the final param.

/be

Comment 12

[Brendan Eich [:brendan]]

> should be changed to call JS_GetInstancePrivate, passing argv as the final
> param.

And null-test that API's return value, and return JS_FALSE if null.

/be

Comment 13

[Johnny Stenback (:jst)]

Attachment: WIP that does what brendan talked about

This should take care of the JS_GetPrivate()'s mentioned above, but it doesn't
fix the crash, at least not completely. With this patch, I now crash at:

#0  0x00002aaaadd878fa in XPCWrappedNativeScope::GetGlobalJSObject (
    this=0x100000009) at xpcprivate.h:1000
#1  0x00002aaaaddc972b in IsSystemCallingContent (cx=0x782be0,
    wrapper_or_proto={{mMaybeWrapper = 0xe59c08, mMaybeProto = 0xe59c08}})
    at
../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:947
#2  0x00002aaaaddca4e1 in XPC_WN_JSOp_Safe_GetProperty (cx=0x782be0,
    obj=0x897ab0, id=8632352, vp=0x7fffffffe238)
    at
../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1282
#3  0x00002aaaaab144d0 in js_Interpret (cx=0x782be0, result=0x7fffffffe488)
    at ../../../mozilla/js/src/jsinterp.c:2823
#4  0x00002aaaaab0653d in js_Invoke (cx=0x782be0, argc=2, flags=2)
    at ../../../mozilla/js/src/jsinterp.c:966
#5  0x00002aaaaab06980 in js_InternalInvoke (cx=0x782be0, obj=0x880170,
    fval=11542224, flags=0, argc=2, argv=0xdc0a40, rval=0x7fffffffe7c8)
    at ../../../mozilla/js/src/jsinterp.c:1043

where it looks like we've got a bad wrapper in wrapper_or_proto. I don't have
tiem to continue investigating this right now, so someone else will need to
continue from here.

Comment 14

[Brendan Eich [:brendan]]

Attachment: fix

Need to test returns from JS_GetInstancePrivate and fail if null.

/be

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 180623 [details] [diff] [review]
fix

sr=shaver

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 180623 [details] [diff] [review]
fix

I was building on Linux, didn't catch the winreg copy/paste bug.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: fix, v2

This is better.  Thanks to dveditz for pointing out an existing, now-wrong and
redundant, null test everywhere; he also pointed out a Windows-only botch or
two.

/be

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 180631 [details] [diff] [review]
fix, v2

r=dveditz

Comment 19

[chris hofmann]

Comment on attachment 180631 [details] [diff] [review]
fix, v2

a=chofmann for the branches -- assuming shaver's sr carries over

Comment 20

[Brendan Eich [:brendan]]

Fixed on both branches and trunk.

/be

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 180631 [details] [diff] [review]
fix, v2

sr=shaver, encore.

Comment 22

[sairuh (rarely reading bugmail)]

vrfy'd fixed on the branch: testcase no longer crashes --tested with
2005041320-1.0.3 ffox build on mac os x 10.3.8.

Comment 23

[georgi - hopefully not receiving bugspam]

preliminary blackbox approach shows js cares (at least before brendan's patch)
about raw C pointers, which is a sign of exploitability.

the following reads memory:

	alert(InstallVersion.toString.call(0x20087df0,"a","a").toSource());
	// reads memory around 2*0xNUMBER (below)	

Comment 24

[georgi - hopefully not receiving bugspam]

almost sure this is reliably exploitable.

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, that's consistent with the misuse of JS_GetPrivate that brendan patched:
the integer is tagged as a jsval (x << 1 + 1) and then stored in the private
slot of the Number object created to serve as the |this| for the invocation of
InstallTrigger.install.  The pre-patch code will use that jsval as an nsInstall
*, and then manipulate it in whatever interesting ways.  We're now safe from
that, as the Number class will not pass the JS_GetInstancePrivate class-pointer
test and will simply return NULL.

Should probably audit the rest of the tree for such bold uses of JS_GetPrivate
elsewhere, too; lxr reports some things that I would look more closely at if it
weren't 4:30AM.

Comment 26

[georgi - hopefully not receiving bugspam]

i believe grep -rniI STRING * in the tree is better than lxr - lxr may have
missed something because of a bug.

or grep with other options.

Comment 27

[Chase Phillips]

Looks like a fix or related fix for this bug caused bug 290312 on the aviary
branch.  See attachment 180703 [details].

Comment 28

[Brendan Eich [:brendan]]

Shaver's kindly patching the regression.

/be

Comment 29

[sairuh (rarely reading bugmail)]

oops, didn't see this since the mac doesn't have an installer. I can reopen if
needed.

Comment 30

[sairuh (rarely reading bugmail)]

the testcase in attachment 180577 [details] no longer causes a crash: tested with
2005041412-1.0.3 (linux) and 2005041413-1.0.3 (windowsXP) ffox builds.

Comment 31

[georgi - hopefully not receiving bugspam]

Attachment: please respect comments in the testcase

please respect comments in the testcase

Comment 32

[Daniel Veditz [:dveditz]]

Fix released

Comment 33

[Juha-Matti Laurio]

This is Secunia's SA14938's vulnerability #7;
http://secunia.com/advisories/14938/  and
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-1159 (CAN-2005-1159).

Comment 34

[Pete Collins (MDG)]

Doug, I presume it was this patch that caused the regression ...

https://bugzilla.mozilla.org/attachment.cgi?id=180631

Looks like a lot of added checks for js contexts in there. I will have a look
tomorrow.

Comment 35

[Ginn Chen]

Firefox 1.0.7 crashed when open the last testcase (https://bugzilla.mozilla.org/attachment.cgi?id=180797)
Firefox 1.0.6 won't crash. (Tested on Linux and Solaris)
Mozilla 1.7.12 also crashed.

Firefox 1.5 on Linux doesn't crash, but Firefox 1.5 on Windows crashed.

Reopen this bug?

Comment 36

[Ginn Chen]

> Firefox 1.5 on Linux doesn't crash, but Firefox 1.5 on Windows crashed.

Sorry, I was using Firefox 1.0.7 on Windows.
Firefox 1.5 doesn't crash on either Linux or Windows.
Firefox 1.0.7 crashed on every platform.
See TB13556108H
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=13556108

Comment 37

[Ginn Chen]

I think I got the bug number which fixed the regression in 1.8 branch and trunk.
That bug is still security-sensitive.
I'm not authorized to access.

Comment 38

[Bob Clary [:bc] (inactive)]

js/src/xpconnect/crashtests/290162-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3