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.

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

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
23 changes: 23 additions & 0 deletions doc/api/deprecations.md
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.

### DEP0XXX: `Object.prototype` Legacy Accessors
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39824
description: Runtime deprecation.
-->

Type: Runtime

Accessors on `Object.prototype` are subject to object traversal attacks and
cause concerns for security audits. A variety of these are deprecated
by [Legacy Object.prototype Accessor Methods][]. Modern
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

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
55 changes: 55 additions & 0 deletions lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
const {
NumberParseInt,
ObjectDefineProperty,
ObjectGetOwnPropertyDescriptor,
ObjectPrototype,
SafeMap,
SafeWeakMap,
StringPrototypeStartsWith,
globalThis,
uncurryThis,
} = primordials;

const {
Expand Down Expand Up @@ -332,6 +335,58 @@ function initializeDeprecations() {
enumerable: false,
configurable: true
});

if (pendingDeprecation) {
const legacy = [
['__proto__', {
get: 'Object.getPrototypeOf',
set: 'Object.setPrototypeOf'
}],
['__defineGetter__', 'Object.defineProperty'],
['__defineSetter__', 'Object.defineProperty'],
['__lookupGetter__', 'Object.getOwnPropertyDescriptor'],
['__lookupSetter__', 'Object.getOwnPropertyDescriptor'],
];
for (let i = 0; i < legacy.length; i++) {
const prop = legacy[i][0];
const replacement = legacy[i][1];
const getReplacement = typeof replacement === 'string' ?
replacement : replacement.get;
const setReplacement = typeof replacement === 'string' ?
replacement : replacement.set;
const protoDescriptor = ObjectGetOwnPropertyDescriptor(
ObjectPrototype,
prop);
if (protoDescriptor && 'value' in protoDescriptor) {
let value = protoDescriptor.value;
const writable = protoDescriptor.writable;
ObjectDefineProperty(ObjectPrototype, prop, {
get: deprecate(function() {
return value;
}, `${prop} is deprecated, use ${getReplacement} instead`, 'DEP0XXX'),
set: deprecate(function(v) {
if (!writable) return;
value = v;
}, `${prop} is deprecated, use ${setReplacement} instead`, 'DEP0XXX'),
enumerable: false,
configurable: true
});
} else {
const getter = uncurryThis(protoDescriptor.get);
const setter = uncurryThis(protoDescriptor.set);
ObjectDefineProperty(ObjectPrototype, prop, {
get: deprecate(function() {
return getter(this);
}, `${prop} is deprecated, use ${getReplacement} instead`, 'DEP0XXX'),
set: deprecate(function(value) {
setter(this, value);
}, `${prop} is deprecated, use ${setReplacement} instead`, 'DEP0XXX'),
enumerable: false,
configurable: true
});
}
}
}
}

function setupChildProcessIpcChannel() {
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-object-proto-accessor-warning.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Flags: --pending-deprecation
'use strict';

const common = require('../common');

common.expectWarning(
'DeprecationWarning',
'__proto__ is deprecated, use Object.getPrototypeOf instead', 'DEP0XXX');

const obj = {};
// eslint-disable-next-line
const _ = obj.__proto__;
// eslint-disable-next-line
obj.__proto__ = null;
1 change: 1 addition & 0 deletions test/parallel/test-repl-tab-complete.js
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ testMe.complete('obj.', common.mustCall(function(error, data) {

data[0].forEach((key) => {
if (!key || key === 'ele.biu') return;
console.error(key, ele[key.substr(4)]);
assert.notStrictEqual(ele[key.substr(4)], undefined);
});
}));
Expand Down