Hacker News Reader

Comment by faeyanpiraat | Hacker News Reader

It’s like sql injection in php.

It's actually like using unsanitized input in a lot of languages.

Specifically for PHP SQL injection, you can use PDO's prepared statements, and for shell commands you have https://www.php.net/manual/en/function.escapeshellcmd.php and https://www.php.net/manual/en/function.escapeshellarg.php

If you're escaping shell tokens to reduce your arguments to a single executable command then you might as well not bother with forking to a shell in the first place and instead fork that executable directly since you're removing the one benefit forking to a shell has (effectively embedding a DSL). You'd get some small performance improvements in doing so too.

I think it makes sense to prefer this hypothetical API:

  subprocess.run_escaped("tac -- %s | grep 1 > %s", "foo.txt", "bar.txt")

over this:

  with open("bar.txt", "w") as outfile:
      tac = subprocess.Popen(["tac", "--", "foo.txt"], stdout=subprocess.PIPE)
      grep = subprocess.Popen(["grep", "1"], stdin=tac.stdout, stdout=outfile)
  tac.wait()
  grep.wait()

(In practice the shell grammar is so grotesque that I'd never trust it to be 100% correct. But the idea is nice.)

That doesn’t really solve the problem. Those shell tokens are still getting parsed and interpreted as special tokens only you’re now doing it in your host application rather than the Borne shell. And as you’ve pointed out, reimplementing shell syntax is non-trivial and highly error prone.

Plus if you’re going to those lengths then you’re better off embedding a Lua/Python/Perl/whatever interpreter in your application.

Well, it's known as a "command injection". It's a common vulnerability that can happen in all languages.

https://owasp.org/www-community/attacks/Command_Injection

> It's a common vulnerability that can happen in all languages.

From the article:

> I would have written this in Rust, but, alas, it’s not vulnerable to this particular attack :)

But of course, strictly speaking, this isn't the case, as you can always call sh -c and rust doest prevent you from doing that (though makes it harder).

There is nothing fundamental about Rust that prevents command injection vulnerabilities. (What Rust has going for it in this specific case is that the standard library AFAIK doesn't have an easy way of doing it accidentally laying around - if you want to call the shell, you need to do it explicitly)

But that's exactly the point. I'm often the first to tell people that Rust doesn't automatically guarantee safer code however in this instance Rust (and others who don't call `sh` by default) are safer because the developers have to explicitly write code to fork a shell. And if they're explicitly doing that then you'd fucking hope they'd understand the risks of doing so (and at least it would stand out more in a code peer review too).

Whereas standard libraries that silently fork sh are less safer because they introduce the shell injection vulnerability by default thus requiring developers to consciously be aware of the need to sanitise their inputs.

As I've mentioned elsewhere, I personally think it was a boneheaded decision to ever make exec() functions call a shell. Maybe I'll forgive Perl because that has it's origins as being an extension to shell scripting. But by the time node.js was released people really should have known better.

So you're saying Rust is safe because it does exactly what C does?

That's clearly not what they are saying, given that the C standard library contains (only contains, the safer ones are platform-dependent) the vulnerable form.

Where C does things that are a good idea (and there are some) they don't stop being good ideas just because C did them.

> It's a common vulnerability that can happen in all languages.

While that's true, some language's standard libraries don't expose this vulnerability so the developer has to explicitly write bad code. And that's what really matters.

Take node.js for example: it's not going to be obvious that `exec(command)` is actually equivalent to `sh -c "$command"` and thus a lot of people will get caught short if they have unescaped shell script tokens (eg $, <, >, ;, etc) in their command string. And that's very easy to happen.

Personally I think it was a boneheaded decision when most of the languages adopted that approach of forking to sh. If developers wanted to fork to a shell then they could still explicitly write the code to do so (like the `spawn("/bin/sh", "-c", cmd)` example in the article). But by having exec fork sh by default you're creating a risk for all sorts of unexpected behaviour from developers who don't know their language's standard library inside out (which, lets be honest, is going to be most developers given the point of standard libraries is to abstract away that complexity so developers don't need to think about it).

This isn't just a web scale problem either. In fact the example given in that article is a perfect one because its a CI/CD pipeline running trusted input that still failed unexpectedly.

Cannot agree more. To me it seems the original program in the article should be correct, given the `exec` from node would indeed perform a simple exec, instead of actually being an `exec_shell` function.

Reply on news.ycombinator.com