Closed
Bug 313500
Opened 19 years ago
Closed 19 years ago
Unrooted access to "prototype" property
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8rc1
People
(Reporter: igor, Assigned: brendan)
Details
(Keywords: js1.6, verified1.8, Whiteboard: [sg:critical?])
Attachments
(2 files)
5.56 KB,
patch
|
igor
:
review+
shaver
:
superreview+
dveditz
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
2.28 KB,
text/plain
|
Details |
js_Interpret in jsinterp.c stores the result of accessing "prototype" property of function objects in unrooted "rval" local variable. This is GC-unsafe since there is object allocation before the result of accessing "prototype" property is rooted. If one recompiles JS shell with TOO_MUCH_GC defined then the following example gives segmentation fault: function F() { } var prepared = new Object(); F.prototype = {}; F.__defineGetter__('prototype', function() { var tmp = prepared; prepared = null; return tmp; }); new F(); There is a similar pattern is 2 places in jsobj.c with problematic access of "prototype" property. I suspect that there is a problem in fun_resolve from jsfun.c as well, but it may be actually GC-safe. On the other hand accessing the property in Exception from jsexn.c is GC safe since a script can not define a getter for "prototype" for Error object as it already defined as read-only property.
Assignee | ||
Updated•19 years ago
|
Assignee: general → brendan
Flags: blocking1.8rc1+
Keywords: js1.6
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8rc1
Assignee | ||
Comment 1•19 years ago
|
||
(In reply to comment #0) > There is a similar pattern is 2 places in jsobj.c with problematic access of > "prototype" property. I suspect that there is a problem in fun_resolve from > jsfun.c as well, but it may be actually GC-safe. No, those are both hazardous -- thanks for finding them. Are you reviewing all OBJ_GET_PROPERTY usage, or looking for classPrototypeAtom usage, or both? > On the other hand accessing > the property in Exception from jsexn.c is GC safe since a script can not define > a getter for "prototype" for Error object as it already defined as read-only > property. Yes, a saving grace of ECMA-262! Patch soon, thanks again. /be
Assignee: brendan → general
Assignee | ||
Comment 2•19 years ago
|
||
(In reply to comment #1) > (In reply to comment #0) > > On the other hand accessing > > the property in Exception from jsexn.c is GC safe since a script can not define > > a getter for "prototype" for Error object as it already defined as read-only > > property. > > Yes, a saving grace of ECMA-262! Although it is the permanent (ECMA DontDelete) attribute that saves us here. A read-only property can be deleted, and then its id rebound to a getter. So only the permanent attribute prevents an exploit such as this bug's testcase. /be
Assignee: general → brendan
Reporter | ||
Comment 3•19 years ago
|
||
(In reply to comment #1) > Are you reviewing all > OBJ_GET_PROPERTY usage, or looking for classPrototypeAtom usage, or both? I try to go through all cases of OBJ_GET_PROPERTY/ValueToString/ValueToSource in js/src/*.c as time permits. The case of classPrototypeAtom was just a common pattern easy to spot and sufficiently unique to justify a separated bug.
Assignee | ||
Comment 4•19 years ago
|
||
mrbkap, please have a look too. The comments tell the tale. /be
Attachment #200549 -
Flags: superreview?(shaver)
Attachment #200549 -
Flags: review?(igor.bukanov)
Reporter | ||
Updated•19 years ago
|
Attachment #200549 -
Flags: review?(igor.bukanov) → review+
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 200549 [details] [diff] [review] proposed fix Checked into trunk. /be
Attachment #200549 -
Flags: approval1.8rc1?
Comment on attachment 200549 [details] [diff] [review] proposed fix sr=shaver. Almost into XXX territory with the newborn-root hacking! Can you add a comment before GetClassPrototype stating the 'delegate-or-get-class' invariant that it expects its callers to uphold?
Attachment #200549 -
Flags: superreview?(shaver) → superreview+
Comment 7•19 years ago
|
||
Comment on attachment 200549 [details] [diff] [review] proposed fix a=dveditz
Attachment #200549 -
Flags: approval1.8rc1? → approval1.8rc1+
Updated•19 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 8•19 years ago
|
||
Fixed on branch and trunk. /be
Comment 9•19 years ago
|
||
Updated•19 years ago
|
Flags: testcase?
Comment 10•19 years ago
|
||
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8 → verified1.8
Comment 11•19 years ago
|
||
testcase+ to get this off my radar. when this is made public, i will check in the test.
Flags: testcase? → testcase+
Comment 12•18 years ago
|
||
This should have gone on the old branches
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Updated•18 years ago
|
Attachment #200549 -
Flags: approval1.7.14?
Attachment #200549 -
Flags: approval-aviary1.0.9?
Comment 13•18 years ago
|
||
Dan, has there already been an advisory for this, so I can release the patch in 'pseudo 1.7.15/1.0.10' ? How to procede with this one?
Comment 14•18 years ago
|
||
Yes, this was part of MFSA 2006-10 but wasn't explicitly mentioned. I'll go update that advisory.
Comment 15•18 years ago
|
||
verified fixed 1.9 windows/mac(ppc|tel)/linux 20060812
Status: RESOLVED → VERIFIED
Updated•18 years ago
|
Group: security
Comment 16•18 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-313500.js,v <-- regress-313500.js
Updated•16 years ago
|
Attachment #200549 -
Flags: approval1.7.14?
Attachment #200549 -
Flags: approval-aviary1.0.9?
You need to log in
before you can comment on or make changes to this bug.
Description
•