570352 - js_SuppressDeletedProperty must check for iteration mutations under lookupProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

With changes from the bug 569735 AFAICS there is a code path where a script can trigger native stack overflow. In particular, js_DeleteProperty calls js_SuppressDeletedProperty which in turn calls lookupProperty on the prototype of of the iterator object. But the latter can be a proxy function that in turn deletes a property of the object under enumeration leading to unbounded native recursion.

I do not have a working example demonstrating the problem at this moment hence the question mark for the title of the bug.

Comment 1

[Igor Bukanov]

I have to close this since there is another bug in js_SuppressDeletedProperty. It does not check that lookupProperty could mutate ni->props_end. If, for example, a proxy on the prototype would add and delete a property been enumerated then js_SuppressDeletedProperty would recurse and eventually the script can trigger a lot of ni->props_end-- leading to overflow etc.

Comment 2

[Mike Shaver (:shaver emeritus)]

Is that bug in anything we've shipped?  The deletion-suppression code was recently revamped completely, so I'm not sure if you're saying that the old (shipped) code is vulnerable and therefore we have to close it, or the new code is broken so we just need to block release on it.

Is an overflowed (underflowed, if by --?) ni->props_end exploitable?

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Is that bug in anything we've shipped?  The deletion-suppression code was
> recently revamped completely, so I'm not sure if you're saying that the old
> (shipped) code is vulnerable and therefore we have to close it, or the new code
> is broken so we just need to block release on it.

The bug is only in the new code and can only be exposed using recently added proxies. So we should just block the release.

> 
> Is an overflowed (underflowed, if by --?) ni->props_end exploitable?

This is hard to tell. The relevant code path is:

                    if (idp == ni->props_cursor) {
                        ni->props_cursor++;
                    } else {
                        memmove(idp, idp + 1, (ni->props_end - (idp + 1)) * sizeof(jsid));
                        ni->props_end--;
                    }

A script via recursion can trigger  the execution of the second path a number of times and have some control over idp == ni->props_cursor checks. It would lead to something like memove(x, y, 4GB - some bytes). I do not know if that could be exploited.

Comment 4

[Igor Bukanov]

Here is an example that crashes the js shell in memmove:

var counter = 10;

var skip = false;
var p = Proxy.create({
    has: function(name) {
        if (name == 'y' && !skip) {
            print(1);
	    if (--counter == 0)
                return false;
            skip = true;
            obj.y = 2;
            skip = false;
            delete obj.y;
	}
	return false;
    },

    enumerate: function() { return []; }
});
    

var obj = { x: 0, y: 1, __proto__: p};

for (var i in obj) {
    delete obj.y;
}

Comment 5

[Igor Bukanov]

I am changing the title of the bug - there is no unbounded recursion here. Any script that lookupProxy may run will do the recursion checks on its own. So there is only memmove bug.

Comment 6

[Andreas Gal :gal]

Attachment: patch

Start over if a deletion occurs while handling the deletion. This is so ridiculously rare that I didn't bother trying to fix the state (walkind idp to the left should be sufficient, but its more complex).

Comment 7

[Igor Bukanov]

Comment on attachment 449503 [details] [diff] [review]
patch

>+            jsid *props_end = ni->props_end;
>+            for (jsid *idp = ni->props_cursor; idp < props_end; ++idp) {
...
>                     /*
>+                     * If lookupProperty or getAttributes above removed a property from
>+                     * ni, start over.
>+                     */
>+                    if (props_end != ni->props_end)
>+                        goto again;
>+
>+                    /*
>                      * No property along the prototype chain steppeded in to take the
>                      * property's place, so go ahead and delete id from the list.
>                      * If it is the next property to be enumerated, just skip it.
>                      */
>                     if (idp == ni->props_cursor) {
>                         ni->props_cursor++;
>                     } else {
>                         memmove(idp, idp + 1, (ni->props_end - (idp + 1)) * sizeof(jsid));
>                         ni->props_end--;

Suppose here the property is deleted in lookupProperty when idp == ni->props_cursor. Then the innermost recursive js_SuppressDeletedProperty can trigger ni->props_cursor++. If the previous invocation of js_SuppressDeletedProperty was for id when original(ni->props_cursor) + 1 == idp, then that would again increase ni->props_cursor. This way a script can move ni->props_cursor into ni->props_end. If the outermost js_SuppressDeletedProperty will see at this point that idp != ni->props_cursor then it will do ni->props_end-- leading to a situation when ni->props_cursor > ni->props_end.

Due to the rest of code consistently using ni->props_cursor < ni->props_end and not ni->props_cursor != ni->props_end to check the end of iterations this ni->props_cursor > ni->props_end is harmles. But this rather fragile so I prefer to see more bulletproof solution. 


Then the check props_end != ni->props_end

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Andreas Gal :gal]

Not shipped, not on trunk, fix in hand. No need to hide this.

Comment 10

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/db1bcf7b564a

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/db1bcf7b564a