1771381 - (CVE-2022-2200) Don't copy attributes from the prototype

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Andrew McCreight [:mccr8]]

One of the later phases of the Pwn2Own exploit, described in section 3.6 of the writeup, involves session store. By this point, the exploit has managed to contaminate Object.prototype in the JSM compartment by setting an attacker controlled attribute to an attacker controlled value (that was structured cloned).

They were able to leverage this into further control of the parent process because of the code here:

    for (let name in data) {
      if (!ATTRIBUTES_TO_SKIP.has(name)) {
        tab.setAttribute(name, data[name]);
      }
    }

This iterates over all of the properties of data, which is yet another Object being used as a map. This kind of for ... in iterates over properties on the prototype, too, so the attacker's values are copied onto the tab being restored, causing further badness.

There's no actual reason we want to copy properties from anything further up the prototype chain, so we could iterate over Object.entries(data) instead of data. I looked a bit into changing data to a Map, but it looks like it might come from JSON we're storing somewhere so it didn't seem easy to change. (Another place kmag's idea about parsing into JSON without a prototype might have helped.)

Comment 1

[Andrew McCreight [:mccr8]]

I looked over other places in SessionStore that do a for in loop, in case there are other places we could end up copying random junk from the prototype. One of the data structures is SessionStoreInternal._windows. Another is SessionStoreInternal._statesToRestore. It looks like maybe in SessionStoreInternal.getCurrentState() that attacker controlled values from Object.prototype can end up contaminating the state. I'm not sure how bad that is.

There's also a place where for in is used to iterate over winData.extData Maybe this is kind of hopeless, though, because there are lots of places that read from extData, like in getCustomWindowValue.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1771381 - Use entries in TabAttributesInternal.set().

Comment 3

[Andrew McCreight [:mccr8]]

I guess I'll call this sec-moderate, but that might be a bit high, given that it takes a few other steps to get to this, and there may be plenty of other similar issues lying around.

Comment 4

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #1)

I looked over other places in SessionStore that do a for in loop, in case there are other places we could end up copying random junk from the prototype. One of the data structures is SessionStoreInternal._windows. Another is SessionStoreInternal._statesToRestore. It looks like maybe in SessionStoreInternal.getCurrentState() that attacker controlled values from Object.prototype can end up contaminating the state. I'm not sure how bad that is.

There's also a place where for in is used to iterate over winData.extData Maybe this is kind of hopeless, though, because there are lots of places that read from extData, like in getCustomWindowValue.

We could eslint to forbid for (var k in obj) - with a bit of work, only if obj[k] is also accessed inside the loop, and update all of them to use Object.entries. Is there a reason not to, other than "it's work"? I'm not sure how to understand "kind of hopeless" :-)

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use entries in TabAttributesInternal.set(). r=Gijs
https://hg.mozilla.org/integration/autoland/rev/cf40e7b79bb18c1e4c18726eb603702304cc531e
https://hg.mozilla.org/mozilla-central/rev/cf40e7b79bb1

Comment 6

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #4)

We could eslint to forbid for (var k in obj) - with a bit of work, only if obj[k] is also accessed inside the loop, and update all of them to use Object.entries. Is there a reason not to, other than "it's work"?

No, I think we should definitely do that. It's almost always the wrong thing at this point.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to :Gijs (he/him) from comment #4)

We could eslint to forbid for (var k in obj) - with a bit of work, only if obj[k] is also accessed inside the loop, and update all of them to use Object.entries. Is there a reason not to, other than "it's work"? I'm not sure how to understand "kind of hopeless" :-)

I just meant that places like getCustomWindowValue will end up reading data off Object.prototype, even without a for in loop. I don't know how bad that is.

Comment 8

[Frederik Braun [:freddy]]

Developing an eslint rule to disallow for .. in and rewrite into Object.entries might be doable in ~2 days of my time, though I'm not entirely sure about the edge cases and potential breakage depending on the intent of the specific for-loop...

Comment 9

[Jan de Mooij [:jandem]]

