vulnerability-npm

vulnerability.md

This document exposes a vulnerability when running npm scripts directly via npm run or indirectly via node_modules/.bin. To demonstrate the issue, please follow these steps:

• Run $ mkdir vulnerability && cd vulnerability && npm init --yes && npm i jest
• Edit package.json and set scripts.test to jest
• Create index.test.js with it('should work', () => {});

At this point, running $ npm test should run jest successfully. Now lets further experiment with it:

Note that ^ is an escaping char on Windows

• Running $ npm t should run jest successfully.
• Running $ npm t index will send index to jest, which will match 1 file and terminate successfully
• Running $ npm t ^"index^" is the same as above but the argument is just quoted for security
• Running $ npm t ^"\^"index\^"^" will send "index" to jest, which you will see it printed but matched 0 files as expected
• Running $ npm t ^"\^"^(index^)\^"^" will send "(index)" to jest, but it fails with \"" was unexpected at this time.
• But running jest directly with $ node node_modules\jest\bin\jest.js ^"\^"^(index^)\^"^" works as expected.

The issue lies in node_modules/.bin/jest.cmd:

@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\..\jest\bin\jest.js" %*
) ELSE (
  @SETLOCAL
  @SET PATHEXT=%PATHEXT:;.JS;=;%
  node  "%~dp0\..\jest\bin\jest.js" %*
)

The %* gets expanded before the execution of the batch script, which in makes it invalid due to how the parenthesis are interpreted. Please read this for details: https://support.microsoft.com/en-us/help/2524009/error-running-command-shell-scripts-that-include-parentheses

Updating the file to be:

@SETLOCAL
@SET PATHEXT=%PATHEXT:;.JS;=;%
node  "%~dp0\..\jest\bin\jest.js" %*

.. solves the issue but I guess the first @ IF is necessary for users with a local node.exe. We could try to rewrite the file to solve the problem but I'm not experienced with batch scripting. Additionally, I've tried to activate ENABLEDELAYEDEXPANSION but had no success.

With correct crafting of arguments, one could possibly execute arbitrary commands. For example, consider a API that has an endpoint that runs an executable via npm run or its .cmd file. Even when properly escaping the shell arguments, an individual that knows about this could craft a special argument that would allow running arbitrary commands on that machine.

At this time of writing I couldn't figure out a pattern to execute other commands, but as I said, I'm quite unexperienced with Windows batch scripting.

-- André Cruz

A couple observations:

Delayed expansion doesn't work for %* since there's no !* equivalent. (nor is there !1, !2, etc) You'd have to somehow copy %* into another variable, which you can't do without causing cmd.exe to parse escape characters. EDIT: wording, clarification

^ is only an escape character in cmd.exe. PowerShell, for example, is different. In your example, in order to escape arguments correctly, the caller must understand that:
a) They're invoking a .cmd file (as opposed to an .exe)
b) The .cmd file is proxying the command line to an .exe via %* (as opposed to parsing them itself)

If either of those changes, the user must escape arguments differently. EDIT: to clarify, the point I'm making is that .cmd shims are inherently problematic and leaky.

Rewriting the .cmd to avoid parentheses won't help, because you'll still have problems passing ((index)) to jest.

@cspotcode I've read this only now.

^ is only an escape character in cmd.exe. PowerShell, for example, is different. In your example, in order to escape arguments correctly, the caller must understand that:
a) They're invoking a .cmd file (as opposed to an .exe)
b) The .cmd file is proxying the command line to an .exe via %* (as opposed to parsing them itself)

The escaping with ^ is being done in cross-spawn which always calls cmd.exe, which is fine I think?

If either of those changes, the user must escape arguments differently. EDIT: to clarify, the point I'm making is that .cmd shims are inherently problematic and leaky.

Rewriting the .cmd to avoid parentheses won't help, because you'll still have problems passing ((index)) to jest.

Yep, I guess to completely get rid of the issue we must proxy everything to a caller.exe file, which would do what the .cmd does now. Correct?

I'm having good results with the following .cmd file:

@SETLOCAL

@IF EXIST "%~dp0\node.exe" (
  @SET "_node=%~dp0\node.exe"
) ELSE (
  @SET PATHEXT=%PATHEXT:;.JS;=;%
  @SET "_node=node"
)

"%node%" "%~dp0\..\jest\bin\jest.js" %*

This simply moves the %* expansion outside the if/else which resolves the issue.
What do you think?

For context, this issue was discovered when I was developing cross-spawn.

I've made a fix there by applying double-escaping when running a command pointing to node_modules/.bin, see https://github.com/moxystudio/node-cross-spawn/blob/9cb84dbd78e72be59fe6e56cac9a95e778206145/lib/parse.js#L46 and https://github.com/moxystudio/node-cross-spawn/blob/9cb84dbd78e72be59fe6e56cac9a95e778206145/test/index.test.js#L149.

Still the vulnerability exists but cross-spawn goes around it.