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

bmeck · 2021-08-20T20:36:05Z

These accessors cause too much noise in npm security audits for the ecosystem, we should start a path to removal. Figuring out/warning on usage seems a good first step prior to actual removal. This would also be a way to verify that a security audit warning actually is affecting something rather than being false positives. These are optional/legacy in the JS language specification https://tc39.es/ecma262/#sec-object.prototype-legacy-accessor-methods

lib/internal/bootstrap/pre_execution.js

jasnell · 2021-08-20T22:22:17Z

lib/internal/bootstrap/pre_execution.js

+      warnedForProto = true;
+      process.emitWarning('usage of Object.prototype.__* properties are a' +
+        ' common security concern, use static methods on Object instead');
+    }


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

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

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

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

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

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)

mcollina

We need to document this somewhere. Most experienced folks would now but not newbies.

lib/internal/bootstrap/pre_execution.js

mcollina · 2021-08-21T10:04:38Z

Here is a relevant discussion: #31951

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

lib/internal/bootstrap/pre_execution.js

doc/api/deprecations.md

jasnell · 2021-08-21T13:09:40Z

test/parallel/test-object-proto-accessor-warning.js

+
+const common = require('../common');
+
+process.on('warning', common.mustCall());


There is a common.expectWarning() utility.

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.

doc/api/deprecations.md

lib/internal/bootstrap/pre_execution.js

Co-authored-by: James M Snell <jasnell@gmail.com>

test/parallel/test-object-proto-accessor-warning.js

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

targos · 2021-08-22T08:35:38Z

CITGM results: https://ci.nodejs.org/job/citgm-smoker/nodes=ubuntu1804-64/2751/#showFailuresLink

Unfortunately, all modules that depend on node-gyp fail at install time because of another deprecation.

Affects ava, csv-parser, ember-cli through an old version of graceful-fs
Affects bluebird through the cli-table module: https://github.com/Automattic/cli-table/blob/89392b27224b6a0c4a71f3085d1bdb5e9b83df9d/lib/index.js#L62, https://github.com/Automattic/cli-table/blob/89392b27224b6a0c4a71f3085d1bdb5e9b83df9d/lib/index.js#L71
Affects body-parser through an old version of chalk
Affects clinic through tap-mocha-reporter: https://github.com/tapjs/tap-mocha-reporter/blob/5c8846fe3655aceb7ab104e5989b469b57601c00/lib/reporters/dot.js#L62
Seems to affect Prettier's TypeScript parser, but I can't find where it is in Prettier's repository: https://ci.nodejs.org/job/citgm-smoker/nodes=ubuntu1804-64/2751/testReport/junit/(root)/citgm/cheerio_v1_0_0_rc_10/
Affects crc32-stream through some version of yargs-parser
Affects commander through yargs: https://github.com/yargs/yargs/blob/395bb67749787d269cabe80ffc3133c2f6958aeb/index.cjs#L25-L43
Affects duplexer2, esprima through some version of commander
Affects express through cookie-session. The module is fixed but only in an alpha release.

I'm stopping here but there are many more.

targos

cli-table is used by 375k GitHub repositories and >2.5k npm packages
chalk versions < 4 are probably used by a lot of projects
yargs is used by millions of projects
express is used by millions of projects

Maybe we should start with a pending deprecation?

addaleax · 2021-08-22T12:11:50Z

doc/api/deprecations.md

+* `Object.prototype.__defineSetter__`
+* `Object.prototype.__lookupGetter__`
+* `Object.prototype.__lookupSetter__`
+* `Object.prototype.__proto__`


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.

__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.

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.)

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

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.

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.

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.

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

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

bmeck · 2021-08-22T12:24:37Z

@targos added a guard

bmeck · 2021-08-22T13:11:17Z

@targos after some digging, a variety of those are all from the test reporter used: tapjs/tap-mocha-reporter#68

bmeck · 2021-10-28T15:16:43Z

we have patched some stuff in the wild, we should run CITGM again

doc/api/deprecations.md

bmeck · 2022-03-04T21:44:00Z

https://github.com/tapjs/tap-mocha-reporter has landed a fix, we should be good to try a new CITGM to see what the status is in the ecosystem.

Co-authored-by: Rich Trott <rtrott@gmail.com>

bmeck · 2022-03-04T21:53:10Z

CITGM smoker running https://ci.nodejs.org/job/citgm-smoker/2863/

aduh95 · 2022-04-06T14:42:45Z

The last CITGM run reports 81 failures, which seems to be more or less the same as master. Maybe we could land this in v18.0.0?

@bmeck can you rebase please?

bmeck · 2022-04-06T14:50:19Z

Have a newborn, won't be doing PR work for a few weeks

…

