Permalink
Browse files

Check that sendmail path exists before trying to use it

Validate Sender address
Consistent use of empty()
  • Loading branch information...
Synchro committed Dec 23, 2016
1 parent 5f87e21 commit 4835657cd639fbd09afd33307cef164edf807cdc
Showing with 12 additions and 8 deletions.
  1. +12 −8 class.phpmailer.php
View
@@ -164,6 +164,7 @@ class PHPMailer
/** /**
* The path to the sendmail program. * The path to the sendmail program.
+ * Must contain only a path to an executable, with no parameters or switches
* @var string * @var string
*/ */
public $Sendmail = '/usr/sbin/sendmail'; public $Sendmail = '/usr/sbin/sendmail';
@@ -692,7 +693,7 @@ private function mailPassthru($to, $subject, $body, $header, $params)
$subject = $this->encodeHeader($this->secureHeader($subject)); $subject = $this->encodeHeader($this->secureHeader($subject));
} }
- //Can't use additional_parameters in safe_mode + //Can't use additional_parameters in safe_mode, calling mail() with null params breaks
//@link http://php.net/manual/en/function.mail.php //@link http://php.net/manual/en/function.mail.php
if (ini_get('safe_mode') or !$this->UseSendmailOptions or is_null($params)) { if (ini_get('safe_mode') or !$this->UseSendmailOptions or is_null($params)) {
$result = @mail($to, $subject, $body, $header); $result = @mail($to, $subject, $body, $header);
@@ -1364,7 +1365,10 @@ public function postSend()
*/ */
protected function sendmailSend($header, $body) protected function sendmailSend($header, $body)
{ {
- if ($this->Sender != '') { + if (!(is_file($this->Sendmail) and is_executable($this->Sendmail))) {
+ throw new phpmailerException($this->lang('execute') . $this->Sendmail, self::STOP_CRITICAL);
+ }
+ if (!empty($this->Sender) and $this->validateAddress($this->Sender)) {
if ($this->Mailer == 'qmail') { if ($this->Mailer == 'qmail') {
$sendmail = sprintf('%s -f%s', escapeshellcmd($this->Sendmail), escapeshellarg($this->Sender)); $sendmail = sprintf('%s -f%s', escapeshellcmd($this->Sendmail), escapeshellarg($this->Sender));
} else { } else {
@@ -1441,10 +1445,10 @@ protected function mailSend($header, $body)
$params = null; $params = null;
//This sets the SMTP envelope sender which gets turned into a return-path header by the receiver //This sets the SMTP envelope sender which gets turned into a return-path header by the receiver
- if (!empty($this->Sender)) { + if (!empty($this->Sender) and $this->validateAddress($this->Sender)) {

This comment has been minimized.

Show comment
Hide comment

This comment has been minimized.

Show comment
Hide comment
@Synchro

Synchro Dec 26, 2016

Member

Quite correct, thanks. This will be fixed in next release.

@Synchro

Synchro Dec 26, 2016

Member

Quite correct, thanks. This will be fixed in next release.

- $params = sprintf('-f%s', $this->Sender); + $params = sprintf('-f%s', escapeshellarg($this->Sender));
} }
- if ($this->Sender != '' and !ini_get('safe_mode')) { + if (!empty($this->Sender) and !ini_get('safe_mode') and $this->validateAddress($this->Sender)) {
$old_from = ini_get('sendmail_from'); $old_from = ini_get('sendmail_from');
ini_set('sendmail_from', $this->Sender); ini_set('sendmail_from', $this->Sender);
} }
@@ -1498,10 +1502,10 @@ protected function smtpSend($header, $body)
if (!$this->smtpConnect($this->SMTPOptions)) { if (!$this->smtpConnect($this->SMTPOptions)) {
throw new phpmailerException($this->lang('smtp_connect_failed'), self::STOP_CRITICAL); throw new phpmailerException($this->lang('smtp_connect_failed'), self::STOP_CRITICAL);
} }
- if ('' == $this->Sender) { + if (!empty($this->Sender) and $this->validateAddress($this->Sender)) {
- $smtp_from = $this->From;
- } else {
$smtp_from = $this->Sender; $smtp_from = $this->Sender;
+ } else {
+ $smtp_from = $this->From;
} }
if (!$this->smtp->mail($smtp_from)) { if (!$this->smtp->mail($smtp_from)) {
$this->setError($this->lang('from_failed') . $smtp_from . ' : ' . implode(',', $this->smtp->getError())); $this->setError($this->lang('from_failed') . $smtp_from . ' : ' . implode(',', $this->smtp->getError()));

11 comments on commit 4835657

@vaibhavch

This comment has been minimized.

Show comment
Hide comment
@vaibhavch

vaibhavch Dec 26, 2016

$this->Sender is usually equals to $mail->setFrom()

Since $Sender is defined in the script itself and not an out of the box input data. Then why it needs extra validation? as you tagged this small change as critical security update.

$this->Sender is usually equals to $mail->setFrom()

Since $Sender is defined in the script itself and not an out of the box input data. Then why it needs extra validation? as you tagged this small change as critical security update.

@Synchro

This comment has been minimized.

Show comment
Hide comment
@Synchro

Synchro Dec 26, 2016

Member

It's not the validation that's the vuln. It's common for From to be set using user input (even though it almost guarantees delivery failure and docs say not to), and that is often copied to Sender.

Member

Synchro replied Dec 26, 2016

It's not the validation that's the vuln. It's common for From to be set using user input (even though it almost guarantees delivery failure and docs say not to), and that is often copied to Sender.

@ali-master

This comment has been minimized.

Show comment
Hide comment
@bhdresh

This comment has been minimized.

Show comment
Hide comment
@bhdresh

bhdresh Dec 28, 2016

Looking at the details, it looks like this vulnerability could be exploited only if the mail script allow a guest/user to provide "envelope from" which is very less likely and should not be claimed as "Critical PHPMailer Flaw leaves Millions of Websites Vulnerable to Remote Exploit" :)

Looking at the details, it looks like this vulnerability could be exploited only if the mail script allow a guest/user to provide "envelope from" which is very less likely and should not be claimed as "Critical PHPMailer Flaw leaves Millions of Websites Vulnerable to Remote Exploit" :)

@DrMurx

This comment has been minimized.

Show comment
Hide comment
@DrMurx

DrMurx Dec 28, 2016

@bhdresh @vaibhavch Au contraire - this is a quite typical scenario for simple contact forms on websites: Those usually have a "My email address" field for the user to enter his email address, and this will end up as From in the generated email to allow website owner to just press "Reply" in his inbox. Therefore, if PhpMailer is used without any further configuration, the vulnerable $Sender field will be initialized with the unvalidated value in setFrom.

@bhdresh @vaibhavch Au contraire - this is a quite typical scenario for simple contact forms on websites: Those usually have a "My email address" field for the user to enter his email address, and this will end up as From in the generated email to allow website owner to just press "Reply" in his inbox. Therefore, if PhpMailer is used without any further configuration, the vulnerable $Sender field will be initialized with the unvalidated value in setFrom.

@Synchro

This comment has been minimized.

Show comment
Hide comment
@Synchro

Synchro Dec 28, 2016

Member

@DrMurx No, it's typical of naïve contact forms that don't want their mail to arrive - forging the from address means that messages will fail SPF and DMARC checks, and this has been true for several years. The From address is validated before use; the problem is that a valid email address can also be a workable exploit. The correct way to implement contact forms (as advocated in the PHPMailer docs for years, and my answers to hundreds of SO questions) is to use fixed to and from addresses and put the submitter's address in a reply-to header. This avoids both these vulnerabilities, allows replies to go where they're expected, and also doesn't involve forgery.

Member

Synchro replied Dec 28, 2016

@DrMurx No, it's typical of naïve contact forms that don't want their mail to arrive - forging the from address means that messages will fail SPF and DMARC checks, and this has been true for several years. The From address is validated before use; the problem is that a valid email address can also be a workable exploit. The correct way to implement contact forms (as advocated in the PHPMailer docs for years, and my answers to hundreds of SO questions) is to use fixed to and from addresses and put the submitter's address in a reply-to header. This avoids both these vulnerabilities, allows replies to go where they're expected, and also doesn't involve forgery.

@DrMurx

This comment has been minimized.

Show comment
Hide comment
@DrMurx

DrMurx Dec 28, 2016

@Synchro You're right, I was wrong regarding the validation.

However, I'm afraid many naive after work hours spare developers don't care about best practices in the documentation, create "naive contact forms" and host it on poorly administrated virtual servers with MTA on localhost. SPF, DKIM and DMARC are likely considered political parties of a small eastern country. Despite we're in the 21st century for quite a while, this unfortunately is still reality.

But regardless of the potential impact, we should be happy that this is fixed now.

@Synchro You're right, I was wrong regarding the validation.

However, I'm afraid many naive after work hours spare developers don't care about best practices in the documentation, create "naive contact forms" and host it on poorly administrated virtual servers with MTA on localhost. SPF, DKIM and DMARC are likely considered political parties of a small eastern country. Despite we're in the 21st century for quite a while, this unfortunately is still reality.

But regardless of the potential impact, we should be happy that this is fixed now.

@jjamesgz

This comment has been minimized.

Show comment
Hide comment
@jjamesgz

jjamesgz Jan 16, 2017

Any reason we don't just abandon the -f flag and instead set the Errors-To: header? That way even if they sneak a bad address through your validation it won't make it to the command line.

Any reason we don't just abandon the -f flag and instead set the Errors-To: header? That way even if they sneak a bad address through your validation it won't make it to the command line.

@glensc

This comment has been minimized.

Show comment
Hide comment
@glensc

glensc Jan 17, 2017

Contributor

i don't think the Errors-To: behavior is identical. local sendmail wrapper needs to convert that header to Return-Path header to be identical. does any of the MTA do that?

Contributor

glensc replied Jan 17, 2017

i don't think the Errors-To: behavior is identical. local sendmail wrapper needs to convert that header to Return-Path header to be identical. does any of the MTA do that?

@Synchro

This comment has been minimized.

Show comment
Hide comment
@Synchro

Synchro Jan 17, 2017

Member

I would hope not: I would expect it to be an RFC contravention for an MTA to do so. The envelope sender is entirely independent of anything in the message - though they do often use the same value, in my own applications they almost never do (because I use VERP). Switching to SMTP to localhost is an entirely reasonable workaround - and in fact is the preferred way to send anyway. The errors-to header is mentioned in RFC 2076 (where it's marked as "Non-standard, discouraged"), but not in either of RFC 5322 or 5321. Envelope-level errors should be handled at the SMTP level, not by parsing messages. The key thing as far as PHPMailer is concerned is that it's not PHPMailer's call to make - there's no way we can dictate policy to every mail server in the world just because PHP has a security problem.

Member

Synchro replied Jan 17, 2017

I would hope not: I would expect it to be an RFC contravention for an MTA to do so. The envelope sender is entirely independent of anything in the message - though they do often use the same value, in my own applications they almost never do (because I use VERP). Switching to SMTP to localhost is an entirely reasonable workaround - and in fact is the preferred way to send anyway. The errors-to header is mentioned in RFC 2076 (where it's marked as "Non-standard, discouraged"), but not in either of RFC 5322 or 5321. Envelope-level errors should be handled at the SMTP level, not by parsing messages. The key thing as far as PHPMailer is concerned is that it's not PHPMailer's call to make - there's no way we can dictate policy to every mail server in the world just because PHP has a security problem.

@Synchro

This comment has been minimized.

Show comment
Hide comment
@Synchro

Synchro Jan 17, 2017

Member

Separately, I'm intrigued by the fix for this same security issue done by zend_mail; all they do is reject any envelope sender address that contains \", rather than the more aggressive stripping we are doing. I'm not entirely convinced that's sufficient, but it is a much simpler solution than what we have at present.

Member

Synchro replied Jan 17, 2017

Separately, I'm intrigued by the fix for this same security issue done by zend_mail; all they do is reject any envelope sender address that contains \", rather than the more aggressive stripping we are doing. I'm not entirely convinced that's sufficient, but it is a much simpler solution than what we have at present.

Please sign in to comment.