Do what we can to avoid command injection

[lydell]

If you have a source-directory named $(touch pwned) (like one typically does 😅 ), running elm-review --fix results in command injection – a file called pwned is created in this case.

Repro repo: https://github.com/lydell/elm-review-command-injection

This is because some spawn calls use shell: true. This means “I want my arguments to be interpreted as shell script”, but that’s not what we want in elm-review’s case – we want all arguments to be passed as-is to the program we’re spawning.

(Note: One reason to use shell: true with Node.js’ built-in spawn function is to be able to spawn more stuff on Windows, but we’re already using cross-spawn so that’s not something we need to think about: https://github.com/moxystudio/node-cross-spawn#using-optionsshell-as-an-alternative-to-cross-spawn)

As far as I can tell, shell: true has been there since the code was written. #43 noticed something was up with the arguments handling, but solved it with a non-sufficient escape technique.

I’ve tested this on my repro repo. That example also has a space in a folder name and things still work, so #43 should still be fixed.

[jfmengels]

Nice find!

The solution doesn't work however on Linux Ubuntu I think, as I can still reproduce even with this fix 🤔

I'm not familiar enough with what could be different from your system (Windows? MacOs?). Do you have any clue? 🤔

[lydell]

I wrote this on macOS, but I booted up my Ubuntu 20.04 machine now and it behaves exactly the same.

When I test this, I run rm -f Main.elm pwned && ~/forks/node-elm-review/bin/elm-review --fix in the repro repo.

[jfmengels]

That's weird, I feel like I'm doing the same thing. I also added console.log at some point to confirm I was running your version 🤔

[lydell]

Then I’d try to reproduce from the Node.js REPL instead to minimize the surface and get faster feedback. Something like this:

> require("cross-spawn").sync("echo", ["$(touch pwned)"], {encoding: "utf8"}).output
[ null, '$(touch pwned)\n', '' ]
> require("cross-spawn").sync("echo", ["$(touch pwned)"], {encoding: "utf8", shell: true}).output
[ null, '\n', '' ]
> fs.existsSync("pwned")
true

[lydell]

@jfmengels I made a new branch in the repro repo that installs my branch of node-elm-review and automatically verifies that it works: https://github.com/lydell/elm-review-command-injection/tree/fixed

1. Clone https://github.com/lydell/elm-review-command-injection
2. Switch to the fixed branch
3. npm ci (installs node-elm-review from my fork on github)
4. npm test (runs elm-review --fix for you and verifies the result)

If that prints “ALL OK!” it works, if it prints “PWNED!” it doesn’t (edit: it actually doesn’t print PWNED – see below).

[jfmengels]

It's very weird. When trying the REPL, I get the same results that you do. But when I try actually using elm-review --fix or the repro using your fixed branch, I still get "pwned". So I can confirm that this still doesn't work for me.

(Details: I'm never getting the echo "PWNED!" because elm-review keeps thinking there is a change to apply somewhere)

[lydell]

So, the command injection still happens if using npx@7 (I’ve been using npx@6). Even though we fixed command injection on our end by removing shell: true, npx@7 also has command injection! 🤦‍♂️

I made a PR for npm/npx: npm/run-script#31

But I’d like to fix node-elm-review by removing trying to run elm-format using npx. Why?

If you execute elm-review using npx (npx elm-review --fix), npx has already added the local ./node_modules/.bin to PATH, so spawn.sync("elm-format", args, options) should be fine. (I’ve written about this before here in Discord: https://discord.com/channels/534524278847045633/729776405524512789/792388838512525322)

If you execute elm-review using ./node_modules/.bin/elm-review --fix, the attempt to execute elm-format using npx doesn’t help anyway:

❯ ./node_modules/.bin/elm-review --fix
-- ELM NOT FOUND ---------------------------------------------------------------

I could not find the executable for the elm compiler.

A few options:
- Install it globally
- Specify the path using --compiler <path-to-elm>

…because elm-review doesn’t try to find elm using npx, so in this scenario a global elm is required, or using --compiler. elm-format should be handled the same (using npx just works, otherwise need global or --elm-format-path). And I think it makes sense to drop npx for elm-format rather than adding it for elm, since npx can’t safely be used by scripts with untrusted input anymore (well, unless my PR gets merged and people update their npx).

I’d also like to fix the command injection in elm-review new-package: If you answer for example $(touch pwned) as your GitHub user name, a file called pwned will be created.

But before I do anything, I’d like to double-check with @jfmengels:

• Do you agree on removing the npx approach for elm-format?
• Am I understanding lib/autofix.js right that we ignore all elm-format errors, except “binary not found”? If so, it looks like a mistake for the global approach – there any exit 1 seems to count as a real error?

  node-elm-review/lib/autofix.js

  Lines 172 to 182 in c0c74b1

  	if (spawnedUsingGlobal.status !== 0) {
  	throw new ErrorMessage.CustomError(
  	/* eslint-disable prettier/prettier */
  	'ERROR WHEN RUNNING ELM-FORMAT',
  	`I got an unexpected error when running ${chalk.magentaBright('elm-format')}:
  	${spawnedUsingGlobal.stderr.toString()}`,
  	options.elmJsonPath
  	/* eslint-enable prettier/prettier */
  	);
  	}

• elm-review new-package executes npx license, but license seems to be missing from package.json. Is that on purpose? I’d like to add it to package.json dependencies, instead of implicitly depending on npx downloading something when running. EDIT: That package is 8 MB! That’s a very valid reason for downloading it on demand… maybe we have to live with command injection here, or switch to another, smaller package

[jfmengels]

Sorry for the delay!

Do you agree on removing the npx approach for elm-review?

If everything is as you say (and it seems like it makes sense), then yes. I'm willing to try it out and see if people run into issues. If they do, we can backtrack to re-using npx.

Do you agree on removing the npx approach for elm-review?

Yes, it's a rarely used feature of the tool and a pretty big package, so I figured it would be okay to include it as needed 😅

Am I understanding lib/autofix.js right that we ignore all elm-format errors, except “binary not found”? If so, it looks like a mistake for the global approach – there any exit 1 seems to count as a real error?

I imagine that the most likely case for elm-format exiting with 1 (except the "binary not found") is when it reports an error such as Unable to parse file. When that happens, that means that we tried to change the contents of an Elm file to something that is not valid Elm code, and I definitely want to let the user know when that happens. In practice, that has not happened because we check for this through elm-syntax and I'm not aware of other elm-format errors.
So I think the current behavior is justified.

Thanks for the awesome investigative work, and I hope your work gets merged into npx!

[lydell]

So to summarize:

• Remove the npx approach for calling elm-format. This means:

  • Use --elm-format-path if present.
    • If error.code === "ENOENT", show the “the elm-format you provided not found” message.
    • If statusCode !== 0, show “error when running elm-format”.
  • Otherwise just call elm-format as if it were global.
    • If error.code === "ENOENT", show the “elm-format not found, here are some suggestions” message.
    • If statusCode !== 0, show “error when running elm-format”.

• Keep npx for license. We might be able to avoid command injection by validating GitHub user names and repo names – I don’t think command injections are valid names.

Am I understanding that correctly?

[jfmengels]

That all sounds good.

For the license, I think I already do some validation about the user/repo names (to be confirmed), I'm not sure what can be done about the license question though 🤔

[lydell]

We decided to leave the npx stuff as-is to avoid unnecessary, subtle breaking changes. We’ve done what we can on our side to avoid command injection, and hope that npx fix their part.

We’ll revisit how to call elm-format in the next major version.