oss-security - Re: PHPMailer < 5.2.18 Remote Code Execution [updated advisory] [CVE-2016-10033]

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 28 Dec 2016 03:24:14 +0100
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Cc: marcus@...chromedia.co.uk
Subject: Re: PHPMailer < 5.2.18 Remote Code Execution [updated advisory] [CVE-2016-10033]

Dawid,

That's another nice find of yours, thanks!

Going forward, please just "reply" to the same thread whenever you want
to share an updated advisory.  As you realized, having a new thread
means that some people reading the old thread only won't find the new.

Now, I think the fix might be incomplete:

On Tue, Dec 27, 2016 at 09:45:48AM -0200, Dawid Golunski wrote:
> The parameters include the 5th parameter of $params which allows to pass extra
> parameters to sendmail binary installed on the system as per PHP documentation
> of mail() function:
> 
> http://php.net/manual/en/function.mail.php
> 
> As can we see from:
> 
> $params = sprintf('-f%s', $this->Sender);
> 
> PHPMailer uses the Sender variable to build the params string.
[...]
> The vulnerability was responsibly disclosed to PHPMailer vendor.
> The vendor released a critical security release of PHPMailer 5.2.18 to fix the
> issue as notified at:
> 
> https://github.com/PHPMailer/PHPMailer/blob/master/changelog.md
> 
> https://github.com/PHPMailer/PHPMailer/blob/master/SECURITY.md

The fix appears to be in this commit:

https://github.com/PHPMailer/PHPMailer/commit/4835657cd639fbd09afd33307cef164edf807cdc

The code becomes:

        if (!empty($this->Sender) and $this->validateAddress($this->Sender)) {
            $params = sprintf('-f%s', escapeshellarg($this->Sender));
        }

PHP documentation for mail() says this about the 5th parameter:

"This parameter is escaped by escapeshellcmd() internally to prevent
command execution. escapeshellcmd() prevents command execution, but
allows to add additional parameters.  For security reasons, it is
recommended for the user to sanitize this parameter to avoid adding
unwanted parameters to the shell command."

So now we effectively have escapeshellcmd(escapeshellarg()).  Is this
combination meant to be safe?  Maybe escapeshellcmd()'s escaping of
backslashes will stop them from being treated as escape characters for
the single quotes escaped by escapeshellarg()?

PHPMailer itself uses both of these functions elsewhere, but separately,
like this:

        if (!empty($this->Sender)) {
            if ($this->Mailer == 'qmail') {
                $sendmail = sprintf('%s -f%s', escapeshellcmd($this->Sendmail), escapeshellarg($this->Sender));
            } else {
                $sendmail = sprintf('%s -oi -f%s -t', escapeshellcmd($this->Sendmail), escapeshellarg($this->Sender));
            }
        } else {
            if ($this->Mailer == 'qmail') {
                $sendmail = sprintf('%s', escapeshellcmd($this->Sendmail));
            } else {
                $sendmail = sprintf('%s -oi -t', escapeshellcmd($this->Sendmail));
            }
        }

I guess this code runs when PHPMailer does not use mail().  And the code
path leading to mail() is separate.  But I did not study this in detail.
Anyway, my point is that escapeshellcmd(escapeshellarg()) is something
new to PHPMailer.  Let's see how it behaves:

$ cat phpmailer.php
#!/usr/bin/php
<?php
$from = "\"from ' -Xstuff\"@host.tld";
print "From is $from\n";
$arg = escapeshellarg($from);
print 'From is ' . $arg . " after escapeshellarg()\n";
$cmd = escapeshellcmd($arg);
print 'From is ' . $cmd . " after escapeshellcmd(escapeshellarg())\n";
#system('/bin/echo From is ' . $cmd);
mail('root@...alhost', '', '', '', '-f' . $arg);
?>
$ env - strace -fe execve ./phpmailer.php
execve("./phpmailer.php", ["./phpmailer.php"], [/* 0 vars */]) = 0
>From is "from ' -Xstuff"@host.tld
>From is '"from '\'' -Xstuff"@host.tld' after escapeshellarg()
>From is '\"from '\\'' -Xstuff\"@host.tld\' after escapeshellcmd(escapeshellarg())
Process 16698 attached
[pid 16698] execve("/bin/sh", ["sh", "-c", "/usr/sbin/sendmail -t -i -f'\\\"fr"...], [/* 0 vars */]) = 0
[pid 16698] execve("/usr/sbin/sendmail", ["/usr/sbin/sendmail", "-t", "-i", "-f\\\"from \\", "-Xstuff\"@host.tld'"], [/* 3 vars */]) = 0
sendmail: fatal: unsupported: -Xs

I ran this test on a RHEL6'ish and on a RHEL7'ish system, with their
packages of PHP, and the result is the same.

As you can see, /usr/sbin/sendmail (in this case Postfix's, which is why
it isn't accepting "-X") is being run with "-Xstuff\"@host.tld'" as a
separate argument.  (There's also some escaping by strace in this output.
But all we care about is that it's a separate argument, which strace
makes clear.)

Now, can we get a single quote character through PHPMailer's
$this->validateAddress($this->Sender)?  I did not test, but the regexps
included in there do list it among the allowed characters in some
places.  There's also the potential (risk) that this code would be run
with $patternselect == 'noregex', which does almost no validation.
(And if there's no such potential for some reason, then the code
handling 'noregex' should simply be dropped.  Not good to keep insecure
hopefully dead code.)

I didn't intend to look into this issue for real, so I'll hand it over
back to you from this point on.  Please either show how the fix is
sufficient, or confirm that it's indeed insufficient.

Either way, I think a more appropriate fix would be to implement a
trivial SMTP client in PHPMailer and have it talk to 127.0.0.1:25.
Of course, there's also the risk of SMTP command injection, so care
should be taken to avoid that, yet it's a better defined protocol and
the impact of possible injections would be less (unless they exploit a
vulnerability in the SMTP server, but having that would be an issue on
its own).

Failing that, and as another short-term workaround, a stricter sanity
check may be applied to the "Sender" address (and maybe to other
addresses as well).  Perhaps much stricter.  Unfortunately, this will
disallow use of some obscure valid-per-RFC addresses, but that's still a
good tradeoff given the risks.

Escaping is OK for trusted user input.  For untrusted and possibly
malicious input, it just doesn't provide sufficient assurance.  Maybe
PHP documentation should be revised to introduce this distinction in its
descriptions of the escaping functions and their intended use (for SQL
escaping, too, where escaping isn't as safe as prepared statements).
As the documentation currently is, it gives the impression that escaping
is somehow sufficient and is a best practice as the only safety measure
for untrusted input.

Alexander
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.