Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 28 Dec 2016 03:10:01 -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]

I created a new thread with 0day bypass of the patch for CVE-2016-10033 vuln.

Quick update on here too.

The advisory of the bypass which was reported to the vendor and
assigned the CVE of CVE-2016-10045 on 26th December has been made
public and is available at:

https://legalhackers.com/advisories/PHPMailer-Exploit-Remote-Code-Exec-CVE-2016-10045-Vuln-Patch-Bypass.html



On Wed, Dec 28, 2016 at 12:58 AM, Dawid Golunski <dawid@...alhackers.com> wrote:
> 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\"@...t.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"@...t.tld
>> From is '"from '\'' -Xstuff"@...t.tld' after escapeshellarg()
>> From is '\"from '\\'' -Xstuff\"@...t.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\"@...t.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\"@...t.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



-- 
Regards,
Dawid Golunski
https://legalhackers.com
t: @dawid_golunski

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