Closed
Bug 487846
Opened 16 years ago
Closed 16 years ago
caching wrong scope shape due to resolve hooks
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
The current code for js_SetPropertyHelper contains the following sequence:
shape = OBJ_SHAPE(obj);
protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
&pobj, &prop);
...
js_FillPropertyCache(cx, obj, shape, 0, 0, obj, sprop, entryp);
That is, the code caches the found property with the shape value taken before running the resolve hooks for the class. Since the resolve hook could run a GC invalidating the shapes, it means that the cache would contain a bogus entry. A similar issue exists in js_GetPropertyHelper.
I do not know how exploitable is that, so to be sure I nominate the bug for 1.9.*.
This bug was discovered during testing of a patch for bug 487204, which addresses a related issue.
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Comment 1•16 years ago
|
||
Since we need it fixed on moz-central first anyway we can hold off on the blocking nom until you know more.
Flags: blocking1.9.0.10? → wanted1.9.0.x+
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee | ||
Comment 2•16 years ago
|
||
In addition to the resolve hooks js_AddScopeProperty can also mutate prematurely read shapes entries. The function calls the GC on shape overflow to see if the GC can free some shape numbers. But that completely regenerates the shapes in the process. From an evil script point of view it is much more easy to control then the case of the resolve hooks.
For example, such script can try to construct sequences like obj.p1 = 0; ... obj.pN = 0 with N approximately matching the number of the cache entries. Before executing that the script triggers the gc, then runs a code that creates enough shape garbage so that sequence would overflow the shape. After that one of the entries would contain the bogus shape value. Since that value would be numerically close to the overflow value, the evil script via creation of the controllable amount of new shapes cached shapes can get one that matches the bogus cache entry.
Assignee | ||
Comment 3•16 years ago
|
||
This patch is a refreshed version of the patch from bug 487204 comment 47 minus tvr rooting. As with the patches in that bug, more work is required here to make sure that SetPropHit|SetPropMiss from the recorder are called before any setter has a chance to run but after the object is unlocked.
Assignee | ||
Comment 4•16 years ago
|
||
Here comes smaller patch. I will comment on it after some testing.
Attachment #373464 -
Attachment is obsolete: true
Assignee | ||
Comment 5•16 years ago
|
||
fixing bogus assert in v2
Attachment #373845 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
The new version is a sync with TM tip of v3.
The main idea behind the patch is to always use the current shape when filling the cache. To support the predictive setters the patch uses a boolean flag which is only set when a new property is added after the initial property lookup has found nothing.
Attachment #373869 -
Attachment is obsolete: true
Attachment #374966 -
Flags: review?(brendan)
Comment 7•16 years ago
|
||
Comment on attachment 374966 [details] [diff] [review]
v4
>+ /*
>+ * The modfills counter is not exact. It increase if a getter or setter
s/increase/&s/
r=me with that, thanks.
/be
Attachment #374966 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 8•16 years ago
|
||
fixing nits and merging with trunk changes
Attachment #374966 -
Attachment is obsolete: true
Attachment #375021 -
Flags: review+
Assignee | ||
Comment 9•16 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 10•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 11•16 years ago
|
||
Keywords: fixed1.9.1
Comment 12•16 years ago
|
||
Do we need this on the 1.9.0 branch? or did the branch-relevant bits get addressed in the 1.9.0.11 patch for bug 487204?
Flags: blocking1.9.0.12?
Assignee | ||
Comment 13•16 years ago
|
||
(In reply to comment #12)
> Do we need this on the 1.9.0 branch?
I do not have a test case demonstrating how the bug can be used to subvert the runtime. But when I tried to construct a test case on the trunk, the bug 488414 was already fixed there which removed a possible way to exploit this bug.
Thus I suppose the bug should be if not a blocker, then at least wanted for 1.9.0.
Updated•16 years ago
|
Flags: blocking1.9.0.12?
Updated•16 years ago
|
Flags: in-testsuite-
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•