child_process.spawn not checking null byte in args

[t-tera]

Version

v18.9.1

Platform

Linux tsc-ubuntu2204 5.15.0-48-generic #54-Ubuntu SMP Fri Aug 26 13:26:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

child_process

What steps will reproduce the bug?

//// spawner.js
const {spawn} = require('child_process');

const myArgs = ['dump.js','AAA','BBB\0XXX', 'CCC'];
console.log('spawner.js:', myArgs);

const childProcess = spawn('node', myArgs);

childProcess.stdout.on('data', (chunk) => {
  console.log(chunk.toString());
});

//// dump.js
console.log('dump.js:', process.argv);

$ node spawner.js
spawner.js: [ 'dump.js', 'AAA', 'BBB\x00XXX', 'CCC' ]
dump.js: [ '/usr/local/bin/node', '/tmp/dump.js', 'AAA', 'BBB', 'CCC' ]

How often does it reproduce? Is there a required condition?

No particular condition required.

What is the expected behavior?

Node.js should raise error when invalid args (containing null byte) are given.

What do you see instead?

No error raised. Null byte and subsequent bytes are silently truncated.

Additional information

Other languages below have null byte checking and raise error in the same situation.

Java:   java.io.IOException: invalid null character in command
PHP:    Uncaught ValueError: Command array element 4 contains a null byte
Python: ValueError: embedded null byte
Ruby:   ArgumentError (string contains null byte)

IMO raising error is safer to avoid null byte injection.

[bnoordhuis]

I agree this ought to be fixed, for consistency with node's fs module (which rejects paths with nul bytes) if nothing else. Pull request welcome.

[RaisinTen]

Sent a PR which adds the required validation: #44782

[LocutusOfBorg]

Hello @RaisinTen , looks like your test is not working on Ubuntu 23.04...
nodejs 18.13.0
https://launchpadlibrarian.net/644796275/buildlog_ubuntu-lunar-amd64.nodejs_18.13.0+dfsg-1ubuntu1_BUILDING.txt.gz
can you please doublecheck?

not ok 266 parallel/test-child-process-reject-null-bytes
  ---
  duration_ms: 0.272
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:636
          throw err;
          ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
    + actual - expected
    
      Comparison {
        code: 'ERR_INVALID_ARG_VALUE',
    +   message: "The argument 'args[3]' must be a string without null bytes. Received 'BBB\\x00XXX'"
    -   message: /The argument 'args\[2\]' must be a string without null bytes/
      }
        at Object.<anonymous> (/<<PKGBUILDDIR>>/test/parallel/test-child-process-reject-null-bytes.js:88:1)
        at Module._compile (node:internal/modules/cjs/loader:1218:14)
        at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
        at Module.load (node:internal/modules/cjs/loader:1081:32)
        at Module._load (node:internal/modules/cjs/loader:922:12)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
        at node:internal/main/run_main_module:23:47 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: TypeError [ERR_INVALID_ARG_VALUE]: The argument 'args[3]' must be a string without null bytes. Received 'BBB\x00XXX'
          at new NodeError (node:internal/errors:400:5)
          at validateArgumentNullCheck (node:child_process:968:11)
          at validateArgumentsNullCheck (node:child_process:975:5)
          at normalizeSpawnArguments (node:child_process:562:3)
          at spawn (node:child_process:750:13)
          at fork (node:child_process:170:10)
          at throws.code (/<<PKGBUILDDIR>>/test/parallel/test-child-process-reject-null-bytes.js:88:14)
          at getActual (node:assert:757:5)
          at throws (node:assert:903:24)
          at Object.<anonymous> (/<<PKGBUILDDIR>>/test/parallel/test-child-process-reject-null-bytes.js:88:1) {
        code: 'ERR_INVALID_ARG_VALUE'
      },
      expected: {
        code: 'ERR_INVALID_ARG_VALUE',
        message: /The argument 'args\[2\]' must be a string without null bytes/
      },
      operator: 'throws'
    }
    
    Node.js v18.13.0
  ...
ok 267 parallel/test-child-process-promisified

[LocutusOfBorg]

Maybe it has something to deal with the fact that we add --openssl-shared-config to some test?
+export TEST_CI_ARGS = --node-args="--openssl-shared-config"

+--- nodejs-18.7.0+dfsg.orig/test/parallel/test-tls-enable-keylog-cli.js
++++ nodejs-18.7.0+dfsg/test/parallel/test-tls-enable-keylog-cli.js
+@@ -18,7 +18,7 @@ tmpdir.refresh();
+ const file = path.resolve(tmpdir.path, 'keylog.log');
+ 
+ const child = fork(__filename, ['test'], {
+-  execArgv: ['--tls-keylog=' + file]
++  execArgv: ['--tls-keylog=' + file, "--openssl-shared-config"]
+ });
+ 
+ child.on('close', common.mustCall((code, signal) => {
+--- nodejs-18.7.0+dfsg.orig/test/parallel/test-tls-enable-trace-cli.js
++++ nodejs-18.7.0+dfsg/test/parallel/test-tls-enable-trace-cli.js
+@@ -19,7 +19,7 @@ if (!binding('tls_wrap').HAVE_SSL_TRACE)
+ 
+ const child = fork(__filename, ['test'], {
+   silent: true,
+-  execArgv: ['--trace-tls']
++  execArgv: ['--trace-tls', '--openssl-shared-config']
+ });
+ 
+ let stdout = '';

[LocutusOfBorg]

changing argv[2] to argv[3] seems to work as workaround, but we should have a smarter way to check if TEST_CI_ARGS have a different value other than the default...

[LocutusOfBorg]

--- nodejs-18.13.0+dfsg.orig/test/parallel/test-child-process-reject-null-bytes.js
+++ nodejs-18.13.0+dfsg/test/parallel/test-child-process-reject-null-bytes.js
@@ -87,7 +87,7 @@

 throws(() => fork(__filename, ['AAA', 'BBB\0XXX', 'CCC']), {
   code: 'ERR_INVALID_ARG_VALUE',
-  message: /The argument 'args\[2\]' must be a string without null bytes/
+  message: /The argument 'args\[3\]' must be a string without null bytes/
 });

 throws(() => spawn(process.execPath, [__filename, 'AAA', 'BBB\0XXX', 'CCC']), {

This works as specific Ubuntu workaround.