0

in one of the project I am working with, we were using backtip approach to run system commands.

resp = `7z x #{zip_file_path} -p#{password} -o#{output_path}`

which works fine. But since it might lead to command injection vulnerability we are planning to use exec or open3. With open3 we are facing issue in executing system commands. We referred this to resolve command injection.

stdin, stdout, stderr = Open3.popen3("7z", "x", zip_file_path, "-p", password, "-o", output_path)

But this leads to below error

error = stderr.readlines
# ["\n", "\n", "Command Line Error:\n", "Too short switch:\n", "-o\n"]

This works when I include params like this.

stdin, stdout, stderr = Open3.popen3("7z", "x", zip_file_path, "-p#{password}", "-o#{output_path}")

But shouldn't we pass arguments separately to avoid command injection? Or Am I doing anything wrong with first version?

Aparichith
  • 1,452
  • 4
  • 20
  • 40

1 Answers1

4

Your second version (the one that works):

stdin, stdout, stderr = Open3.popen3("7z", "x", zip_file_path, "-p#{password}", "-o#{output_path}")

is perfectly safe. There's no shell involved so there's nothing that will interpret password and output_path other than 7z itself.

Unfortunately 7z has a strange way of parsing switches. Normally you'd expect to pass arguments to switches by saying -p xyz and -pxyz would be a short form of -p -x -y -z. When you say:

Open3.popen3("7z", "x", zip_file_path, "-p", password, "-o", output_path)

that's like saying this in the shell:

7z x zip_file_path -p password -o output_path

but 7z doesn't like those spaces, it wants:

7z x zip_file_path -ppassword -ooutput_path

so you're stuck doing a bit of string interpolation. Even so, there's no shell involved with Open3 so no injection.

mu is too short
  • 426,620
  • 70
  • 833
  • 800
  • Just one more clarification. From the doc it seems with `popen3` I need to close the stdin, stdout and stderr stream. Does `capture3` make more sence here? – Aparichith Dec 11 '21 at 09:57
  • Ah yes the good old stutter arguments. – max Dec 11 '21 at 10:00
  • Yes, I think `capture3` would be a better fit for you. You're not having a conversation with `7z` or feeding it a stream of data, you just want it to unpack an archive and then check its output. – mu is too short Dec 11 '21 at 17:13