New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
security: disallow __proto__ / freeze builtins #4324
Comments
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. |
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('...') }
}) |
Yes, likely just need to do something like that when we freeze the Deno namespace. I will raise a PR later today. |
Actually this reminds me of another more obvious instance of the problem: currently we have been using builtins like Probably I should create a new issue for this if this has not been discussed before. |
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? |
Node.js has a Frozen Intrinsics flag that will freeze all mutation of all intrinsics including setting The approach could even be extended to disallow setting |
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.
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! |
I just meant that people aren't taking it seriously enough, IMO. And that hopefully, by contrast, the Deno community will.
You are right, my mistake. Would frozen intrinsics include things like Playing around in my terminal, looks like Node's |
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. |
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 |
Ideally it would be a compile-time error (at least in ts)... |
@hayd it already is... |
I've raised a PR. Throwing unexpectedly is exactly what we want to avoid. While there are other potential exploits of There should be no legitimate code out there that people would run in Deno that intentionally accesses I someone in the future opens a ticket because they try to access |
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 |
Just ran into an issue using the Microsoft Cognitive Services SDK where their usage of This is where they use proto: https://github.com/microsoft/cognitive-services-speech-sdk-js/blob/b062648064d9b353b08ec67af0e41f48a9bea4da/src/common/RawWebsocketMessage.ts#L20 Ended up solving it with this small snippet. Obviously not an ideal solution, but leaving it here for anyone else who stumbles into this discussion and needs a quick patch: Object.defineProperty(Object.prototype, '__proto__', {
get: function() {
return Object.getPrototypeOf(this);
}
}) |
@iddogino FYI you can use |
A recent blog post discusses the evils of property access using
foo[bar]
notation, wherebar
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.The text was updated successfully, but these errors were encountered: