Skip to content

Commit

Permalink
Add test for line breaks in addresses vulnerability
Browse files Browse the repository at this point in the history
Don't allow line breaks in addresses
Don't allow line breaks in SMTP commands
Rearrange tests so slowest tests run last
  • Loading branch information
Synchro committed Nov 1, 2015
1 parent 1cd906b commit 6687a96
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 74 deletions.
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions class.phpmailer.php
Original file line number Diff line number Diff line change
Expand Up @@ -1028,10 +1028,10 @@ public function getLastMessageID()
* 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
Expand All @@ -1040,6 +1040,10 @@ public function getLastMessageID()
*/
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
Expand Down
11 changes: 8 additions & 3 deletions class.smtp.php
Original file line number Diff line number Diff line change
Expand Up @@ -814,15 +814,15 @@ public function quit($close_on_error = true)
* Sets the TO argument to $toaddr.
* Returns true if the recipient was accepted false if it was rejected.
* Implements from rfc 821: RCPT <SP> TO:<forward-path> <CRLF>
* @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)
);
}
Expand Down Expand Up @@ -853,6 +853,11 @@ protected function sendCommand($command, $commandstring, $expect)
$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();
Expand Down
144 changes: 75 additions & 69 deletions test/phpmailerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,6 @@ public function testValidate()
'"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',
Expand Down Expand Up @@ -467,7 +466,7 @@ public function testValidate()
'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',
Expand Down Expand Up @@ -608,6 +607,7 @@ public function testValidate()
'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(
Expand Down Expand Up @@ -1825,73 +1825,11 @@ public function testMiscellaneous()
$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()
{
//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()
public function testBadSMTP()
{
$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');
}

/**
Expand Down Expand Up @@ -2042,11 +1980,79 @@ public function testDuplicateIDNRemoved()
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
* <html>
* <body>
Expand Down

0 comments on commit 6687a96

Please sign in to comment.