Skip to content
Permalink
Browse files

[Security][WIP] Add isSafeShell check before passing extra params to …

…mail()
  • Loading branch information...
barryvdh authored and fabpot committed Dec 29, 2016
1 parent 3547577 commit baf0aea2e99796e77df7abf173ecdc702019b5be
Showing with 62 additions and 2 deletions.
  1. +35 −2 lib/classes/Swift/Transport/MailTransport.php
  2. +27 −0 tests/unit/Swift/Transport/MailTransportTest.php
@@ -238,6 +238,36 @@ private function _getReversePath(Swift_Mime_Message $message)
return $path;
}
/**
* Fix CVE-2016-10074 by disallowing potentially unsafe shell characters.
*
* Note that escapeshellarg and escapeshellcmd are inadequate for our purposes, especially on Windows.
*
* @param string $string The string to be validated
*
* @return bool
*/
private function _isShellSafe($string)
{
// Future-proof
if (escapeshellcmd($string) !== $string || !in_array(escapeshellarg($string), array("'$string'", "\"$string\""))) {
return false;
}
$length = strlen($string);
for ($i = 0; $i < $length; ++$i) {
$c = $string[$i];
// All other characters have a special meaning in at least one common shell, including = and +.
// Full stop (.) has a special meaning in cmd.exe, but its impact should be negligible here.
// Note that this does permit non-Latin alphanumeric characters based on the current locale.
if (!ctype_alnum($c) && strpos('@_-.', $c) === false) {
return false;
}
}
return true;
}
/**
* Return php mail extra params to use for invoker->mail.
*
@@ -249,8 +279,11 @@ private function _getReversePath(Swift_Mime_Message $message)
private function _formatExtraParams($extraParams, $reversePath)
{
if (false !== strpos($extraParams, '-f%s')) {
// no need to escape $reversePath) as mail() already does it
$extraParams = empty($reversePath) ? str_replace('-f%s', '', $extraParams) : sprintf($extraParams, $reversePath);
if (empty($reversePath) || false === $this->_isShellSafe($reversePath)) {
$extraParams = str_replace('-f%s', '', $extraParams);
} else {
$extraParams = sprintf($extraParams, $reversePath);
}
}
return !empty($extraParams) ? $extraParams : null;
@@ -186,6 +186,33 @@ public function testTransportSettingSettingExtraParamsWithoutF()
$transport->send($message);
}
public function testTransportSettingInvalidFromEmail()
{
$invoker = $this->_createInvoker();
$dispatcher = $this->_createEventDispatcher();
$transport = $this->_createTransport($invoker, $dispatcher);
$headers = $this->_createHeaders();
$message = $this->_createMessageWithRecipient($headers);
$message->shouldReceive('getReturnPath')
->zeroOrMoreTimes()
->andReturn(
'"attacker\" -oQ/tmp/ -X/var/www/cache/phpcode.php "@email.com'
);
$message->shouldReceive('getSender')
->zeroOrMoreTimes()
->andReturn(null);
$message->shouldReceive('getFrom')
->zeroOrMoreTimes()
->andReturn(null);
$invoker->shouldReceive('mail')
->once()
->with(\Mockery::any(), \Mockery::any(), \Mockery::any(), \Mockery::any(), null);
$transport->send($message);
}
public function testTransportUsesHeadersFromMessage()
{
$invoker = $this->_createInvoker();

0 comments on commit baf0aea

Please sign in to comment.
You can’t perform that action at this time.