Feature proposal: Mitigation for Client-Side Prototype Pollution

[shhnjk]

Client-Side Prototype Pollution (will refer as PP) are increasing. For example, this shows many libraries are vulnerable to PP just by parsing location.search.

But essentially, a PP bug can be introduced if a JS application has a custom function to merge/clone objects, where an object is controlled by an attacker.

For example, following is a vulnerable code:

function isObject(obj) {
  return obj !== null && typeof obj === 'object';
}

function merge(a, b) {
  for (let key in b) {
    if (isObject(a[key]) && isObject(b[key])) {
      merge(a[key], b[key]);
    } else {
      a[key] = b[key];
    }
  }
  return a;
}

const internalObj = {a: 1, b: 2};
const userSuppliedObj = JSON.parse('{"__proto__":{"polluted": 1}}');
merge(internalObj, userSuppliedObj);
const newObj = {};
console.log(newObj.polluted); // 1

Proposal

Create a Document Policy to stop PP.

Name: restrict-prototype (bikeshed).
Keywords: globalObject, allObjects, and (if possible) objectsIfImplicit (all bikeshed).

Note: I'm still unsure if Arrays should be protected. But if so, we can add same keywords for Arrays.

globalObject

This allows developers to set prototype of local objects, but not for the global Object.

For example:

let obj = {};
obj.__proto__.test = 1;
obj.test; // 1
Object.test; // undefined

This way, prototype addition/modification only affects a local object but not all other objects.

allObjects

This ignores prototype modification of all objects. Which is equivalent of Object.freeze(Object.prototype);

For example:

let obj = {};
obj.__proto__.test = 1;
obj.test; // undefined
Object.test; // undefined

objectsIfImplicit

This allows prototype modification only when explicitly specified.
Explicit prototype modification is defined by property set with the dot notation property accessor. Setting prototype other than the dot notation will be ignored (such as bracket notation).

For example:

let obj = {};
obj["__proto__"]["test"] = 1;
obj.test; // undefined
Object.test; // undefined

obj.__proto__.test = 1;
obj.test; // 1
Object.test; // 1

I'm not sure how technically feasible this option is (in terms of browser implementation), but it seems possible at least in Chromium.

This option is beneficial because most (if not all) of PP bugs are introduced with the code which accesses prototype with bracket notation (obviously this is unintentional). This option looks like a best way to eliminate PP while allowing developers to intentionally modify object prototypes.

Special thanks to @koto for some ideas.
CC: @mikewest and @arturjanc

[mikewest]

I don't have a strong opinion about the solution, other than to note that Document Policy seems like a reasonable place to put a control like this one. I've asked some folks who know more about JavaScript internals than I to (and developer expectations regarding things like __proto__) to weigh in.

I have a few questions:

1. To what extent would this require changes to ecmascript? It seems like we would at least need similar callback structures to what we use for eval() in CSP?
2. To what extent is prototype modification of Object expected? That is, could we turn it off by default if it's really a problem?
3. https://tc39.es/ecma262/#sec-property-accessors doesn't really seem to distinguish between dot- and bracket-notation. Are they sufficiently distinct codepaths that objectsIfImplicit could be easily implemented? (Yes, this is the same question you asked.) Further, assuming this is possible, is it a reasonable distinction to introduce? I don't think member accessors are distinguished in this way anywhere else in the language.

I'll see if I can find folks with (informed) opinions to weigh in.

[mathiasbynens]

https://github.com/tc39/proposal-freeze-prototype is an early-stage proposal for freezing prototypes. It achieved Stage 1 in 2019. If it were to progress further and become a standard, it sounds like this particular document policy feature proposal could be nicely built on top. cc @bakkot

[bakkot]

sounds like this particular document policy feature proposal could be nicely built on top

Well, the allObjects one, yes. [Edit: actually no. The proposal is about freezing the prototype slot, not objects which are prototypes. It prevents x.__proto__ = something, not x.__proto__.property = something.]

For the others:

globalObject

There is no way that behavior could work with ECMAScript. The prototype of obj is the same value as the prototype of Object and all other objects. If obj.test is 1, then then Object.test must be as well.

objectsIfImplicit

As written, that can't work without a fairly significant change to ECMAScript, since currently obj.foo and obj['foo'] are always equivalent. But you could just disable the __proto__ accessor entirely (as node now allows you to do). Anyone who wanted to get the prototype could use Object.getPrototypeOf, which is not subject to these issues.

Of course, doing that would break substantially all webpages, so I don't know how feasible it would be for anyone to use.

[shhnjk]

1. To what extent would this require changes to ecmascript?

Not sure 🙁

