Skip to content

child_process.spawn not checking null byte in args #44768

Closed
#44782
Closed
child_process.spawn not checking null byte in args#44768
#44782
Labels
child_processIssues and PRs related to the child_process subsystem.

Description

@t-tera
t-tera
opened on Sep 24, 2022

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.

Activity

  1. VoltrexKeyva
    added
    child_processIssues and PRs related to the child_process subsystem.
    on Sep 24, 2022

  2. bnoordhuis commented on Sep 25, 2022

    @bnoordhuis
    bnoordhuis
    on Sep 25, 2022
    Member

    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.

  3. RaisinTen commented on Sep 25, 2022

    @RaisinTen
    RaisinTen
    on Sep 25, 2022
    Member

    Sent a PR which adds the required validation: #44782

  4. RaisinTen
    added a commit that references this issue on Oct 3, 2022

    child_process: validate arguments for null bytes

    a6bb1c4
  5. nodejs-github-bot
    closed this as completedin #44782on Oct 14, 2022
  6. nodejs-github-bot
    added a commit that references this issue on Oct 14, 2022

    child_process: validate arguments for null bytes

    91dbd8b
  7. RafaelGSS
    added a commit that references this issue on Nov 1, 2022

    child_process: validate arguments for null bytes

    c2cad0c
  8. RafaelGSS
    added a commit that references this issue on Nov 10, 2022

    child_process: validate arguments for null bytes

    b2e6048

  9. LocutusOfBorg commented on Jan 9, 2023

    @LocutusOfBorg
    LocutusOfBorg
    on Jan 9, 2023

    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

  10. LocutusOfBorg commented on Jan 9, 2023

    @LocutusOfBorg
    LocutusOfBorg
    on Jan 9, 2023

    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 = '';

  11. LocutusOfBorg commented on Jan 10, 2023

    @LocutusOfBorg
    LocutusOfBorg
    on Jan 10, 2023

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

  12. LocutusOfBorg commented on Jan 10, 2023

    @LocutusOfBorg
    LocutusOfBorg
    on Jan 10, 2023
    --- 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    child_processIssues and PRs related to the child_process subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions