security #846 [Security][WIP] Add isSafeShell check before passing ex…

…tra params to mail() (Barry vd. Heuvel)

This PR was squashed before being merged into the 5.x branch (closes #846).

Discussion
----------

[Security][WIP] Add isSafeShell check before passing extra params to mail()

Potential fix for #844

@Zenexer Are you okay with using the (slightly reformatted) code from PHPMailer/PHPMailer#929 here? Do you agree this fixes the issue?

Commits
-------

baf0aea [Security][WIP] Add isSafeShell check before passing extra params to mail()

lib/classes/Swift/Transport/MailTransport.php

@@ -242,6 +242,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.
      *
@@ -253,8 +283,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;

tests/unit/Swift/Transport/MailTransportTest.php

@@ -189,6 +189,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();