If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

js_SuppressDeletedProperty must check for iteration mutations under lookupProperty

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: gal)

Tracking

({regression})

Other Branch
regression
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?], fixed-in-tracemonkey [critsmash:patch])

Attachments

(1 attachment, 1 obsolete attachment)

2.66 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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.
Group: core-security
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?
(Reporter)

Comment 3

7 years ago
(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.
(Reporter)

Comment 4

7 years ago
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;
}
(Reporter)

Comment 5

7 years ago
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.
(Reporter)

Updated

7 years ago
Summary: unbounded recursion with proxy->lookupProperty and the check for deleted properties under enumeration? → js_SuppressDeletedProperty must check for iteration mutations under lookupProperty
(Assignee)

Comment 6

7 years ago
Created attachment 449503 [details] [diff] [review]
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).
Assignee: general → gal
(Assignee)

Updated

7 years ago
Attachment #449503 - Flags: review?(igor)
(Reporter)

Comment 7

7 years ago
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
Attachment #449503 - Flags: review?(igor)
(Assignee)

Comment 8

7 years ago
Created attachment 449519 [details] [diff] [review]
patch
Attachment #449503 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #449519 - Flags: review?(igor)

Updated

7 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 9

7 years ago
Not shipped, not on trunk, fix in hand. No need to hide this.
Group: core-security
(Reporter)

Updated

7 years ago
Attachment #449519 - Flags: review?(igor) → review+
(Assignee)

Comment 10

7 years ago
http://hg.mozilla.org/tracemonkey/rev/db1bcf7b564a
(Assignee)

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?], fixed-in-tracemonkey

Updated

7 years ago
Whiteboard: [sg:critical?], fixed-in-tracemonkey → [sg:critical?], fixed-in-tracemonkey [critsmash:patch]
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Keywords: regression

Comment 11

7 years ago
http://hg.mozilla.org/mozilla-central/rev/db1bcf7b564a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
blocking2.0: ? → betaN+
You need to log in before you can comment on or make changes to this bug.