From 281d8968bb5168031f1d8f69154a35e2883004cd Mon Sep 17 00:00:00 2001 From: = Date: Mon, 24 Feb 2025 20:10:09 +0000 Subject: [PATCH 1/3] child_process: deprecate passing `args` to `spawn` and `execFile` Accepting `args` gives the false impression that the args are escaped while really they are just concatenated. This makes it easy to introduce bugs and security vulnerabilities. --- doc/api/child_process.md | 2 +- doc/api/deprecations.md | 5 ++++- lib/child_process.js | 7 +++++++ test/parallel/test-child-process-execfile.js | 11 +++++++---- test/parallel/test-child-process-spawn-shell.js | 6 ++++++ test/parallel/test-child-process-spawnsync-shell.js | 6 ++++++ 6 files changed, 31 insertions(+), 6 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 0a9c0e3e326cc3..b908419076f148 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -158,7 +158,7 @@ exec('my.bat', (err, stdout, stderr) => { }); // Script with spaces in the filename: -const bat = spawn('"my script.cmd"', ['a', 'b'], { shell: true }); +const bat = spawn('"my script.cmd" a b', { shell: true }); // or: exec('"my script.cmd" a b', (err, stdout, stderr) => { // ... diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 392ef09c2ca3e2..47b5d4c552f16b 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3859,13 +3859,16 @@ deprecated, as their values are guaranteed to be identical to that of `process.f -Type: Documentation-only +Type: Runtime When an `args` array is passed to [`child_process.execFile`][] or [`child_process.spawn`][] with the option `{ shell: true }`, the values are not escaped, only space-separated, which can lead to shell injection. diff --git a/lib/child_process.js b/lib/child_process.js index 3fb21f755be3d7..4992c93c2353d0 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -611,6 +611,13 @@ function normalizeSpawnArguments(file, args, options) { if (options.shell) { validateArgumentNullCheck(options.shell, 'options.shell'); + if (args.length > 0) { + process.emitWarning( + 'Passing args to a child process with shell option true can lead to security ' + + 'vulnerabilities, as the arguments are not escaped, only concatenated.', + 'DeprecationWarning', + 'DEP0190'); + } const command = ArrayPrototypeJoin([file, ...args], ' '); // Set the shell, switches, and commands. if (process.platform === 'win32') { diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index c4dba6b3f9466f..69422d1ac345f0 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -47,8 +47,7 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p { // Verify the shell option works properly execFile( - `"${common.isWindows ? execOpts.env.NODE : '$NODE'}"`, - [`"${common.isWindows ? execOpts.env.FIXTURE : '$FIXTURE'}"`, 0], + common.isWindows ? `"${execOpts.env.NODE}" "${execOpts.env.FIXTURE} 0` : `"$NODE" "$FIXTURE" 0`, execOpts, common.mustSucceed(), ); @@ -117,10 +116,14 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p ...(common.isWindows ? [] : [{ encoding: 'utf8' }]), { shell: true, encoding: 'utf8' }, ].forEach((options) => { - const execFileSyncStdout = execFileSync(file, args, options); + const command = options.shell ? + [[file, ...args].join(' ')] : + [file, args]; + + const execFileSyncStdout = execFileSync(...command, options); assert.strictEqual(execFileSyncStdout, `foo bar${os.EOL}`); - execFile(file, args, options, common.mustCall((_, stdout) => { + execFile(...command, options, common.mustCall((_, stdout) => { assert.strictEqual(stdout, execFileSyncStdout); })); }); diff --git a/test/parallel/test-child-process-spawn-shell.js b/test/parallel/test-child-process-spawn-shell.js index a63e92f29d0adb..0fd530988c7b9f 100644 --- a/test/parallel/test-child-process-spawn-shell.js +++ b/test/parallel/test-child-process-spawn-shell.js @@ -18,6 +18,12 @@ doesNotExist.on('exit', common.mustCall((code, signal) => { })); // Verify that passing arguments works +common.expectWarning( + 'DeprecationWarning', + 'Passing args to a child process with shell option true can lead to security ' + + 'vulnerabilities, as the arguments are not escaped, only concatenated.', + 'DEP0190'); + const echo = cp.spawn('echo', ['foo'], { encoding: 'utf8', shell: true diff --git a/test/parallel/test-child-process-spawnsync-shell.js b/test/parallel/test-child-process-spawnsync-shell.js index 9db8a53366fc47..8c6f180accc2f3 100644 --- a/test/parallel/test-child-process-spawnsync-shell.js +++ b/test/parallel/test-child-process-spawnsync-shell.js @@ -19,6 +19,12 @@ else assert.strictEqual(doesNotExist.status, 127); // Exit code of /bin/sh // Verify that passing arguments works +common.expectWarning( + 'DeprecationWarning', + 'Passing args to a child process with shell option true can lead to security ' + + 'vulnerabilities, as the arguments are not escaped, only concatenated.', + 'DEP0190'); + internalCp.spawnSync = common.mustCall(function(opts) { assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''), 'echo foo'); From 7a04435f174985bd9b3e7b237923abd3a2dba8e3 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 19 Mar 2025 11:23:21 +0100 Subject: [PATCH 2/3] fixup! child_process: deprecate passing `args` to `spawn` and `execFile` --- lib/child_process.js | 4 +++- test/parallel/test-child-process-execfile.js | 17 ++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 4992c93c2353d0..2335a7f4222379 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -535,6 +535,7 @@ function copyProcessEnvToEnv(env, name, optionEnv) { } } +let emittedDEP0190Already = false; function normalizeSpawnArguments(file, args, options) { validateString(file, 'file'); validateArgumentNullCheck(file, 'file'); @@ -611,12 +612,13 @@ function normalizeSpawnArguments(file, args, options) { if (options.shell) { validateArgumentNullCheck(options.shell, 'options.shell'); - if (args.length > 0) { + if (args.length > 0 && !emittedDEP0190Already) { process.emitWarning( 'Passing args to a child process with shell option true can lead to security ' + 'vulnerabilities, as the arguments are not escaped, only concatenated.', 'DeprecationWarning', 'DEP0190'); + emittedDEP0190Already = true; } const command = ArrayPrototypeJoin([file, ...args], ' '); // Set the shell, switches, and commands. diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 69422d1ac345f0..80d94cc5626f2f 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -12,6 +12,12 @@ const fixture = fixtures.path('exit.js'); const echoFixture = fixtures.path('echo.js'); const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: process.execPath, FIXTURE: fixture } }; +common.expectWarning( + 'DeprecationWarning', + 'Passing args to a child process with shell option true can lead to security ' + + 'vulnerabilities, as the arguments are not escaped, only concatenated.', + 'DEP0190'); + { execFile( process.execPath, @@ -47,7 +53,8 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p { // Verify the shell option works properly execFile( - common.isWindows ? `"${execOpts.env.NODE}" "${execOpts.env.FIXTURE} 0` : `"$NODE" "$FIXTURE" 0`, + `"${common.isWindows ? execOpts.env.NODE : '$NODE'}"`, + [`"${common.isWindows ? execOpts.env.FIXTURE : '$FIXTURE'}"`, 0], execOpts, common.mustSucceed(), ); @@ -116,14 +123,10 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p ...(common.isWindows ? [] : [{ encoding: 'utf8' }]), { shell: true, encoding: 'utf8' }, ].forEach((options) => { - const command = options.shell ? - [[file, ...args].join(' ')] : - [file, args]; - - const execFileSyncStdout = execFileSync(...command, options); + const execFileSyncStdout = execFileSync(file, args, options); assert.strictEqual(execFileSyncStdout, `foo bar${os.EOL}`); - execFile(...command, options, common.mustCall((_, stdout) => { + execFile(file, args, options, common.mustCall((_, stdout) => { assert.strictEqual(stdout, execFileSyncStdout); })); }); From 180b40a8af211b1ce10d666993c08d8772be9416 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 19 Mar 2025 11:26:29 +0100 Subject: [PATCH 3/3] fixup! fixup! child_process: deprecate passing `args` to `spawn` and `execFile` --- lib/child_process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index 2335a7f4222379..5c853716a26a4e 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -618,7 +618,7 @@ function normalizeSpawnArguments(file, args, options) { 'vulnerabilities, as the arguments are not escaped, only concatenated.', 'DeprecationWarning', 'DEP0190'); - emittedDEP0190Already = true; + emittedDEP0190Already = true; } const command = ArrayPrototypeJoin([file, ...args], ' '); // Set the shell, switches, and commands.