Closed Bug 487846 Opened 11 years ago Closed 11 years ago

caching wrong scope shape due to resolve hooks

Categories

(Core :: JavaScript Engine, defect, P2, critical)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

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?
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+
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Attached patch v2 (obsolete) — Splinter Review
Here comes smaller patch. I will comment on it after some testing.
Attachment #373464 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
fixing bogus assert in v2
Attachment #373845 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
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 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+
Attached patch v5Splinter Review
fixing nits and merging with trunk changes
Attachment #374966 - Attachment is obsolete: true
Attachment #375021 - Flags: review+
https://hg.mozilla.org/tracemonkey/rev/9995065c42dd
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/9995065c42dd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 484042
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?
(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.
Flags: blocking1.9.0.12?
Flags: in-testsuite-
Group: core-security
You need to log in before you can comment on or make changes to this bug.