This does not prevent all forms of command injection.
<?php
$files_to_archive = [];
foreach ($_GET['file'] as $file) {
$files_to_archive[] = escapeshellarg($file);
}
exec("tar cf my.tar " . implode(' ', $files_to_archive));
?>
Despite correct escaping to prevent shell injection, this will run the specified code. The arguments instruct tar to run a command in bash. You can then check the /tmp directory to verify that the code ran.
Of course, an attacker would replace this with a more malicious variant. For black box vulnerability testing, a sleep of a few seconds can be safely used to determine whether the parameter is vulnerable.
The vulnerability here is that tar, like nearly every other program, will interpret arguments that start with a hyphen as an option that modifies its behaviour. Many programs, such as tcpdump, man, zip, gpg, tar, etc., have an option to execute another command. Even if you use a program that does not (and will never) have such an execution option, its functioning would be influenced by the specified extra options, either on purpose or by accident because some string just happens to start with a hyphen (similar to how fields vulnerable to SQL injection accidentally break on any data with an apostrophe or quote symbol in it: it's just bad UX).
Many programs allow using a double hyphen, --, to separate positional arguments from options. If you change the above code to use this exec line, it is no longer vulnerable:
<?php
exec("tar cf my.tar -- " . implode(' ', $files_to_archive));
?>
Not every program supports this separator, in which case you need to find an alternative input method (e.g., nmap -iL targets.txt instead of nmap 2001:db8::/96) or deny starting arguments with a hyphen.
Of course, ideally one would use data bindings via libraries to avoid having to do the dangerous escaping dance altogether, similar to parameterized SQL queries, but I did not yet see this caveat mentioned and thought it worth adding for when escapeshellarg() is still used.
