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.

### 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

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
57 changes: 57 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,60 @@ function initializeDeprecations() {
enumerable: false,
configurable: true
});

let warnedForProto = false;
function warnOnProto() {
if (!warnedForProto) {
warnedForProto = true;
process.emitWarning('usage of Object.prototype.__* properties are a' +
bmeck marked this conversation as resolved.
Show resolved Hide resolved
' common security concern, use static methods on Object instead');
}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should make these proper runtime deprecations?
/cc @addaleax

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not opposed, is it just adding to the docs a new number to get the DEP###?

Copy link
Member Author

Choose a reason for hiding this comment

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

added to DEP doc, idk if there is a process to do instead

Copy link
Member

Choose a reason for hiding this comment

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

Look for other DEP codes in the code and you'll see how those are emitted :-)

Copy link
Member

Choose a reason for hiding this comment

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

Example:

node/lib/sys.js

Lines 28 to 29 in e46c680

process.emitWarning('sys is deprecated. Use util instead.',
'DeprecationWarning', 'DEP0025');

As soon as this is changed to emit a deprecation warning, I'll launch a CITGM run with --throw-deprecation

Copy link
Member Author

Choose a reason for hiding this comment

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

this seems difficult to wrangle without giving a unique DEP id to each accessor given the other feedback of giving more actionable feedback in #39824 (comment)

}
const legacy = [
'__proto__',
bmeck marked this conversation as resolved.
Show resolved Hide resolved
'__defineGetter__',
'__defineSetter__',
'__lookupGetter__',
'__lookupSetter__',
];
for (let i = 0; i < legacy.length; i++) {
const prop = legacy[i];
const protoDescriptor = ObjectGetOwnPropertyDescriptor(
ObjectPrototype,
prop);
if (protoDescriptor && 'value' in protoDescriptor) {
let value = protoDescriptor.value;
const writable = protoDescriptor.writable;
ObjectDefineProperty(ObjectPrototype, legacy[i], {
get() {
warnOnProto();
return value;
},
set(v) {
warnOnProto();
if (!writable) return;
value = v;
},
enumerable: false,
configurable: true
});
} else {
const getter = uncurryThis(protoDescriptor.get);
const setter = uncurryThis(protoDescriptor.set);
ObjectDefineProperty(ObjectPrototype, legacy[i], {
get() {
warnOnProto();
return getter(this);
},
set(value) {
warnOnProto();
setter(this, value);
},
enumerable: false,
configurable: true
});
}
}
}

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

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

process.on('warning', common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

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

There is a common.expectWarning() utility.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, but it seems deprecate(...) in lib seems to only fire once per dep code so having specialized messages per accessor seems to require unique DEP codes.


const obj = {};
// eslint-disable-next-line
const _ = obj.__proto__;
// eslint-disable-next-line
obj.__proto__ = null;