2. To what extent is prototype modification of Object expected? That is, could we turn it off by default if it's really a problem?

The hard part of this problem is that, while we can use Object.freeze(Object.prototype) to block PP, we can't meaningfully debug what will break (thus we can't tell if prototype modification is expected in real world application). By adding Document Policy, my hope is that people can start debugging with report-only mode, and if we see that this doesn't cause almost any application to break, we can turn it off by default in the future.

3. https://tc39.es/ecma262/#sec-property-accessors doesn't really seem to distinguish between dot- and bracket-notation. Are they sufficiently distinct codepaths that objectsIfImplicit could be easily implemented? (Yes, this is the same question you asked.) Further, assuming this is possible, is it a reasonable distinction to introduce? I don't think member accessors are distinguished in this way anywhere else in the language.

Yes, this seem difficult to implement currently (both spec and browser implementation). However, this is probably useful in the future too. For example, many developers are bypassing 'unsafe-eval' in CSP with window[functionName].apply(window, [args]). So I think dynamic object reference using bracket notation will continued to be a problem in security 😊

sounds like this particular document policy feature proposal could be nicely built on top

Well, the allObjects one, yes. For the others:

globalObject

There is no way that behavior could work with ECMAScript. The prototype of obj is the same value as the prototype of Object and all other objects. If obj.test is 1, then then Object.test must be as well.

objectsIfImplicit

As written, that can't work without a fairly significant change to ECMAScript, since currently obj.foo and obj['foo'] are always equivalent. But you could just disable the __proto__ accessor entirely (as node now allows you to do). Anyone who wanted to get the prototype could use Object.getPrototypeOf, which is not subject to these issues.

Of course, doing that would break substantially all webpages, so I don't know how feasible it would be for anyone to use.

Given @bakkot's feedback, maybe it's better to just introduce allObjects for now, and see how that works? If we get feedback from developers that this doesn't work, we can look for alternative with those feedback. But either way, I think introducing at least allObjects primitive is important for debugging, deployability and potentially turning PP off by default. The good thing about allObjects is that once developers know it won't cause an issue, we can make a polyfill with Object.freeze(Object.prototype).

WDYT?

[bakkot]

Oh, I should mention also that doing Object.freeze(Object.prototype), while it is coherent from the language's perspective, will also break a lot of pages. This is because of a (IMO) very stupid part of the language (the "override mistake") which we can't change. Specifically:

Object.freeze(Object.prototype);
let x = {};
x.toString = 0;
console.log(x.toString === 0); // false. writing to an own property fails if the object inherits that property from its prototype and its prototype has that property as non-writeable.

So it would probably have to be something slightly different.

For example, Object.preventExtensions would prevent adding any new properties to Object.prototype while still keeping existing properties as writable, which would not trigger the issue in my code snippet here. Or we could explore more exotic possibilities, like making writing to properties on Object.prototype fail without technically marking it non-writeable, which would require some integration with the ECMAScript spec but is technically allowed (for example, module namespace objects work like this, for reasons which are coherent but not necessarily obvious; I'm happy to explain more about the relevant invariants if you want).

[shhnjk]

Oh, I should mention also that doing Object.freeze(Object.prototype), while it is coherent from the language's perspective, will also break a lot of pages. This is because of a (IMO) very stupid part of the language (the "override mistake") which we can't change. Specifically:

Object.freeze(Object.prototype);
let x = {};
x.toString = 0;
console.log(x.toString === 0); // false. writing to an own property fails if the object inherits that property from its prototype and its prototype has that property as non-writeable.

I think this is the behavior that we want (or at least it's good from security perspective). Because if we have a method where it allows overwrite of existing property, those might be still vulnerable to PP if developers called Object.preventExtensions after including libraries which might add new prototype to Object.

However, if it turns out that allObjects (i.e. Object.freeze(Object.prototype)) won't work in real applications, then we can look for alternatives that you suggested ☺

[bakkot]

if it turns out that allObjects (i.e. Object.freeze(Object.prototype)) won't work in real applications

I promise you that it will not work in real applications; there's no need to run the experiment. The code snippet I wrote is very, very common. (Note that it's not attempting to change the value of toString on Object.prototype, just to set it on x. Which fails.)

[shhnjk]

Got it.

So maybe we expose 2 keywords?

1. objectFreeze
   Where the browser will perform Object.freeze(Object.prototype).

2. objectPreventExtensions
   Where the browser will perform Object.preventExtensions(Object.prototype).

Maybe doing experiment with those 2 would work?

For Object, all built-in properties are functions, so Object.preventExtensions can prevent PP with Document Policy.

However, if we decide to do the same for Array, Array has length property which isn't function, so that will be vulnerable to PP even with preventExtensions. But we can look into it later if we decide to protect Arrays.

[bakkot]

Array has length property which isn't function, so that will be vulnerable to PP even with preventExtensions

length is an own property of every array, not a property on Array.prototype. And locking Array.prototype down isn't feasible, since polyfills need to add new properties to it as they get added to the language. (This differs from Object.prototype because we [that is, TC39] will almost certainly never add a new property to Object.prototype, so there's nothing to polyfill.)

