CVE-2006-0293 GC hazard during function allocation.

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.1, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.8.0.1, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 -
blocking-aviary1.0.8 -
blocking1.8.1 +
blocking1.8.0.1 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: checked into trunk Jan 2 -- baking)

Attachments

(2 attachments)

(Assignee)

Description

12 years ago
js_NewFunction from jsfun.c contains the following code:

    /* Allocate a function struct. */
     fun = (JSFunction *) js_NewGCThing(cx, GCX_PRIVATE,
sizeof(JSFunction));
     if (!fun)
         return NULL;

     /* If funobj is null, allocate an object for it. */
     if (funobj) {
         OBJ_SET_PARENT(cx, funobj, parent);
     } else {
         funobj = js_NewObject(cx, &js_FunctionClass, NULL, parent);

Since js_NewObject also allocates slots using GCX_PRIVATE, it will remove
"fun" from "cx->newborn[GCX_PRIVATE]". If JS embedding installs JSRuntime.objectHook which triggers GC during "funobj" allocation, "fun" would be GC-ed when js_NewObject returns to js_NewFunction.

This is one possible reason for bug 317815.
(Assignee)

Comment 1

12 years ago
Created attachment 207296 [details] [diff] [review]
Fix via code rearangment
Attachment #207296 - Flags: review?(brendan)
(Assignee)

Comment 2

12 years ago
Lets see if the fix helps to address bug 317815 or that was caused by something else.
Blocks: 317815
Marking up bug to get on release tracks, hope you don't mind.

/be
Blocks: 309169
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 207296 [details] [diff] [review]
Fix via code rearangment

>+    /*
>+     * Allocate a function struct after allocating funobj so slot allocation
>+     * in js_NewObject would not wipe out fun from cx->newborn[GCX_PRIVATE].
>+     */
>+

Nit: no need for blank line after major comment.

Great, simple fix for likely bug.  Nominating for branches.  Thanks,

/be
Attachment #207296 - Flags: review?(brendan)
Attachment #207296 - Flags: review+
Attachment #207296 - Flags: approval1.8.1?
Attachment #207296 - Flags: approval1.8.0.1?

Comment 5

12 years ago
with the patch from bug 322001 and this patch I no longer crash using a dbg trunk build on winxp without venkman with the test URL in bug 317815.

I don't know if this is related to this patch, but installing venkman and shutting down causes the following 

###!!! ASSERTION: CheckForDeactivation called from wrong thread!: 'Error', file c:/work/mozilla/builds/ff/trunk-test/mozilla/xpcom/t
hreads/nsEventQueue.cpp, line 255

        /*
         * If this ASSERT fails, a bad pointer has been passed in. It may be
         * totally bogus, or it may have been allocated from another heap.
         * The pointer MUST come from the 'local' heap.
         */
        _ASSERTE(_CrtIsValidHeapPointer(pUserData));

_free_dbg_lk(void * 0x03651420, int 0x00000001) line 1044 + 48 bytes
_free_dbg(void * 0x03651420, int 0x00000001) line 1001 + 13 bytes
free(void * 0x03651420) line 956 + 11 bytes
js_FinalizeStringRT(JSRuntime * 0x00fb0068, JSString * 0x02d8c538) line 2715 + 13 bytes
js_atom_uninterner(JSHashEntry * 0x03651550, int 0x00000a72, void * 0x0012fb00) line 403 + 21 bytes
JS_HashTableEnumerateEntries(JSHashTable * 0x0104d538, int (JSHashEntry *, int, void *)* 0x00438c20 js_atom_uninterner(JSHashEntry *, int, void *), void * 0x0012fb00) line 362 + 15 bytes
js_FinishAtomState(JSAtomState * 0x00fb0204) line 418 + 21 bytes
JS_Finish(JSRuntime * 0x00fb0068) line 737 + 15 bytes
nsJSRuntimeServiceImpl::~nsJSRuntimeServiceImpl() line 93 + 13 bytes
nsJSRuntimeServiceImpl::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes
nsJSRuntimeServiceImpl::Release(nsJSRuntimeServiceImpl * const 0x01030000) line 103 + 141 bytes
nsJSRuntimeServiceImpl::FreeSingleton() line 126 + 26 bytes
xpcModuleDtor(nsIModule * 0x0102e430) line 135
nsGenericModule::Shutdown() line 339 + 10 bytes
nsGenericModule::~nsGenericModule() line 242
nsGenericModule::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes
nsGenericModule::Release(nsGenericModule * const 0x0102e430) line 244 + 139 bytes
nsCOMPtr<nsIModule>::assign_assuming_AddRef(nsIModule * 0x00000000) line 569
nsCOMPtr<nsIModule>::assign_with_AddRef(nsISupports * 0x00000000) line 1225
nsCOMPtr<nsIModule>::operator=(nsIModule * 0x00000000) line 714
nsNativeModuleLoader::ReleaserFunc(nsIHashable * 0x0102e17c, nsNativeModuleLoader::NativeLoadData & {...}, void * 0x00000000) line 211
nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable * 0x003fb720, PLDHashEntryHdr * 0x01d47ee8, unsigned int 0x00000015, void * 0x0012fc7c) line 346 + 28 bytes
PL_DHashTableEnumerate(PLDHashTable * 0x003fb720, int (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* 0x002e2310 nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *), void * 0x0012fc7c) line 621 + 34 bytes
nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::Enumerate(PLDHashOperator (nsIHashable *, nsNativeModuleLoader::NativeLoadData &, void *)* 0x002e1e50 nsNativeModuleLoader::ReleaserFunc(nsIHashable *, nsNativeModuleLoader::NativeLoadData &, void *), void * 0x00000000) line 221 + 18 bytes
nsNativeModuleLoader::UnloadLibraries() line 250
nsComponentManagerImpl::Shutdown() line 800
NS_ShutdownXPCOM_P(nsIServiceManager * 0x00000000) line 826 + 11 bytes
ScopedXPCOMStartup::~ScopedXPCOMStartup() line 553 + 12 bytes
XRE_main(int 0x00000003, char * * 0x003f79d8, const nsXREAppData * 0x0040301c kAppData) line 2337
main(int 0x00000003, char * * 0x003f79d8) line 61 + 19 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

