The default bug view has changed. See this FAQ.

Unrooted access to "prototype" property

VERIFIED FIXED in mozilla1.8rc1

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({js1.6, verified1.8})

Trunk
mozilla1.8rc1
js1.6, verified1.8
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments)

(Reporter)

Description

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

12 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

12 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

12 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

12 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

12 years ago
Created attachment 200549 [details] [diff] [review]
proposed fix

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

12 years ago
Attachment #200549 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 5

12 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 on attachment 200549 [details] [diff] [review]
proposed fix

a=dveditz
Attachment #200549 - Flags: approval1.8rc1? → approval1.8rc1+
Whiteboard: [sg:critical?]
(Assignee)

Comment 8

12 years ago
Fixed on branch and trunk.

/be
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Comment 9

12 years ago
Created attachment 200579 [details]
js1_5/Regress/regress-313500.js

Updated

12 years ago
Flags: testcase?

Comment 10

12 years ago
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8 → verified1.8

Comment 11

11 years ago
testcase+ to get this off my radar. when this is made public, i will check in the test.
Flags: testcase? → testcase+
This should have gone on the old branches
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Attachment #200549 - Flags: approval1.7.14?
Attachment #200549 - Flags: approval-aviary1.0.9?

Comment 13

11 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?
Yes, this was part of MFSA 2006-10 but wasn't explicitly mentioned. I'll go update that advisory.

Comment 15

11 years ago
verified fixed 1.9 windows/mac(ppc|tel)/linux 20060812
Status: RESOLVED → VERIFIED
Group: security

Comment 16

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-313500.js,v  <--  regress-313500.js
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.