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

child_process: harden against prototype pollution #48726

Merged
merged 1 commit into from Jul 14, 2023
Merged

child_process: harden against prototype pollution #48726

merged 1 commit into from Jul 14, 2023

Conversation

LiviaMedeiros
Copy link
Contributor

Improve robustness against something like

Object.assign(Object.prototype, {
  cwd: '/etc',
  uid: 82, // www-data
  execFile: '/bin/rm', // using rm to "spawn" modules
  execArgv: [ ... ],
  shell: '...',
  env: { ... },
  ...
});

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Jul 10, 2023
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52685/

@KhafraDev KhafraDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 10, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52697/

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52717/

aduh95
aduh95 approved these changes Jul 12, 2023
View reviewed changes
lib/child_process.js
@@ -136,7 +136,7 @@ function fork(modulePath, args = [], options) {
if (options != null) {
validateObject(options, 'options');
}
options = { ...options, shell: false };
options = { __proto__: null, ...options, shell: false };
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but I wonder if we should use { __proto__: options, shell: false } and let users decide if they want prototype pollution or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it make this object affected by subsequent mutations outside?
In fact, I wonder if we should use structuredClone() or at least make copies of object properties like .env to avoid race conditions if multiple fork()s are called with reused options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make this object affected by subsequent mutations outside?

I guess it depends if we read the properties in the same tick or not (which I expect we do most of the time, but I haven't checked).

In fact, I wonder if we should use structuredClone()

That wouldn't help with inconsistency of our API: sometimes, we dismiss inherited properties, sometimes we don't. But it's clearly not a big deal as I don't think we ever received any bug report regarding that.

or at least make copies of object properties like .env to avoid race conditions if multiple fork()s are called with reused options.

but wouldn't folks expect the same env to be shared if they supply the same object? The fact that no one complains makes me think we're overthinking it 😅

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 14, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6a88926 into nodejs:main Jul 14, 2023
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6a88926

This was referenced Jul 16, 2023
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@LiviaMedeiros @Ceres6
child_process: harden against prototype pollution
788bdf9 
PR-URL: nodejs#48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@LiviaMedeiros @Ceres6
child_process: harden against prototype pollution
e41a9c5 
PR-URL: nodejs#48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@LiviaMedeiros @UlisesGascon
child_process: harden against prototype pollution
9df65d8 
PR-URL: nodejs#48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@LiviaMedeiros @RafaelGSS
child_process: harden against prototype pollution
1f35c0c 
PR-URL: #48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 12, 2023
@LiviaMedeiros @ruyadorno
child_process: harden against prototype pollution
996a371 
PR-URL: #48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@LiviaMedeiros @ruyadorno
child_process: harden against prototype pollution
ee024cf 
PR-URL: #48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 13, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@LiviaMedeiros @ruyadorno
child_process: harden against prototype pollution
cc33a18 
PR-URL: #48726
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Sep 19, 2023
@codebytere
child_process: harden against prototype pollution
63e78c0 
nodejs/node#48726
jkleinsc pushed a commit to electron/electron that referenced this pull request Sep 20, 2023
@electron-roller @codebytere
chore: bump node to v18.18.0 (main) (#39915)
1766511 
* chore: bump node in DEPS to v18.18.0

* child_process: harden against prototype pollution

nodejs/node#48726

* deps: upgrade to libuv 1.46.0

nodejs/node#49591

* module: reduce url invocations in esm/load.js

nodejs/node#48337

* Revert "test: remove test-crypto-keygen flaky designation"

nodejs/node#48652

* fix: FTBTFS in ada dep

ada-url/ada#464
ada-url/idna#31

* fix: force_colors snapshot line number

* chore: fixup patch indices

* chore: update filenames.json

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@electron-roller @codebytere
chore: bump node to v18.18.0 (main) (electron#39915)
a848ba1 
* chore: bump node in DEPS to v18.18.0

* child_process: harden against prototype pollution

nodejs/node#48726

* deps: upgrade to libuv 1.46.0

nodejs/node#49591

* module: reduce url invocations in esm/load.js

nodejs/node#48337

* Revert "test: remove test-crypto-keygen flaky designation"

nodejs/node#48652

* fix: FTBTFS in ada dep

ada-url/ada#464
ada-url/idna#31

* fix: force_colors snapshot line number

* chore: fixup patch indices

* chore: update filenames.json

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@KhafraDev KhafraDev KhafraDev approved these changes

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@LiviaMedeiros @nodejs-github-bot @aduh95 @KhafraDev @VoltrexKeyva @lpinca