Skip to content
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.

Sign up for GitHub

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

Jump to bottom

node: warn for Object.prototype.__* accessors common in security warnings #39824

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
+93 −0
Open

node: warn for Object.prototype.__* accessors common in security warnings #39824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
813dd33
node: warn accessors common in security warnings
bmeck Aug 20, 2021
c49435f
linter
bmeck Aug 20, 2021
fd4e664
Update lib/internal/bootstrap/pre_execution.js
bmeck Aug 21, 2021
bc2ef67
DEP
bmeck Aug 21, 2021
05ef3f2
DEP meta
bmeck Aug 21, 2021
9f266d6
Update doc/api/deprecations.md
bmeck Aug 21, 2021
301d5ea
fix nits
bmeck Aug 21, 2021
9198ad2
oops, define correct prop name
bmeck Aug 21, 2021
04fbd18
cleanup
bmeck Aug 21, 2021
083c842
Update test/parallel/test-object-proto-accessor-warning.js
bmeck Aug 21, 2021
308eb0e
linter
bmeck Aug 21, 2021
c55d04a
fix refactor regression
bmeck Aug 22, 2021
741f229
pendingDeprecation
bmeck Aug 22, 2021
4b73dde
test needs flag
bmeck Aug 22, 2021
bd37bbc
Update doc/api/deprecations.md
bmeck Mar 4, 2022
07faa4b
Apply suggestions from code review
bmeck Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2801,7 +2801,30 @@ non-number value for `hints` option, a non-nullish non-boolean value for `all`
option, or a non-nullish non-boolean value for `verbatim` option in
[`dns.lookup()`][] and [`dnsPromises.lookup()`][] is deprecated.

### DEP0154: `Object.prototype` Legacy Accessors
bmeck marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/38906
bmeck marked this conversation as resolved.
Show resolved Hide resolved
description: Documentation-only deprecation.
-->

Type: Runtime

Accessors on `Object.prototype` are subject to object traversal attacks and
cause concerns for security audits. A variety of these are considered deprecated
bmeck marked this conversation as resolved.
Show resolved Hide resolved
by the [Legacy Object.prototype Accessor Methods][] by the JS standard. Modern
bmeck marked this conversation as resolved.
Show resolved Hide resolved
replacements are `Object.defineProperty`, `Object.getPrototypeOf`, and
`Object.setPrototypeOf` and not subject to path traversal. This affects:

* `Object.prototype.__defineGetter__`
* `Object.prototype.__defineSetter__`
* `Object.prototype.__lookupGetter__`
* `Object.prototype.__lookupSetter__`
* `Object.prototype.__proto__`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that __proto__ is annoying, but it’s also widely used (which is why I had excluded it from #39576 as well). Runtime-deprecating that is a big breaking change.

Copy link
Member Author

@bmeck bmeck Aug 22, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__proto__ is the main problem for CVEs regarding prototype pollution, it is the most important to figure out how to address. I am not arguing popularity or utility, just that the API is problematic in practice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right – that’s what --disable-proto is for, no? Anyway, if we do this, it should be communicated very clearly to users.

(I also don’t think putting this behind --pending-deprecation is particularly useful, given that there already is a flag to opt-out of this behavior. If we do make the decision to runtime-deprecate fully eventually, then I guess that decision can also be made now.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--disable-proto is a bit different, it doesn't let you see that something is using __proto__ so you can fix it. It just removes it or makes it throw; also it is opt-in so the ecosystem noise is just permanent since it doesn't actually cause any sort of signal that security warning isn't a false positive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I would say that throwing exceptions definitely lets you do that :)

In any case, to be clear I’m not -1 on this per se, I just think that this is a big change and we should call it out very explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing alters behavior, so maybe --disable-proto could get a warning mode that lets programs run and you fix it when you see it rather than taking down a process potentially?

Seems fine to have whole blog posts before this and waiting for a major to me on this PR. Since this affects legacy codebases as well it will likely also take some effort to PR things.

We could also add a flag to re-add the accessors if we ever do remove them for people needing to run legacy code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing alters behavior, so maybe --disable-proto could get a warning mode that lets programs run and you fix it when you see it rather than taking down a process potentially?

I mean – yes, throwing alters behavior, but practically speaking, people will notice whether they are using __proto__ with either method, which is the point here anyw2ay.

We could also add a flag to re-add the accessors if we ever do remove them for people needing to run legacy code.

Yeah, I think in the long run that might be a good idea – just remove the accessors, but add a flag to add them for those who really need them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad that --disable-proto=log wasn't an option ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, --disable-proto=throw during development should spot all issues (if using a package lock).


[Legacy URL API]: url.md#url_legacy_url_api
[Legacy Object.prototype Accessor Methods]: https://tc39.es/ecma262/#sec-object.prototype-legacy-accessor-methods
[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
[RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
[WHATWG URL API]: url.md#url_the_whatwg_url_api
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/bootstrap/pre_execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const {
NumberParseInt,
ObjectDefineProperty,
ObjectGetOwnPropertyDescriptor,
ObjectGetPrototypeOf,
ObjectPrototype,
SafeMap,
SafeWeakMap,
StringPrototypeStartsWith,
Expand Down Expand Up @@ -337,7 +337,6 @@ function initializeDeprecations() {
});

let warnedForProto = false;
const ObjectPrototype = ObjectGetPrototypeOf({});
function warnOnProto() {
if (!warnedForProto) {
warnedForProto = true;
Expand Down