487846 - caching wrong scope shape due to resolve hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Igor Bukanov]

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.

Comment 1

[Daniel Veditz [:dveditz]]

Since we need it fixed on moz-central first anyway we can hold off on the blocking nom until you know more.

Comment 2

[Igor Bukanov]

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.

Comment 3

[Igor Bukanov]

Attachment: v1

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.

Comment 4

[Igor Bukanov]

Attachment: v2

Here comes smaller patch. I will comment on it after some testing.

Comment 5

[Igor Bukanov]

Attachment: v3

fixing bogus assert in v2

Comment 6

[Igor Bukanov]

Attachment: v4

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.

Comment 7

[Brendan Eich [: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

Comment 8

[Igor Bukanov]

Attachment: v5

fixing nits and merging with trunk changes

Comment 9

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/9995065c42dd

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9995065c42dd

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/872f676e5d1a

Comment 12

[Daniel Veditz [:dveditz]]

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?

Comment 13

[Igor Bukanov]

(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.