security: disallow __proto__ / freeze builtins

[kitsonk]

A recent blog post discusses the evils of property access using foo[bar] notation, where bar comes from somewhere else opens up an attack vector to compromise code.

The root of the "evil" though is access __proto__. A co-worker (@camjackson) pointed out to me that Node.js issue discussing it (nodejs/node#31951) and it is of course a lot harder for them and they are considering a flag.

Cam asked me what Deno's stance was. I indicated we hadn't specifically talked about it, but with our security first footing, it seems like something important we should consider. I think we are at the stage where we could just get rid of it. __proto__ is Annex B anyways, and we technically don't have to implement any Annex B to still be compliant with ECMAScript.

Another interesting point, which maybe better overall, is that Node.js supports --frozen-intrinsics and I am almost wondering if we would do that by default, so that built-ins are frozen. I personally don't see a need to even flag it, because while the augmentation of builtins might have been popular in the day, it really runs afoul of good practice. I think it is a bit radical and there really isn't basis in the standard to do it (though I think you would be hard pressed to say that mutability of builtins is specified.

[ry]

Thanks for bringing this up - I didn't know about any of this.

How would we disable proto ?

Frozen intrinsics by default sounds good to me as long as it doesn't effect our benchmarks.

[LinusU]

How would we disable proto ?

From what I understand we just need to add the following line to run as soon as v8 starts:

delete Object.prototype.__proto__

edit: potentially we could also throw an error to be even more explicit:

Object.defineProperty(Object.prototype, '__proto__', {
  configurable: true,
  enumerable: false,
  get () { throw new Error('...') },
  set (_) { throw new Error('...') }
})

[kitsonk]

Yes, likely just need to do something like that when we freeze the Deno namespace. I will raise a PR later today.

[kevinkassimo]

Actually this reminds me of another more obvious instance of the problem: currently we have been using builtins like Object String directly. This is also an attack vector under runtime replacement: if you try to do window.Object = 1; in the REPL, you'll immediately crash with the error Object.is is not a function. While this seems to be just annoying, it is more dangerous in other places (e.g. replacing String.fromCharCode completely ruins security for our TextDecoder).

Probably I should create a new issue for this if this has not been discussed before.

[sholladay]

The amount of push back on that Node.js issue is disheartening. From my research, a lot of real world services are affected by this. Practically any service built with Node that attempts to parse multipart payloads can be easily poisoned or crashed by sending it a very simple malicious payload - no XSS or CSRF necessary. Yet another reason to embrace functional programming...

I'd say disable the attack vector even if it affects the benchmarks. What good is fast software if it's not secure?

[guybedford]

Node.js has a Frozen Intrinsics flag that will freeze all mutation of all intrinsics including setting Object.__proto__ etc. The implementation is here - https://github.com/nodejs/node/blob/master/lib/internal/freeze_intrinsics.js.

The approach could even be extended to disallow setting __proto__ on any object since getters and setters on prototypes are called even with subclasses.

[kitsonk]

The amount of push back on that Node.js issue is disheartening.

Let's save Node.js discussion for Node.js forums. There are a lot of other considerations they have that we don't at the moment.

I'd say disable the attack vector even if it affects the benchmarks.

Ry was suggesting disabling the attack vector no matter what. It was freezing builtins that was debatable, which I agree with. Not freezing intrinsics is more like a "good idea" than closing an attack vector. People can do all sorts of stupid things in a sandbox, to what degree we let them is always debatable.

As always @guybedford thanks for the info!

[sholladay]

Let's save Node.js discussion for Node.js forums.

I just meant that people aren't taking it seriously enough, IMO. And that hopefully, by contrast, the Deno community will.

Ry was suggesting disabling the attack vector no matter what. It was freezing builtins that was debatable, which I agree with.

You are right, my mistake. Would frozen intrinsics include things like console? I ask because overriding console methods is a hacky but useful trick when implementing a logging framework. That's just one example off the top of my head where it would probably be annoying.

Playing around in my terminal, looks like Node's --frozen-intrinsics does include console.

[kitsonk]

I just meant that people aren't taking it seriously enough, IMO.

There is enough argo in communities at large, let alone between communities. Node.js has different considerations than Deno. If you feel they aren't taking it seriously enough, that is discussion for that community. We need less stone throwing between respective glass houses. There is no benefit in that.

[sholladay]

No stone throwing here... We do, though, need to lay out why doing this matters, for the uninitiated, and why ignoring the problem is not acceptable. Security vulnerabilities happen - they'll happen to Deno, too - and that's understandable. The response matters a lot and I hope Deno's will be appropriately strong.

Specifically, I think the suggestion to throw an error for __proto__ usage is a good idea worth considering. It would help debug legacy code that won't work as expected. On the other hand, it's debatable how much legacy code will run through Deno. Also, naive code might crash unexpectedly. However, frameworks that parse multipart payloads, which seem to be the common attack surface in the wild, should be using try / catch already to handle parsing errors, anyway. So hopefully throwing won't be too inconvenient.

[hayd]

Ideally it would be a compile-time error (at least in ts)...

[kitsonk]

@hayd it already is...

[kitsonk]

I've raised a PR. Throwing unexpectedly is exactly what we want to avoid. While there are other potential exploits of __proto__ the most common is a denial of service, where you make a request to a server with something like { "__proto__": null } and suddenly the service crashes.

There should be no legitimate code out there that people would run in Deno that intentionally accesses Object.prototype.__proto__. The code in the PR gives both the before and after output, and I think everyone can agree that the behaviour after the patch is totally acceptable. It actually behaves like someone might expect. Throwing on accessing the property would effectively put us in the same situation, just earlier in the process, doesn't mean people will properly guard their code to handle these unexpected throws.

I someone in the future opens a ticket because they try to access Object.prototype.__proto__ and it doesn't work the way they expect it, I would be glad to have a conversation with them about their use case! 😆

[kitsonk]

I also think there is still merit in freezing builtins by default, but we should treat that as a seperate issue and debate more what builtins we would freeze, because as pointed out, modifying instances like console are useful, while we generally wouldn't want people to modify Object or Array for example, but what about Error, sometimes people augment that as part of an error framework, and I wouldn't "blame" them for that.