I also think it would be good to enforce this. for..in not only includes the proto chain, but the engine also has complicated code to deal with properties that are deleted while iterating (and JS engines don't exactly agree on this in some edge cases, as for..in predates proper specs). Hopefully frontend code usually doesn't delete while iterating, but still.

Comment 10

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #7)

(In reply to :Gijs (he/him) from comment #4)

We could eslint to forbid for (var k in obj) - with a bit of work, only if obj[k] is also accessed inside the loop, and update all of them to use Object.entries. Is there a reason not to, other than "it's work"? I'm not sure how to understand "kind of hopeless" :-)

I just meant that places like getCustomWindowValue will end up reading data off Object.prototype, even without a for in loop. I don't know how bad that is.

I think given aKey is presumably a hardcoded string in most/all of the consumers of that method, and the property will likely / almost always exist on the object in question, that's probably an almost-non-existent issue. I mean, tbh, at this point it seems that because we've nuked changing Object.prototype in chrome code anyway, there's probably limited gains left from fixing consumer issues around it in JS. But based on Jan's comments, there are other reasons we might want a bug for the eslint rule.

(In reply to Frederik Braun [:freddy] from comment #8)

Developing an eslint rule to disallow for .. in and rewrite into Object.entries might be doable in ~2 days of my time, though I'm not entirely sure about the edge cases and potential breakage depending on the intent of the specific for-loop...

If the above makes sense, mind filing a (public, I guess) bug about this?

Comment 11

[Frederik Braun [:freddy]]

(Leaving the needinfo, this is merely a status report. I will conclude whether this is actually doable later)

I've spent a couple of hours this morning to prototype something, however eslint will not be able to identify if the right part of the "for ... in" is an Object at all. Maybe it's fine to also rewrite for all other cases given that ?Object.entries()` is somewhat useful for Arrays too?

I'm also hitting some limitations of their replace-API:
Replacing the left part with something like [key, value] and the right part with Object.keys(...) is pretty easy, but replacing the body of the loop seems really tricky, as there seems no simple way to nest through all variables (called Identifiers) below and replacing occurrences.

I have a prototype at https://astexplorer.net/#/gist/daa5cbc5dc0fceb246862760f55ee387/2d0c52d54947ba4dc07dd836ce4b67a67d6985d8, that I will get back to in a couple of days.

Comment 12

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from comment #11)

(Leaving the needinfo, this is merely a status report. I will conclude whether this is actually doable later)

I've spent a couple of hours this morning to prototype something, however eslint will not be able to identify if the right part of the "for ... in" is an Object at all. Maybe it's fine to also rewrite for all other cases given that ?Object.entries()` is somewhat useful for Arrays too?

For arrays I'd expect us to use a straight for loop (for (let i = 0; i < ary.length; i++)) if the index is important, or a for of loop otherwise (for (let item of someList)).

I'm also hitting some limitations of their replace-API:
Replacing the left part with something like [key, value] and the right part with Object.keys(...) is pretty easy, but replacing the body of the loop seems really tricky, as there seems no simple way to nest through all variables (called Identifiers) below and replacing occurrences.

I have a prototype at https://astexplorer.net/#/gist/daa5cbc5dc0fceb246862760f55ee387/2d0c52d54947ba4dc07dd836ce4b67a67d6985d8, that I will get back to in a couple of days.

Maybe Mark can suggest something?

I wouldn't mind not having an automated fix, but if there are a lot of instances in the tree perhaps not having one won't fly?

Comment 13

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddy] from comment #11)

(Leaving the needinfo, this is merely a status report. I will conclude whether this is actually doable later)

I've spent a couple of hours this morning to prototype something, however eslint will not be able to identify if the right part of the "for ... in" is an Object at all. Maybe it's fine to also rewrite for all other cases given that ?Object.entries()` is somewhat useful for Arrays too?

I think we also don't want for-in loops for arrays. It has the same problems there as it does for objects. for-of loops works to iterate over array values, and ary.keys() and ary.entries() work when you need the indices too.

I'm also hitting some limitations of their replace-API:
Replacing the left part with something like [key, value] and the right part with Object.keys(...) is pretty easy, but replacing the body of the loop seems really tricky, as there seems no simple way to nest through all variables (called Identifiers) below and replacing occurrences.

The replace API is intentionally simple and meant to work only for simple cases. It possibly isn't a good idea to use it for this.

Comment 14

[Frederik Braun [:freddy]]

The API is indeed quite simple. I was hoping to get somewhere with their recent Code Path Analysis support, but even that seems to tricky.
Meanwhile, I have noticed that we have too many occurences of for...in loops, and I suppose that we don't want to disallow something with a linter if we can't get rid of it automatically.

I suppose we might have to abandon the code-rewriting aspect of this via eslint and should consider a different approach.

Comment 15

[Mark Banner (:standard8)]

(In reply to Frederik Braun [:freddy] from comment #14)

Meanwhile, I have noticed that we have too many occurences of for...in loops, and I suppose that we don't want to disallow something with a linter if we can't get rid of it automatically.

Searchfox implies only around a quarter of those are production code, and the rest are tests. I think it would need a quick verification test to make sure searchfox results are correct, but maybe we could at least ban for..in for production code?

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 9278382 [details]
Bug 1771381 - Use entries in TabAttributesInternal.set().

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a simple fix for part of the Pwn2Own 2022 exploit chain.
• User impact if declined: possible security issues
• Fix Landed on Version: 102
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I don't know how well session restore is tested, but this change is rather simple.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 9278382 [details]
Bug 1771381 - Use entries in TabAttributesInternal.set().

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9278382 [details]
Bug 1771381 - Use entries in TabAttributesInternal.set().

Approved for 91.11esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/13afef538349

Comment 20

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddy] from comment #14)

The API is indeed quite simple. I was hoping to get somewhere with their recent Code Path Analysis support, but even that seems to tricky.
Meanwhile, I have noticed that we have too many occurences of for...in loops, and I suppose that we don't want to disallow something with a linter if we can't get rid of it automatically.

We disallow a lot of things we can't get rid of automatically. That said, if there are too many occurrences to fix by hand, I have a separate rewriting framework I tend to use for large/complicated rewrites that ESLint can't handle.

Comment 21

[Andrew McCreight [:mccr8]]

Does somebody want to file a new bug for this for-in elimination discussion?

Another possible piece of work would be to rewrite this session store code to not use Objects as maps everywhere. That might be a bit of work though.

Comment 22

[Frederik Braun [:freddy]]

(In reply to Kris Maglione [:kmag] from comment #20)

We disallow a lot of things we can't get rid of automatically. That said, if there are too many occurrences to fix by hand, I have a separate rewriting framework I tend to use for large/complicated rewrites that ESLint can't handle.

Can you tell me some more about that separate rewriting framework?

Comment 23

[Kris Maglione [:kmag]]

(In reply to Frederik Braun [:freddy] from comment #22)

(In reply to Kris Maglione [:kmag] from comment #20)

We disallow a lot of things we can't get rid of automatically. That said, if there are too many occurrences to fix by hand, I have a separate rewriting framework I tend to use for large/complicated rewrites that ESLint can't handle.

Can you tell me some more about that separate rewriting framework?

I'm not sure how useful it would be to someone else, but I've used it for a bunch of large rewrites, adding features when they were necessary: https://github.com/kmaglione/m-c-rewrites

At this point it may make more sense to use jscodeshift, but I started having to do rewrites when our code wasn't actually standards-compliant, and even after that I couldn't find a framework that I was actually happy with.