Closed Bug 290162 Opened 15 years ago Closed 15 years ago

crash in InstallTrigger.install.call

Categories

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

x86
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: guninski, Assigned: dveditz)

References

Details

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

Attachments

(4 files, 3 obsolete files)

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
Attached file testcase (obsolete) —
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Keywords: testcase
This is core (trying JS Engine)
Assignee: firefox → general
Severity: normal → critical
Component: General → JavaScript Engine
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
completely different stack
Group: security
Severity: critical → normal
Component: JavaScript Engine → Build Config
OS: All → Linux
Product: Core → Firefox
Version: Trunk → 1.0 Branch
bugzilla overwrote some entries on reload, feel free to correct.
OS: Linux → All
Product: Firefox → Core
Version: 1.0 Branch → Trunk
Component: Build Config → Installer: XPInstall Engine
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
Assignee: general → dveditz
Severity: normal → critical
Flags: blocking1.8b2+
Flags: blocking1.7.7?
Flags: blocking-aviary1.0.3?
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
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
	alert(InstallVersion.toString());
	alert(InstallVersion.toString.call(document,"a","a"));
	
	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
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
> 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

Status: NEW → ASSIGNED
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.
Attached patch fix (obsolete) — Splinter Review
Need to test returns from JS_GetInstancePrivate and fail if null.

/be
Attachment #180609 - Attachment is obsolete: true
Attachment #180623 - Flags: superreview?(shaver)
Attachment #180623 - Flags: review?(dveditz)
Attachment #180623 - Flags: approval1.8b2+
Attachment #180623 - Flags: approval1.7.7?
Attachment #180623 - Flags: approval-aviary1.0.3?
Comment on attachment 180623 [details] [diff] [review]
fix

sr=shaver
Attachment #180623 - Flags: superreview?(shaver) → superreview+
Comment on attachment 180623 [details] [diff] [review]
fix

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

/be
Attachment #180623 - Attachment is obsolete: true
Attachment #180623 - Flags: review?(dveditz)
Attachment #180623 - Flags: approval1.8b2+
Attachment #180623 - Flags: approval1.7.7?
Attachment #180623 - Flags: approval-aviary1.0.3?
Attached patch fix, v2Splinter Review
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
Attachment #180631 - Flags: superreview?(shaver)
Attachment #180631 - Flags: review?(dveditz)
Attachment #180631 - Flags: approval1.8b2+
Attachment #180631 - Flags: approval1.7.7?
Attachment #180631 - Flags: approval-aviary1.0.3?
Comment on attachment 180631 [details] [diff] [review]
fix, v2

r=dveditz
Attachment #180631 - Flags: review?(dveditz) → review+
Comment on attachment 180631 [details] [diff] [review]
fix, v2

a=chofmann for the branches -- assuming shaver's sr carries over
Attachment #180631 - Flags: approval1.7.7?
Attachment #180631 - Flags: approval1.7.7+
Attachment #180631 - Flags: approval-aviary1.0.3?
Attachment #180631 - Flags: approval-aviary1.0.3+
Fixed on both branches and trunk.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 180631 [details] [diff] [review]
fix, v2

sr=shaver, encore.
Attachment #180631 - Flags: superreview?(shaver) → superreview+
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.
Status: RESOLVED → VERIFIED
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)	
almost sure this is reliably exploitable.
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.
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.
Looks like a fix or related fix for this bug caused bug 290312 on the aviary
branch.  See attachment 180703 [details].
Shaver's kindly patching the regression.

/be
oops, didn't see this since the mac doesn't have an installer. I can reopen if
needed.
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.
Flags: blocking1.7.7? → blocking1.7.7+
Blocks: sbb?
Whiteboard: [sg:fix]
please respect comments in the testcase
Attachment #180577 - Attachment is obsolete: true
Fix released
Group: security
Flags: blocking-aviary1.0.3?
Keywords: fixed1.7.7
Blocks: sbb+
No longer blocks: sbb?
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.
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?
> 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
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.
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
js/src/xpconnect/crashtests/290162-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3
Flags: in-testsuite? → in-testsuite+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.