On Wed, Apr 6, 2022, 9:43 AM Antoine du Hamel ***@***.***> wrote: The last CITGM run reports 81 failures, which seems to be more or less the same as master. Maybe we could land this in v18.0.0? @bmeck <https://github.com/bmeck> can you rebase please? — Reply to this email directly, view it on GitHub <#39824 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI6MSA43X277Y32G53DVDWPHDANCNFSM5CQ7UVQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mcollina · 2022-09-09T21:22:40Z

Congrats!

kapouer · 2022-09-09T21:24:36Z

Was passing by here too. Nice one. Babies are so cute, and they don't have proto !

node: warn accessors common in security warnings

813dd33

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 20, 2021

linter

c49435f

VoltrexKeyva reviewed Aug 20, 2021

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

jasnell reviewed Aug 20, 2021

View reviewed changes

mcollina reviewed Aug 21, 2021

View reviewed changes

targos reviewed Aug 21, 2021

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

Update lib/internal/bootstrap/pre_execution.js

fd4e664

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

targos reviewed Aug 21, 2021

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

bmeck added 2 commits August 21, 2021 08:00

DEP

bc2ef67

DEP meta

05ef3f2

jasnell reviewed Aug 21, 2021

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

jasnell reviewed Aug 21, 2021

View reviewed changes

targos reviewed Aug 21, 2021

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

devsnek reviewed Aug 21, 2021

View reviewed changes

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved

bmeck and others added 4 commits August 21, 2021 09:34

Update doc/api/deprecations.md

9f266d6

Co-authored-by: James M Snell <jasnell@gmail.com>

fix nits

301d5ea

oops, define correct prop name

9198ad2

cleanup

04fbd18

VoltrexKeyva reviewed Aug 21, 2021

View reviewed changes

test/parallel/test-object-proto-accessor-warning.js Outdated Show resolved Hide resolved

bmeck and others added 3 commits August 21, 2021 11:12

Update test/parallel/test-object-proto-accessor-warning.js

083c842

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

linter

308eb0e

fix refactor regression

c55d04a

targos requested changes Aug 22, 2021

View reviewed changes

addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Aug 22, 2021

addaleax requested changes Aug 22, 2021

View reviewed changes

pendingDeprecation

741f229

test needs flag

4b73dde

Trott reviewed Oct 28, 2021

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

Trott reviewed Oct 28, 2021

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

bmeck and others added 2 commits March 4, 2022 15:44

Update doc/api/deprecations.md

bd37bbc

Co-authored-by: Rich Trott <rtrott@gmail.com>

Apply suggestions from code review

07faa4b

Co-authored-by: Rich Trott <rtrott@gmail.com>

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

bmeck mentioned this pull request Feb 7, 2023

Alternative "Delete the __proto__ and * keys" non-viable tc39/proposal-symbol-proto#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bmeck commented Aug 20, 2021

jasnell Aug 20, 2021

bmeck Aug 21, 2021

bmeck Aug 21, 2021

jasnell Aug 21, 2021

targos Aug 21, 2021

bmeck Aug 21, 2021

mcollina left a comment •

edited

Loading

mcollina commented Aug 21, 2021

jasnell Aug 21, 2021

bmeck Aug 21, 2021

targos commented Aug 22, 2021

targos left a comment

addaleax Aug 22, 2021

bmeck Aug 22, 2021 •

edited

Loading

addaleax Aug 22, 2021

bmeck Aug 22, 2021

addaleax Aug 22, 2021

bmeck Aug 22, 2021

addaleax Aug 22, 2021

kapouer Sep 9, 2022

kapouer Sep 9, 2022

bmeck commented Aug 22, 2021

bmeck commented Aug 22, 2021

bmeck commented Oct 28, 2021

bmeck commented Mar 4, 2022

bmeck commented Mar 4, 2022

aduh95 commented Apr 6, 2022

bmeck commented Apr 6, 2022 via email

mcollina commented Sep 9, 2022

kapouer commented Sep 9, 2022

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


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

		process.on('warning', common.mustCall());

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

Are you sure you want to change the base?

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

Conversation

bmeck commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment • edited Loading

Choose a reason for hiding this comment

mcollina commented Aug 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Aug 22, 2021

targos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck Aug 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Aug 22, 2021

bmeck commented Aug 22, 2021

bmeck commented Oct 28, 2021

bmeck commented Mar 4, 2022

bmeck commented Mar 4, 2022

aduh95 commented Apr 6, 2022

bmeck commented Apr 6, 2022 via email

mcollina commented Sep 9, 2022

kapouer commented Sep 9, 2022

mcollina left a comment •

edited

Loading

bmeck Aug 22, 2021 •

edited

Loading