php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #60116 escapeshellcmd() cannot escape the chars which causes shell injection.
Submitted: 2011-10-23 11:16 UTC Modified: 2011-11-19 23:48 UTC
From: hirokawa@php.net Assigned: tyrael
Status: Closed Package: Filter related
PHP Version: trunk-SVN-2011-10-23 (SVN) OS: Ubuntu Linux
Private report: No CVE-ID:
 [2011-10-23 11:16 UTC] hirokawa@php.net
Description:
------------
escapeshellcmd() escapes " and ' only if it isn't paired (it is documented in 
the PHP manual). 

For the test script to look for some keyword in the files of the specified 
directory (/var/data/), the double quotation in the user input as shown below 
cannot be escaped because it is paired (it is found by Mr. Tokumaru).

$_GET['key'] = ':" "/etc/passwd';

The command line will be,

grep ":" "/etc/passwd" /var/data/*

The content of arbitrary file such as /etc/passwd will be shown.

The attached patch (made by Mr. Ohgaki, slightly modified be me) will add an 
option flag for escapeshellcmd().
The recommended code to escape the quotation in this case will be,

$key = escapeshellcmd($_GET['key'], ESCAPE_CMD_ALL);

output: grep ":\" \"/etc/passwd" /var/data/*

The option flag has the three different value.
There is no backward incompatibility because the default behavior 
(ESCAPE_CMD_PAIR) is unchanged.

ESCAPE_CMD_PAIR : escape if it is not paired (default)
ESCAPE_CMD_END  : escape except for end/beginning of the string
ESCAPE_CMD_ALL  : escape without exception (recommended)

In Windows, the quotation is always escaped, but, in other environment,
it is only escaped if it is not paired.

It is highly recommended to apply this patch to prevent the possible shell 
command injection attack.









Test script:
---------------
<?php
$_GET['key'] = ':" "/etc/passwd'; // sample input
$key = escapeshellcmd($_GET['key']);
$cmd = "grep \"$key\" /var/data/*";
echo $cmd,PHP_EOL;
system($cmd);
?>

Expected result:
----------------
grep ":\" \"/etc/passwd" /var/data/*


Actual result:
--------------
grep ":" "/etc/passwd" /var/data/*
[content of /etc/passwd]



Patches

php-escape.patch (last revision 2011-10-23 11:17 UTC) by hirokawa@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-10-23 11:17 UTC] hirokawa@php.net
The following patch has been added/updated:

Patch Name: php-escape.patch
Revision:   1319368645
URL:        https://bugs.php.net/patch-display.php?bug=60116&patch=php-escape.patch&revision=1319368645
 [2011-10-23 13:49 UTC] hirokawa@php.net
Automatic comment from SVN on behalf of hirokawa
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=318342
Log: fixed bug #60116 escapeshellcmd() cannot escape the dangerous quotes.
 [2011-10-23 15:08 UTC] tyrael@php.net
-Assigned To: +Assigned To: hirokawa
 [2011-10-23 15:08 UTC] tyrael@php.net
judging from http://svn.php.net/viewvc?view=revision&revision=318342 this can be 
closed, right?
 [2011-10-24 14:13 UTC] hirokawa@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 [2011-10-24 14:13 UTC] hirokawa@php.net
-Status: Assigned +Status: To be documented
 [2011-10-30 05:57 UTC] hirokawa@php.net
Automatic comment from SVN on behalf of hirokawa
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=318568
Log: added a test script for bug60116 and fixed behabior of ESCAPE_CMD_END.
 [2011-11-10 14:19 UTC] hirokawa@php.net
Automatic comment from SVN on behalf of hirokawa
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=318996
Log: MFH: fixed bug #60116 (escapeshellcmd() cannot escape the characters which cause shell command injection).
 [2011-11-10 15:09 UTC] lbarnaud@php.net
-Status: To be documented +Status: Open
 [2011-11-10 15:09 UTC] lbarnaud@php.net
Hi,

It seems that you are not using escapeshellcmd() correctly, and that's why it's unsafe in the way you are using it.

You are enclosing escapeshellcmd's output in double quotes.

However escapeshellcmd() and escapeshellarg() do not work like mysql_real_escape_string() for example, and you must *not* enclose the string in quotes yourself. (The example in the documentation is wrong.)

When you don't do it it's perfectly fine:

echo escapeshellcmd('foo" "bar');

Result: foo" "bar // the quotes don't allow to inject a command.

echo escapeshellcmd('foo"bar')

Result: foo\"bar // This time the quote is escaped since it's not paired. Again, injecting a command is not possible.

Also, I believe that escapeshell*arg*() should be used instead or escapeshell*cmd*() when escaping an argument:

$cmd = sprintf('grep %s /var/data/*', escapeshellarg($_GET['key']));

(escapeshellcmd() won't escape spaces and would allow to inject an additional argument; escapeshellarg() encloses the whole argument in single quotes and ensures that it's treated as a single argument)
 [2011-11-10 15:18 UTC] lbarnaud@php.net
The example at http://docs.php.net/manual/en/function.escapeshellcmd.php is wrong. It is enclosing an escaped argument in double quotes, but the escapeshellcmd function doesn't expect this.

As a result the second command in the example is unsafe.

IMO the second command in the example should be removed and replaced by a warning telling to use escapeshellarg instead (because escapeshellcmd doesn't escape spaces and an argument escaped by escapeshellcmd may be interpreted as multiple arguments by the shell).
 [2011-11-10 22:49 UTC] hirokawa@php.net
The default behavier which not escaped paired quotes is still dangerous even if 
the single-quotes is used.

$_GET['key'] = ":' '/etc/hosts";
$key = escapeshellcmd($_GET['key']);
$cmd = "grep '$key' /var/data/*"; // <- single quote
system($cmd);  // output: grep ':' '/etc/hosts' /var/data/*

You are right, escapeshellarg() is better than escapeshellcmd() in this case.
But, generally, escapeshellcmd() is used to escape the user input 
(GET/POST/Cookie), the default behavior (paired quotes are not escaped) is 
not recommended.
 [2011-11-11 09:53 UTC] lbarnaud@php.net
> The default behavier which not escaped paired quotes is still dangerous even if the single-quotes is used.

Yes, I was speaking of both single quotes and double quote: Don't enclose the escaped string in quotes at all :)

This is a bit puzzling because all escaping function like mysql_escape_string expect the user to enclose the string in quotes. But escapeshellcmd and escapeshellarg don't.

It's like htmlspecialchars: it just removes the special meaning of special characters.

> But, generally, escapeshellcmd() is used to escape the user input

It shouldn't be the case. escapeshellcmd escapes all control characters from a string, which avoids command injection, redirection, etc but doesn't prevent argument injection (it doesn't escape spaces).
 [2011-11-11 14:52 UTC] hirokawa@php.net
Automatic comment from SVN on behalf of hirokawa
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=319057
Log: revert changes to fix bug #60116.
 [2011-11-11 15:06 UTC] hirokawa@php.net
Thank you for taking the time to write to us, but this is not
a bug. Please double-check the documentation available at
http://www.php.net/manual/ and the instructions on how to report
a bug at http://bugs.php.net/how-to-report.php


 [2011-11-11 15:06 UTC] hirokawa@php.net
-Status: Assigned +Status: Bogus
 [2011-11-11 15:06 UTC] hirokawa@php.net
-Status: Bogus +Status: Closed
 [2011-11-11 15:19 UTC] tyrael@php.net
-Status: Closed +Status: Re-Opened -Type: Bug +Type: Documentation Problem
 [2011-11-11 15:19 UTC] tyrael@php.net
the documentation should be updated  (removing the flawed example, and emphasizing 
the correct usage).
 [2011-11-12 12:09 UTC] hirokawa@php.net
>the documentation should be updated  (removing the flawed example, and 
>emphasizing the correct usage).
Please do by yourself if you have the karma for phpdoc. 
(I already removed the sentence which I added for the patch.)
If you don't have the karma, I will update the document.

And, you need to show a typical example of escapeshellcmd() 
instead of the 'flawed' example you mentioned.
 [2011-11-12 12:09 UTC] hirokawa@php.net
-Status: Re-Opened +Status: Feedback -Assigned To: hirokawa +Assigned To: lbarnaud
 [2011-11-12 13:11 UTC] lbarnaud@php.net
@hirokawa you've assigned the bug to me, but I don't feel confident enough about my english to update the docs myself.

The example could be something like this:

<?php
$e = escapeshellcmd($userinput);
system("echo $e");
?>

And a warning may be added:

The returned string may be interpreted as multiple arguments by the shell. Use escapeshellarg() for escaping arguments.
 [2011-11-14 09:50 UTC] tyrael@php.net
-Assigned To: lbarnaud +Assigned To: tyrael
 [2011-11-14 09:50 UTC] tyrael@php.net
Arnaud, I will fix the docs, thanks for pointing out the problem.
 [2011-11-19 23:04 UTC] tyrael@php.net
Automatic comment from SVN on behalf of tyrael
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=319565
Log: fixing #60116
 [2011-11-19 23:48 UTC] tyrael@php.net
-Status: Feedback +Status: Closed
 [2011-11-19 23:48 UTC] tyrael@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

I updated the documentation that reflect that:
- escapeshellcmd should be used only on the full command
- it will only prevent executing arbitrary commands, but still allow argument 
injection.
- for escaping an argument, escapeshellarg should be used

I didn't mentioned the quoting, because escapeshellarg already describe that the 
argument will be quoted, and as I mentioned, escapeshellcmd should only be used 
with a full command, so the quoting shouldn't be ambiguous there.
http://svn.php.net/viewvc?view=revision&revision=319565
 [2012-04-18 09:47 UTC] laruence@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=50b2e02c045b61f99e8c72d54e6bec055aee98e4
Log: revert changes to fix bug #60116.
 [2012-04-18 09:48 UTC] laruence@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f17a21549319c2ea882649ab1225b8b4f2133eec
Log: fixed bug #60116 escapeshellcmd() cannot escape the dangerous quotes.
 [2012-07-24 23:38 UTC] rasmus@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=50b2e02c045b61f99e8c72d54e6bec055aee98e4
Log: revert changes to fix bug #60116.
 [2012-07-24 23:39 UTC] rasmus@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f17a21549319c2ea882649ab1225b8b4f2133eec
Log: fixed bug #60116 escapeshellcmd() cannot escape the dangerous quotes.
 [2013-11-17 09:35 UTC] laruence@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=50b2e02c045b61f99e8c72d54e6bec055aee98e4
Log: revert changes to fix bug #60116.
 [2013-11-17 09:35 UTC] laruence@php.net
Automatic comment on behalf of hirokawa
Revision: http://git.php.net/?p=php-src.git;a=commit;h=f17a21549319c2ea882649ab1225b8b4f2133eec
Log: fixed bug #60116 escapeshellcmd() cannot escape the dangerous quotes.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC