[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CADSYzstAHkEB7fegYo_VeDjL34kg032bZ2u6SjHn_AtMWqKSuQ@mail.gmail.com>
Date: Wed, 28 Dec 2016 00:58:54 -0200
From: Dawid Golunski <dawid@...alhackers.com>
To: oss-security@...ts.openwall.com
Cc: Marcus Bointon <marcus@...chromedia.co.uk>
Subject: Re: PHPMailer < 5.2.18 Remote Code Execution [updated
advisory] [CVE-2016-10033]
Hi Alexander,
Cheers.
I've already reported this to Marcus. He's got some more improvements in place.
There will be another revision of my advisory soon.
On Wed, Dec 28, 2016 at 12:24 AM, Solar Designer <solar@...nwall.com> wrote:
> 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
--
Regards,
Dawid Golunski
https://legalhackers.com
t: @dawid_golunski
Powered by blists - more mailing lists
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.
