2

I found a vulnerability, and I'm not sure how to patch it. I currently accept input from the user and use it to create a terminal command which I run on the server. Here's the code simplified.

// Get user input
$monochromeThreshold = $_POST['monochromeThreshold'];
$originalFile = $_POST['originalFile'];
$monochromeLocation = $_POST['monochromeLocation'];

// Create command
$command = 'convert -density 150 -threshold ' . $monochromeThreshold . '% -fuzz 1% -transparent white ' . $originalFile . ' ' . $monochromeLocation;

// Execute command
$out = system($command);

Is there a way to sanitize the user input or take other actions prevent injection into the terminal with confidence?

Anders
  • 64,406
  • 24
  • 178
  • 215
Goose
  • 1,394
  • 1
  • 11
  • 17
  • Is treshhold an `int`? Then cast it. For the others, sanatize the file names sothat people can't do path traversal. Also quote the filepaths and use `escapeshellarg()` on the last two params. As it stands, people can execute arbitrary shell commands (set `$_POST['monochromeLocation']` to `lol && nc -lvp 1337 -e /bin/bash` for a bind shell on port 1337). On a seperate manner, you use `convert` from ImageMagicks. Is your system patched against the ImageTragick vuln? – Maximilian Gerhardt Jun 13 '16 at 20:37
  • @MaximilianGerhardt This would work well as an answer. I may accept if better answers are not posted in the next day. Could you go into detail on the correct way to prevent path traversal? And thank you, we are patched on the recent ImageMagick vuln. – Goose Jun 13 '16 at 21:48

2 Answers2

1

Php has some escape functions for using user supplied data in commands, escapeshellcmd1 and escapeshellarg2. There are however some limitations when using these functions. If used with a command that has unsafe command line switches. I wrote a detailed blog post about it 3.

You should also ensure your imagemagick installation is up to date and configured correctly to prevent exploitation of the recent imagetragick 4 and popen() 5 vulnerabilities.

wireghoul
  • 5,745
  • 2
  • 17
  • 26
  • I choose this answer because it was a better and more through answer on the PHP functions and using them correctly, however this answer should be noting that validation is a wise idea in this case. – Goose Jun 14 '16 at 14:29
1

I think that you should do two kind of security measures to prevent injection.

1.Firstly, validate the input data, you should use a white list or black list, think in what is the best way to control your input data, the white list is usually used to validate input data, because it's easier to determine what is the correct format for input data. I recommend you to use regular expressions like this:

$originalFile = $_POST['originalFile'];
$regex = "/[a-zA-Z1-9]{1,10}(\.)((jpg)|(gif))/";

if (preg_match($regex, $originalFile)) {
   //correct input data
}

2.Second, there will be characters that you can't avoid to use on your code, then you should use escape functions, the PHP language has some options, you should visit the official page, I think this function could help you:

http://php.net/manual/en/function.addslashes.php

I hope this information helps you, good luck.

hmrojas.p
  • 1,049
  • 1
  • 8
  • 16
  • +1 for the above. You may also want to do range checks on any input values that represent numbers. – Alfred Armstrong Jun 14 '16 at 13:46
  • Do you mean to link commands with a list of numbers? – hmrojas.p Jun 15 '16 at 20:38
  • 1
    No, just hypothetically suppose your command takes a parameter representing a number of subtasks which causes a process to be spawned for each subtask. User input of 1000000 might be problematic, to say the least. So you would want not just to check for a valid number but put an upper bound on it as well. – Alfred Armstrong Jun 16 '16 at 08:23
  • Ok, I get it, you right, even you can avoid a possible attack DoS. – hmrojas.p Jun 16 '16 at 14:27