Say more about "deleting the constructor property"?

[bakkot]

The current shape of this proposal says it would (when enabled) "delet[e] the __proto__ and constructor properties".

I know what "deleting __proto__" means: it's a single getter/setter pair on Object.prototype, and it can indeed be outright deleted before any code executes (as for example Deno already does). That makes sense.

I don't know what "deleting constructor" means. constructor is an own property of the prototype created along with any function, as in

Object.hasOwn((function(){}).prototype, 'constructor') // true

It's also a property of the prototypes of most built-ins, of course, including Object.prototype, Array.prototype, etc.

Is the proposal that this property would no longer be created, but instead a symbol-named property would be created in its place, and it would be moved on all built-ins? And also all static access of .constructor would be changed at parse time - even when accessing properties not created by the engine, like if I did x = { constructor: "something" }; x.constructor === 'something'? Or is there some other proposal?

Also, I've encountered security vulnerabilities involving dynamic access of __proto__ often enough to appreciate the scale of the security issues there, but I'm less familiar with examples involving dynamic access of constructor. Some concrete examples, and a sense of the relative scale of the two issues, would be very helpful. Especially since my intuition is that changing constructor is going to be a lot more problematic than changing __proto__.

[salcho]

To explain why constructor must be dealt with, we split prototype pollution bugs in two variants that we call shallow and deep access.

Bugs with shallow access can only chain a fixed or limited number of properties together, for example the statement obj[kOne][kTwo] allows pollution via obj['__proto__']['foo']. If __proto__ is not there anymore, then this code is no longer vulnerable. constructor can't be used here because obj['constructor']['prototype'] can't actually set any properties on that prototype. The attacker will need to find a similar statement, but with 3 or more bracket pairs. These bugs are more rare compared with the other variant, but they do come up every now and then (all it takes is chain 2 or more bracket pairs together). I'd hazard a guess of 1 shallow for every 5-10 deep bugs. After scanning a huge volume of code, we detected 10.000 instances of 2 brackets and 10 instances of 4 bracket pairs, without including a = obj[kOne]; b = a[kTwo] constructs. Here is an exploitable, real-world example for clarity.

Bugs with deep access are those that follow an object hierarchy recursively or iteratively, so they don't have a limit on how many properties they can chain together. These bugs are common and a lot more versatile to exploit. If __proto__ is not available, constructor is the only avenue to reach and pollute prototypes. Generally speaking, for most exploits one encounters involving dynamic access of __proto__, there is an equivalent exploit that can use constructor to reach the prototype. To see this in action, ctrl+f for 'constructor' in https://github.com/BlackFan/client-side-prototype-pollution

The following is a recursive function that is vulnerable to pollution through deep access. Removing __proto__ in this application will not make the bug go away.

function merge(target, source) {
  for (var key in source) {
      if ( typeof source[key] === 'object' )  {
        if(target[key] === undefined) target[key] = {};
        target[key] = merge(target[key], source[key]);
      } else {
        target[key] = source[key];
      }
    }
}

An example exploit is merge({}, {'constructor': {'prototype': {'foo': 123}}}). Functions that follow this structure are extremely popular.

I want to call out that the statement above (constructor has to be dealt with to make exploits moot) has a more general theorem which is cut all routes to the prototype to make exploits moot. If removing constructor proves too difficult, we should consider removing prototype from constructor, which will also work. I don't fully understand the intricacies of removing constructor, so any feedback on this point would be great.

Removing constructor in this context means what you mentioned above: keep the existing behavior exactly as it is now, but make the constructor's getter accessible through a symbol-named property rather than a string property. Apply this to all built-in prototypes.

As for automatically refactoring code at parse-time, I don't think we'd have enough information to accurately know when a constructor or __proto__ is user-defined. In those cases, developer would need to opt out of automatic refactoring.

[bakkot]

Thanks for the example.

I don't fully understand the intricacies of removing constructor, so any feedback on this point would be great.

Unlike __proto__, constructor is just a normal string-named property. Built-in prototypes have such a property by default, as do the prototypes of all user-defined classes and functions. "removing" it means changing all the built-ins so they call it something else, as well as changing what happens when you define a class or function so that it's called something else there. This is a much more complex operation than removing __proto__.

Removing constructor in this context means what you mentioned above: keep the existing behavior exactly as it is now, but make the constructor's getter accessible through a symbol-named property rather than a string property. Apply this to all built-in prototypes.

So, I think this is basically not going to be viable - you will break too much code if you don't automatically rewrite x.constructor to x[Symbol.constructor] at parse time, but doing that rewrite will break a lot of code which is accessing a constructor property they created by hand (or from JSON.parse or whatever). So either way a lot of things will break. Probably too much for it to be considered good practice.

__proto__ doesn't have this problem because __proto__ is not a reasonable name for a user-defined property, so rewriting x.__proto__ to x[Symbol.proto] is viable, but constructor absolutely is a reasonable name for a user-defined property, so I would be surprised if you could get away with rewriting accesses of it.

prototype is exactly like constructor in the analysis above, except that it is marginally less likely to be used as a user-defined property unrelated to JS's built-in inheritance mechanism. But it would still be pretty risky, I think.

This inclines me more towards the option I suggested in #1, which I think is less likely to break code (and is also easier to learn). I realize it solves a slightly smaller class of bugs than the "cut all routes to the prototype" approach, but on the other hand if it can be deployed more widely I expect it would still end up eliminating more bugs total.

[salcho]

Thanks, that's a useful explanation. From the research I've done, user-defined constructor properties show up on a minority of codebases, but they do exist. For example, see this sample GitHub query and that is incompatible with automatic parse-time refactoring.

Nevertheless, it seems that the breakage risk is contained by both secure mode and automatic refactoring being opt-in, which would stop breakages while maximizing the number of code bases that can benefit from this. That is, if removing constructor or prototype is viable. @syg, because I know he has thought about this before.

In your opinion, would the main blocker be how complex removing constructor/prototype would be or the breakage potential, which we could regulate through the opt in mechanism?

[bakkot]

In your opinion, would the main blocker be how complex removing constructor/prototype would be or the breakage potential, which we could regulate through the opt in mechanism?

I'm worried about both, but more about the breakage potential. In particular, I don't think the opt-in mechanism really solves that problem, because a security mechanism is only useful if it's actually widely adopted, and we can't recommend everyone adopt a security mechanism that has a significant chance of breaking their code in a subtle way.