in js_FinalizeStringRT(JSRuntime * 0x00fb0068, JSString * 0x02d8c538) line 2715 + 13 bytes

+	str->chars	0x03651420 "file:///C:/work/mozilla/builds/ff/trunk-test/mozilla/extensions/venkman/xpi/venkman-0.9.85.xpi"
	valid	0x00000001

with venkman installed, I now longer crash immediately nor on shutdown in bug 317815.
Bob: can you test with just this bug's patch, and not the patch for bug 322001?  The crash you show looks like it might be a regression from that bug's patch.  This bug's patch should not cause any such crash.

/be

Comment 7

12 years ago
(In reply to comment #6)

I cleaned out my tree and still see the heap assert when closing the browser after installing venkman so this is an existing issue not caused by bug 322001. I tried to reproduce with another extension and could not although the ASSERTION: CheckForDeactivation called from wrong thread! was still present. I looked for other bugs on this but couldn't find an exact match but Bug 155245 has the same mozilla assert.
(In reply to comment #7)
> (In reply to comment #6)
> 
> I cleaned out my tree and still see the heap assert when closing the browser
> after installing venkman so this is an existing issue not caused by bug 322001.

Please file a new bug.

> I tried to reproduce with another extension and could not although the
> ASSERTION: CheckForDeactivation called from wrong thread! was still present. I
> looked for other bugs on this but couldn't find an exact match but Bug 155245
> has the same mozilla assert.

Could you comment in that bug?  It seems old and stale, perhaps something has changed to make it bite anew, or possibly its symptom that you see deserves a new bug, but for now that one will do.  Timeless may be able to help.

Just to confirm: with only this bug's patch applied, you can't reproduce the crash reported in bug 317815?  And without this bug's patch, you can?

/be

Comment 9

12 years ago
(In reply to comment #8)

> 
> Please file a new bug.
> 

Bug 322131 

> 
> Just to confirm: with only this bug's patch applied, you can't reproduce the
> crash reported in bug 317815?  And without this bug's patch, you can?

Yes and Yes.
Bob: great, thanks -- and thanks to eagle-eyed Igor.  This is a no-brainer for 1.8.0.1.  I'm checking Igor's patch into the trunk immediately.

/be
Flags: blocking1.8.0.1? → blocking1.8.0.1+
Created attachment 207390 [details] [diff] [review]
version of Igor's patch that I committed
Attachment #207390 - Flags: review+
Attachment #207390 - Flags: approval1.8.1?
Attachment #207390 - Flags: approval1.8.0.1?

Updated

12 years ago
Attachment #207296 - Flags: approval1.8.1?
Attachment #207296 - Flags: approval1.8.0.1?
Fixed on trunk.  Nominations for the patch should keep this bug on the radar until the patch is approved for landing.

/be
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: checked into trunk Jan 2 -- baking
Anyone see any js_NewGCThing talkback from trunk builds newer than the day this patch went into the trunk?

/be

Comment 14

12 years ago
(In reply to comment #13)

No. I see 4 with js_NewGCThing in the signature for builds from 20051229 and 20051218, but the small number does not yet make me comfortable. I'll try to keep up with it.
This patch fixed bug 312588.  It's a pure fix for real bugs.  I say it's baked.

/be

Updated

12 years ago
Blocks: 312588
Comment on attachment 207390 [details] [diff] [review]
version of Igor's patch that I committed

a=dveditz for drivers.
do we need this for the 1.7 branch as well?
Attachment #207390 - Flags: approval1.8.1?
Attachment #207390 - Flags: approval1.8.1+
Attachment #207390 - Flags: approval1.8.0.1?
Attachment #207390 - Flags: approval1.8.0.1+
(In reply to comment #16)
> (From update of attachment 207390 [details] [diff] [review] [edit])
> a=dveditz for drivers.
> do we need this for the 1.7 branch as well?

1.7 and Aviary 1.0.1 branches, you mean? :-P

No, this depends on allocation of JSFunction and object slots via the GC, which came in during 1.8.

/be

Fixed on branches.

/be
Keywords: fixed1.8.0.1, fixed1.8.1

Updated

12 years ago
Flags: testcase-
Is there a way for QA to verify this on the branch?

Comment 20

12 years ago
from talkback: last trunk builds with js_NewGCThing signatures are 20051230 or earlier, last 1.5.x builds from 2006 with js_NewGCThing signatures are 20060109 or earlier.

Comment 21

12 years ago
v by bonsai:

2006-01-02 17:41	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.131 	8/5  	Fix from Igor Bukanov <igor.bukanov@gmail.com> to reorder allocations to avoid pigeon-hole problem (322045, r=me).

2006-01-10 17:15	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.117.2.9 	MOZILLA_1_8_BRANCH  	8/5  	Fix for 322045, a=dveditz.

2006-01-10 17:15	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.117.2.7.2.2 	MOZILLA_1_8_0_BRANCH  	8/5 
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.1, fixed1.8.1 → verified1.8.0.1, verified1.8.1
Flags: blocking1.7.13-
Flags: blocking-aviary1.0.8-

Updated

12 years ago
Summary: GC hazard during function allocation. → CVE-2006-0293 GC hazard during function allocation.
Group: security

Comment 22

12 years ago
Advisory 2006-01 http://www.mozilla.org/security/announce/mfsa2006-01.html indicates that this flaw is also in Mozilla but no references seem to be here about fixing this.  This Advisory does indicate to disable javascript in Mail which is commonly done and prudent.  However, it does not address this for Browser and javascript is needed by just about everyone to access at least one web site.  Hence, disabling this in Browser to work around this flaw is impractical.

Is someone fixing this for Mozilla Suite and when will it be available?

thanks,
r

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.1

Updated

10 years ago
Duplicate of this bug: 286407
You need to log in before you can comment on or make changes to this bug.