which isn't function

I don't really understand what the function-vs-data distinction has to do with prototype pollution.

[shhnjk]

Array has length property which isn't function, so that will be vulnerable to PP even with preventExtensions

length is an own property of every array, not a property on Array.prototype.

Oh, yeah 😊

And locking Array.prototype down isn't feasible, since polyfills need to add new properties to it as they get added to the language. (This differs from Object.prototype because we [that is, TC39] will almost certainly never add a new property to Object.prototype, so there's nothing to polyfill.)

Not sure if I understand this. Polyfill will just be Object.preventExtensions(Array.prototype). So we don't have to track new changes, unless there is a new property that affects all arrays which isn't a function.

which isn't function

I don't really understand what the function-vs-data distinction has to do with prototype pollution.

Attacker's entry point is method like JSON.parse. So the only things they can alter/inject is data, but they can't add/change functions. Therefore, exploitable places are properties that return data. Of couse, if there are properties in applications which will be taken to eval, but as long as we call Object.preventExtensions(Array.prototype) in the beginning, the only place attackers can inject data will be built-in properties (which are functions, so changing that to data would only make an application throw, I think).

[bakkot]

Not sure if I understand this. Polyfill will just be Object.preventExtensions(Array.prototype).

I mean, people will want to polyfill new JS language features, not this feature. That is, when JS adds a new method to Array.prototype, polyfills of that method will try to add it to Array.prototype. And if you've done Object.preventExtensions(Array.prototype), that won't work.

[shhnjk]

Not sure if I understand this. Polyfill will just be Object.preventExtensions(Array.prototype).

I mean, people will want to polyfill new JS language features, not this feature. That is, when JS adds a new method to Array.prototype, polyfills of that method will try to add it to Array.prototype. And if you've done Object.preventExtensions(Array.prototype), that won't work.

I think that's a tradeoff that developers/security engineers has to make 😊 But if they are calling Object.preventExtensions(Array.prototype) to polyfill Document Policy, they can just add polyfill of new feature before the Object.preventExtensions(Array.prototype) call too.

@terjanq, do you think something like #33 (comment) might work in production?

@clelland, how does new addition to document policy look like in terms of spec?

[syg]

I'm not clear on why this needs to be a document policy instead of being done programmatically, if the behavior is equivalent to calling freeze and preventExtensions. Is the security model different?

#33 (comment) suggests it has to do with a better debugging experience, which I don't quite understand either. What is a report-only mode?

[shhnjk]

Please see:
https://wicg.github.io/document-policy/#report-only

But to elaborate, you are right that these can be programmatically done (at least for the proposal in #33 (comment)). However, for a real-world application to enable such code programmatically, they would first write a tool to scan all of dependencies and verify that the code change doesn't affect the application in any way (and scan for each application). That might be an expensive cost for a mitigation.

But if we can roll out a report-only mode, we get benefit of:

1. People experimenting with the API without impacting applications, and possibly get feedback on whether or not it worked.
2. Provides easy tool to debug which code would break, before enforcing the mitigation. This makes the mitigation more deployable.

[terjanq]

As written, that can't work without a fairly significant change to ECMAScript, since currently obj.foo and obj['foo'] are always equivalent. But you could just disable the __proto__ accessor entirely (as node now allows you to do). Anyone who wanted to get the prototype could use Object.getPrototypeOf, which is not subject to these issues.

Of course, doing that would break substantially all webpages, so I don't know how feasible it would be for anyone to use.

That seems as the best solution for global Prototype Pollution. That would still have to apply to any other native types such as Function, String, Array, since it's easy to reference those prototypes as well. Not only __proto__ needs to be inaccessible but obj.constructor.prototype as well. And should work similarly as CSP so that empty iframes inherit the setting.

For the local pollution, as mentioned, this is only concern for a specific application and the application can prevent themselves in multiple ways (for example passing an object with frozen __proto__ so that local pollution is not possible, or creating a proxy that will block it temporarily). The same can't be applied for global pollution since it's impossible to entirely overwrite native prototypes.

But per my understanding the __proto__ block would fix both at the same time, since it is always required to access the prototype of the object via Object.getPrototypeOf, which acts exactly as Trusted Types would work, no?