Merge branch 'hotfix/140'

Close #140
zendframework · Jun 8, 2017 · 536377c · 536377c
2 parents 9c8da9e + e322082
commit 536377c
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,15 @@ All notable changes to this project will be documented in this file, in reverse
 
 - Nothing.
 
+### Changed
+
+- [#140](https://github.com/zendframework/zend-mail/pull/140) updates the
+  `Sendmail` transport such that `From` and `Sender` addresses are passed to
+  `escapeshellarg()` when forming the `-f` argument for the `sendmail` binary.
+  While malformed addresses should never reach this class, this extra hardening
+  helps ensure safety in cases where a developer codes their own
+  `AddressInterface` implementations for these types of addresses.
+
 ### Deprecated
 
 - Nothing.

diff --git a/src/Transport/Sendmail.php b/src/Transport/Sendmail.php
@@ -257,15 +257,15 @@ protected function prepareParameters(Mail\Message $message)
 
         $sender = $message->getSender();
         if ($sender instanceof AddressInterface) {
-            $parameters .= ' -f' . $sender->getEmail();
+            $parameters .= ' -f' . \escapeshellarg($sender->getEmail());
             return $parameters;
         }
 
         $from = $message->getFrom();
         if (count($from)) {
             $from->rewind();
             $sender      = $from->current();
-            $parameters .= ' -f' . $sender->getEmail();
+            $parameters .= ' -f' . \escapeshellarg($sender->getEmail());
             return $parameters;
         }
 

diff --git a/test/MessageTest.php b/test/MessageTest.php
@@ -12,6 +12,7 @@
 use stdClass;
 use Zend\Mail\Address;
 use Zend\Mail\AddressList;
+use Zend\Mail\Exception;
 use Zend\Mail\Header;
 use Zend\Mail\Headers;
 use Zend\Mail\Message;
@@ -853,4 +854,16 @@ public function testMailHeaderContainsZeroValue()
         $msg = Message::fromString($message);
         $this->assertContains('X-Spam-Score: 0', $msg->toString());
     }
+
+    /**
+     * @ref CVE-2016-10033 which targeted WordPress
+     */
+    public function testSecondCodeInjectionInFromHeader()
+    {
+        $message = new Message();
+        $this->setExpectedException(Exception\InvalidArgumentException::class);
+        // @codingStandardsIgnoreStart
+        $message->setFrom('user@xenial(tmp1 -be ${run{${substr{0}{1}{$spool_directory}}usr${substr{0}{1}{$spool_directory}}bin${substr{0}{1}{$spool_directory}}touch${substr{10}{1}{$tod_log}}${substr{0}{1}{$spool_directory}}tmp${substr{0}{1}{$spool_directory}}test}}  tmp2)', 'Sender\'s name');
+        // @codingStandardsIgnoreEnd
+    }
 }
diff --git a/test/Protocol/ProtocolTraitTest.php b/test/Protocol/ProtocolTraitTest.php
@@ -24,6 +24,9 @@ public function testTls12Version()
     {
         $mock = $this->getMockForTrait(ProtocolTrait::class);
 
-        $this->assertNotEmpty(STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT & $mock->getCryptoMethod(), 'TLSv1.2 must be present in crypto method list');
+        $this->assertNotEmpty(
+            STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT & $mock->getCryptoMethod(),
+            'TLSv1.2 must be present in crypto method list'
+        );
     }
 }
diff --git a/test/Transport/SendmailTest.php b/test/Transport/SendmailTest.php
@@ -9,6 +9,11 @@
 
 namespace ZendTest\Mail\Transport;
 
+use ReflectionMethod;
+use Zend\Mail\Address\AddressInterface;
+use Zend\Mail\AddressList;
+use Zend\Mail\Header;
+use Zend\Mail\Headers;
 use Zend\Mail\Message;
 use Zend\Mail\Transport\Exception\RuntimeException;
 use Zend\Mail\Transport\Sendmail;
@@ -88,7 +93,7 @@ public function testReceivesMailArtifactsOnUnixSystems()
         $this->assertContains("From: zf-devteam@zend.com,\n Matthew <matthew@zend.com>\n", $this->additional_headers);
         $this->assertContains("X-Foo-Bar: Matthew\n", $this->additional_headers);
         $this->assertContains("Sender: Ralph Schindler <ralph.schindler@zend.com>\n", $this->additional_headers);
-        $this->assertEquals('-R hdrs -fralph.schindler@zend.com', $this->additional_parameters);
+        $this->assertEquals('-R hdrs -f\'ralph.schindler@zend.com\'', $this->additional_parameters);
     }
 
     public function testReceivesMailArtifactsOnWindowsSystems()
@@ -158,4 +163,53 @@ public function testValidEmailLocaDomainInFromHeader()
         $this->transport->send($message);
         $this->assertContains('From: Foo Bar <"foo-bar"@domain>', $this->additional_headers);
     }
+
+    /**
+     * @ref CVE-2016-10033 which targeted WordPress
+     */
+    public function testPrepareParametersEscapesSenderUsingEscapeShellArg()
+    {
+        // @codingStandardsIgnoreStart
+        $injectedEmail = 'user@xenial(tmp1 -be ${run{${substr{0}{1}{$spool_directory}}usr${substr{0}{1}{$spool_directory}}bin${substr{0}{1}{$spool_directory}}touch${substr{10}{1}{$tod_log}}${substr{0}{1}{$spool_directory}}tmp${substr{0}{1}{$spool_directory}}test}}  tmp2)';
+        // @codingStandardsIgnoreEnd
+
+        $sender = $this->prophesize(AddressInterface::class);
+        $sender->getEmail()->willReturn($injectedEmail);
+
+        $message = $this->prophesize(Message::class);
+        $message->getSender()->will([$sender, 'reveal']);
+        $message->getFrom()->shouldNotBeCalled();
+
+        $r = new ReflectionMethod($this->transport, 'prepareParameters');
+        $r->setAccessible(true);
+
+        $parameters = $r->invoke($this->transport, $message->reveal());
+        $this->assertEquals(' -f' . escapeshellarg($injectedEmail), $parameters);
+    }
+
+    /**
+     * @ref CVE-2016-10033 which targeted WordPress
+     */
+    public function testPrepareParametersEscapesFromAddressUsingEscapeShellArg()
+    {
+        // @codingStandardsIgnoreStart
+        $injectedEmail = 'user@xenial(tmp1 -be ${run{${substr{0}{1}{$spool_directory}}usr${substr{0}{1}{$spool_directory}}bin${substr{0}{1}{$spool_directory}}touch${substr{10}{1}{$tod_log}}${substr{0}{1}{$spool_directory}}tmp${substr{0}{1}{$spool_directory}}test}}  tmp2)';
+        // @codingStandardsIgnoreEnd
+
+        $address = $this->prophesize(AddressInterface::class);
+        $address->getEmail()->willReturn($injectedEmail)->shouldBeCalledTimes(2);
+
+        $from = new AddressList();
+        $from->add($address->reveal());
+
+        $message = $this->prophesize(Message::class);
+        $message->getSender()->willReturn(null);
+        $message->getFrom()->willReturn($from);
+
+        $r = new ReflectionMethod($this->transport, 'prepareParameters');
+        $r->setAccessible(true);
+
+        $parameters = $r->invoke($this->transport, $message->reveal());
+        $this->assertEquals(' -f' . escapeshellarg($injectedEmail), $parameters);
+    }
 }