0

I have the following code:

exec('sh cert-check-script-delete.sh', req.body.deletedCert);
console.log(req.body.deletedCert);

The console log correctly shows the req.body.deletedCert is non-empty.

And in cert-check-script-delete.sh I have:

#!/bin/sh

certs.json="./"  # Name of JSON file and directory location
echo -e $1 >> certs.json

But it's just writing an empty line to certs.json

I've also tried: 

exec('sh cert-check-script-delete.sh' + req.body.deletedCert)

But neither formats work

tnoel999888
  • 584
  • 3
  • 12
  • 29
  • BTW, is there a reason you aren't giving your file a proper shebang (`#!/bin/bash`, since `echo -e` isn't safe with `sh`), setting the +x bit, and executing it directly without an explicit interpreter? – Charles Duffy Apr 20 '18 at 15:16
  • Try adding a space after .sh, exec('sh cert-check-script-delete.sh ' + req.body.deletedCert) – Dave Carruthers Apr 20 '18 at 15:17
  • @DaveCarruthers, that's a huge security risk. Let's say someone submits code saying they want to delete the certificate `$(rm -rf ~)`. You **never, ever, ever** should substitute text from incoming requests into text parsed as part of a shell command. – Charles Duffy Apr 20 '18 at 15:18
  • Thanks for the suggestion but writing it in that format is now not even writing the empty line to `certs.json`. Also regarding the shebang, I was not aware there were different types of shebangs and I must've just copied and pasted the shebang from online – tnoel999888 Apr 20 '18 at 15:21
  • @DaveCarruthers, ...particularly since node [explicitly supports](https://nodejs.org/api/child_process.html) passing arguments out-of-band; see `child_process.execFile()` and all the other forms with an explicit argument array. Using string concatenation to form a string to run with a shell is dangerous; passing a list of strings as an out-of-band argument is safe. – Charles Duffy Apr 20 '18 at 15:22
  • @tnoel999888, ...note too that `sh foo.sh` overrides a shebang and always uses the shell `sh`, running your code as a POSIX sh script, not a bash script. If you want to run code with bash, it needs to be invoked with `bash scriptname` -- or, better, by setting permissions and shebang correctly, and running `/path/to/scriptname` and letting that shebang determine the interpreter to use. – Charles Duffy Apr 20 '18 at 15:23
  • I already read the post you linked this to as a duplicate and that's where I saw the `exec('scriptname.sh' + arg)` format and that didn't work for me – tnoel999888 Apr 20 '18 at 15:28
  • @tnoel999888 Add the missing space. exec('scriptname.sh ' + arg) – Dave Carruthers Apr 20 '18 at 15:30
  • @DaveCarruthers, ...but, again, it would be a massive security hole if the advice you're giving was followed, making it utterly irresponsible to offer. Even in administrative interfaces, a shell injection attack is still CVE-worthy -- I'm happy to provide links. – Charles Duffy Apr 20 '18 at 15:33
  • @DaveCarruthers, ...see for example https://nvd.nist.gov/vuln/detail/CVE-2016-9683 -- that's a 9.8 on the CVSSv3 scale and a 10.0 on the CVSSv2 scale, and you're encouraging the OP to create *more* bugs of that type. – Charles Duffy Apr 20 '18 at 15:35
  • @CharlesDuffy not encouraging anything, just telling the OP why his codes not working, simple as that. Thats how the command hes selected works. – Dave Carruthers Apr 20 '18 at 15:37
  • @DaveCarruthers, yes, but if the OP selects something that's not going to do what they want without huge caveats, a good answer will help them make a more appropriate selection instead. "What they want" surely doesn't include executing commands embedded inside the certificate name -- it's the same reason we don't tell people how to make code with [SQL injections](https://xkcd.com/327/) "work", and instead tell them to use bind variables instead. – Charles Duffy Apr 20 '18 at 15:40
  • 1
    @DaveCarruthers thank you this worked! – tnoel999888 Apr 20 '18 at 15:56
  • @DaveCarruthers, ...and *that* is **exactly** why bad answers shouldn't be given at all -- we now have one more product with an undetected security bug in the world. – Charles Duffy Apr 20 '18 at 16:08

1 Answers1

2

Use execFile(), and pass your arguments out-of-band:

child_process.execFile('./cert-check-script-delete.sh', [req.body.deletedCert])

That way your string (from req.body.deletedCert) is passed as a literal argument, not parsed as code. Note that this requires that your script be successfully marked executable (chmod +x check-cert-script-delete.sh), and that it start with a valid shebang.

If you can't fix your file permissions to make your executable, at least pass the arguments out-of-band:

child_process.execFile('/bin/sh', ['./check-cert-script-delete.sh', req.body.deletedCert])
Charles Duffy
  • 280,126
  • 43
  • 390
  • 441
  • Thanks for the answer, but where does `child_process` come from? I have the following variable defined `const exec = require('child_process').exec, child;`, is this sufficient or do I need to import anything else too? – tnoel999888 Apr 20 '18 at 15:31
  • `const child_process = require('child_process')` will get you the whole module, not just one function from it. – Charles Duffy Apr 20 '18 at 15:32
  • Thanks for the suggestion but this still doesn't work, I've used the solution involving the space for now – tnoel999888 Apr 20 '18 at 15:57
  • @tnoel999888, re: "doesn't work" -- it's worth digging into the details. Security matters, and if this doesn't work, it's because something else is broken (your file permissions are wrong, or your shebang is invalid, or so forth). – Charles Duffy Apr 20 '18 at 16:01
  • 1
    @tnoel999888, ...that said, if you **really must** use a shell, at least do it like so: `child_process.execFile('/bin/sh', ['./check-cert-script-delete.sh', req.body.deletedCert])` – Charles Duffy Apr 20 '18 at 16:02