From 6687a96a18b8f12148881e4ddde795ae477284b0 Mon Sep 17 00:00:00 2001 From: Synchro Date: Sun, 1 Nov 2015 11:12:04 +0100 Subject: [PATCH] Add test for line breaks in addresses vulnerability Don't allow line breaks in addresses Don't allow line breaks in SMTP commands Rearrange tests so slowest tests run last --- changelog.md | 2 + class.phpmailer.php | 8 ++- class.smtp.php | 11 +++- test/phpmailerTest.php | 144 +++++++++++++++++++++-------------------- 4 files changed, 91 insertions(+), 74 deletions(-) diff --git a/changelog.md b/changelog.md index dc9c0fac..5cd891ce 100644 --- a/changelog.md +++ b/changelog.md @@ -3,6 +3,8 @@ * Allow addresses with IDN (Internationalized Domain Name) in PHP 5.3+, thanks to @fbonzon * Allow access to POP3 errors * Make all POP3 private properties and methods protected +* **SECURITY** Fix vulnerability that allowed email addresses with line breaks (valid in RFC5322) to pass to SMTP, permitting message injection at the SMTP level. Mitigated in both the address validator and in the lower-level SMTP class. Thanks to Takeshi Terada. +* Updated Brazilian Portuguese translations (Thanks to @phelipealves) ## Version 5.2.13 (Sep 14th 2015) * Rename internal oauth class to avoid name clashes diff --git a/class.phpmailer.php b/class.phpmailer.php index ebe1baed..0073b5dc 100644 --- a/class.phpmailer.php +++ b/class.phpmailer.php @@ -1028,10 +1028,10 @@ class PHPMailer * Check that a string looks like an email address. * @param string $address The email address to check * @param string $patternselect A selector for the validation pattern to use : - * * `auto` Pick strictest one automatically; + * * `auto` Pick best pattern automatically; * * `pcre8` Use the squiloople.com pattern, requires PCRE > 8.0, PHP >= 5.3.2, 5.2.14; * * `pcre` Use old PCRE implementation; - * * `php` Use PHP built-in FILTER_VALIDATE_EMAIL; same as pcre8 but does not allow 'dotless' domains; + * * `php` Use PHP built-in FILTER_VALIDATE_EMAIL; * * `html5` Use the pattern given by the HTML5 spec for 'email' type form input elements. * * `noregex` Don't use a regex: super fast, really dumb. * @return boolean @@ -1040,6 +1040,10 @@ class PHPMailer */ public static function validateAddress($address, $patternselect = 'auto') { + //Reject line breaks in addresses; it's valid RFC5322, but not RFC5321 + if (strpos($address, "\n") !== false or strpos($address, "\r") !== false) { + return false; + } if (!$patternselect or $patternselect == 'auto') { //Check this constant first so it works when extension_loaded() is disabled by safe mode //Constant was added in PHP 5.2.4 diff --git a/class.smtp.php b/class.smtp.php index 77edd5f3..27ebc758 100644 --- a/class.smtp.php +++ b/class.smtp.php @@ -814,15 +814,15 @@ class SMTP * Sets the TO argument to $toaddr. * Returns true if the recipient was accepted false if it was rejected. * Implements from rfc 821: RCPT TO: - * @param string $toaddr The address the message is being sent to + * @param string $address The address the message is being sent to * @access public * @return boolean */ - public function recipient($toaddr) + public function recipient($address) { return $this->sendCommand( 'RCPT TO', - 'RCPT TO:<' . $toaddr . '>', + 'RCPT TO:<' . $address . '>', array(250, 251) ); } @@ -853,6 +853,11 @@ class SMTP $this->setError("Called $command without being connected"); return false; } + //Reject line breaks in all commands + if (strpos($commandstring, "\n") !== false or strpos($commandstring, "\r") !== false) { + $this->setError("Command '$command' contained line breaks"); + return false; + } $this->client_send($commandstring . self::CRLF); $this->last_reply = $this->get_lines(); diff --git a/test/phpmailerTest.php b/test/phpmailerTest.php index 762c641c..8958231f 100644 --- a/test/phpmailerTest.php +++ b/test/phpmailerTest.php @@ -361,7 +361,6 @@ class PHPMailerTest extends PHPUnit_Framework_TestCase '"Fred\ Bloggs"@iana.org', '"Joe.\Blow"@iana.org', '"Abc@def"@iana.org', - '"Fred Bloggs"@iana.org', 'user+mailbox@iana.org', 'customer/department=shipping@iana.org', '$A12345@iana.org', @@ -467,7 +466,7 @@ class PHPMailerTest extends PHPUnit_Framework_TestCase 'first.last@[IPv6:a1::b2:11.22.33.44]', 'test@test.com', 'test@xn--example.com', - 'test@example.com', + 'test@example.com' ); $invalidaddresses = array( 'first.last@sub.do,com', @@ -608,6 +607,7 @@ class PHPMailerTest extends PHPUnit_Framework_TestCase 'first.last@[IPv6:a1:a2:a3:a4:b1:b2:b3:]', 'first.last@[IPv6::a2:a3:a4:b1:b2:b3:b4]', 'first.last@[IPv6:a1:a2:a3:a4::b1:b2:b3:b4]', + "(\r\n RCPT TO:websec02@d.mbsd.jp\r\n DATA \\\nSubject: spam10\\\n\r\n Hello,\r\n this is a spam mail.\\\n.\r\n QUIT\r\n ) a@gmail.com" //This is valid RCC5322, but we don't want to allow it ); // IDNs in Unicode and ASCII forms. $unicodeaddresses = array( @@ -1825,73 +1825,11 @@ EOT; $this->assertEquals($q['extension'], 'mp3', 'Windows extension not matched'); $this->assertEquals($q['filename'], '飛兒樂 團光茫', 'Windows filename not matched'); } - - /** - * Use a fake POP3 server to test POP-before-SMTP auth. - * With a known-good login - */ - public function testPopBeforeSmtpGood() + public function testBadSMTP() { - //Start a fake POP server - $pid = shell_exec('nohup ./runfakepopserver.sh >/dev/null 2>/dev/null & printf "%u" $!'); - $this->pids[] = $pid; - - sleep(2); - //Test a known-good login - $this->assertTrue( - POP3::popBeforeSmtp('localhost', 1100, 10, 'user', 'test', $this->Mail->SMTPDebug), - 'POP before SMTP failed' - ); - //Kill the fake server - shell_exec('kill -TERM '.escapeshellarg($pid)); - sleep(2); - } - - /** - * Use a fake POP3 server to test POP-before-SMTP auth. - * With a known-bad login - */ - public function testPopBeforeSmtpBad() - { - //Start a fake POP server on a different port - //so we don't inadvertently connect to the previous instance - $pid = shell_exec('nohup ./runfakepopserver.sh 1101 >/dev/null 2>/dev/null & printf "%u" $!'); - $this->pids[] = $pid; - - sleep(2); - //Test a known-bad login - $this->assertFalse( - POP3::popBeforeSmtp('localhost', 1101, 10, 'user', 'xxx', $this->Mail->SMTPDebug), - 'POP before SMTP should have failed' - ); - shell_exec('kill -TERM '.escapeshellarg($pid)); - sleep(2); - } - - /** - * Test SMTP host connections. - * This test can take a long time, so run it last - */ - public function testSmtpConnect() - { - $this->Mail->SMTPDebug = 4; //Show connection-level errors - $this->assertTrue($this->Mail->smtpConnect(), 'SMTP single connect failed'); - $this->Mail->smtpClose(); - $this->Mail->Host = "ssl://localhost:12345;tls://localhost:587;10.10.10.10:54321;localhost:12345;10.10.10.10"; - $this->assertFalse($this->Mail->smtpConnect(), 'SMTP bad multi-connect succeeded'); - $this->Mail->smtpClose(); - $this->Mail->Host = "localhost:12345;10.10.10.10:54321;" . $_REQUEST['mail_host']; - $this->assertTrue($this->Mail->smtpConnect(), 'SMTP multi-connect failed'); - $this->Mail->smtpClose(); - $this->Mail->Host = " localhost:12345 ; " . $_REQUEST['mail_host'] . ' '; - $this->assertTrue($this->Mail->smtpConnect(), 'SMTP hosts with stray spaces failed'); - $this->Mail->smtpClose(); - $this->Mail->Host = $_REQUEST['mail_host']; - //Need to pick a harmless option so as not cause problems of its own! socket:bind doesn't work with Travis-CI - $this->assertTrue( - $this->Mail->smtpConnect(array('ssl' => array('verify_depth' => 10))), - 'SMTP connect with options failed' - ); + $this->Mail->smtpConnect(); + $smtp = $this->Mail->getSMTPInstance(); + $this->assertFalse($smtp->mail("somewhere\nbad"), 'Bad SMTP command containing breaks accepted'); } /** @@ -2042,11 +1980,79 @@ EOT; count($this->Mail->getReplyToAddresses()), 'Bad count of "reply-to" addresses'); } + + /** + * Use a fake POP3 server to test POP-before-SMTP auth. + * With a known-good login + */ + public function testPopBeforeSmtpGood() + { + //Start a fake POP server + $pid = shell_exec('nohup ./runfakepopserver.sh >/dev/null 2>/dev/null & printf "%u" $!'); + $this->pids[] = $pid; + + sleep(2); + //Test a known-good login + $this->assertTrue( + POP3::popBeforeSmtp('localhost', 1100, 10, 'user', 'test', $this->Mail->SMTPDebug), + 'POP before SMTP failed' + ); + //Kill the fake server + shell_exec('kill -TERM ' . escapeshellarg($pid)); + sleep(2); + } + + /** + * Use a fake POP3 server to test POP-before-SMTP auth. + * With a known-bad login + */ + public function testPopBeforeSmtpBad() + { + //Start a fake POP server on a different port + //so we don't inadvertently connect to the previous instance + $pid = shell_exec('nohup ./runfakepopserver.sh 1101 >/dev/null 2>/dev/null & printf "%u" $!'); + $this->pids[] = $pid; + + sleep(2); + //Test a known-bad login + $this->assertFalse( + POP3::popBeforeSmtp('localhost', 1101, 10, 'user', 'xxx', $this->Mail->SMTPDebug), + 'POP before SMTP should have failed' + ); + shell_exec('kill -TERM ' . escapeshellarg($pid)); + sleep(2); + } + + /** + * Test SMTP host connections. + * This test can take a long time, so run it last + */ + public function testSmtpConnect() + { + $this->Mail->SMTPDebug = 4; //Show connection-level errors + $this->assertTrue($this->Mail->smtpConnect(), 'SMTP single connect failed'); + $this->Mail->smtpClose(); + $this->Mail->Host = "ssl://localhost:12345;tls://localhost:587;10.10.10.10:54321;localhost:12345;10.10.10.10"; + $this->assertFalse($this->Mail->smtpConnect(), 'SMTP bad multi-connect succeeded'); + $this->Mail->smtpClose(); + $this->Mail->Host = "localhost:12345;10.10.10.10:54321;" . $_REQUEST['mail_host']; + $this->assertTrue($this->Mail->smtpConnect(), 'SMTP multi-connect failed'); + $this->Mail->smtpClose(); + $this->Mail->Host = " localhost:12345 ; " . $_REQUEST['mail_host'] . ' '; + $this->assertTrue($this->Mail->smtpConnect(), 'SMTP hosts with stray spaces failed'); + $this->Mail->smtpClose(); + $this->Mail->Host = $_REQUEST['mail_host']; + //Need to pick a harmless option so as not cause problems of its own! socket:bind doesn't work with Travis-CI + $this->assertTrue( + $this->Mail->smtpConnect(array('ssl' => array('verify_depth' => 10))), + 'SMTP connect with options failed' + ); + } } /** * This is a sample form for setting appropriate test values through a browser - * These values can also be set using a file called testbootstrap.php (not in svn) in the same folder as this script + * These values can also be set using a file called testbootstrap.php (not in repo) in the same folder as this script * which is probably more useful if you run these tests a lot * *