Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPMailer - A critical vulnerability #4963

Closed
natiq2004 opened this issue Dec 26, 2016 · 27 comments

Comments

Projects
None yet
9 participants
@natiq2004
Copy link

commented Dec 26, 2016

Please read: http://thehackernews.com/2016/12/phpmailer-security.html

Please update: /system/libraries/Email.php

@doodoori2

This comment has been minimized.

Copy link

commented Dec 27, 2016

Is it a related issue?

@jim-parry

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

How is this a CodeIgniter problem? CodeIgniter does not have any third party email packages, like PHPMailer, bundled with it.

@natiq2004 natiq2004 closed this Dec 27, 2016

@Federkun

This comment has been minimized.

Copy link

commented Dec 27, 2016

$this->email->from($this->input->post('email')); is indeed vulnerable.

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

$this->email->from($this->input->post('email')); is indeed vulnerable.

But only if you've intentionally turned off validation and aren't validating yourself, right? Which would be a self-inflicted vulnerability.

$this->email->from('"attacker\" -oQ/tmp/ -X/var/www/cache/phpcode.php  some"@email.com');

// protected '_debug_msg' => 
//   array (size=1)
//     0 => string 'Invalid email address: "attacker\" -oQ/tmp/ -X/var/www/cache/phpcode.php  some"@email.com<br />' (length=95)
@Federkun

This comment has been minimized.

Copy link

commented Dec 27, 2016

It may be not so obvious,

// this is a valid address
var_dump(filter_var('"foo\"\ -bar/baz"@strange.example.com', FILTER_VALIDATE_EMAIL)); 

I'm sure someone can find a valid email address that filter_var accept and carry out the attack.

Even SQL injection are self-inflicted vulnerability, that's why any decent library shall not expect/pretend that the user provide parameters already escaped. If a vendor has a disableEscape flag that allows any kind of attack, that's not good either IMHO.

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

that's why any decent library shall not expect/pretend that the user provide parameters already escaped.

Well it doesn't, it uses filter_var() to validate the values unless you explicitly turn it off. And it's unfortunate that filter_var() passes your example, but it is technically a valid address per the spec. The issue is whether or not there's a security vulnerability, which I'm honestly not able to reproduce with CI's Email library, which is not 1:1 with PHPMailer's use of mail(). Do you have a reproducible vector?

That said, I'm always for practical implementation of specs rather than literal. It might be worth adding the FILTER_SANITIZE_EMAIL flag to the validation, which would obviate all of this, with the rare negative consequence of failing on an uncommon, but technically valid email address.

@natiq2004 natiq2004 reopened this Dec 28, 2016

@Federkun

This comment has been minimized.

Copy link

commented Dec 28, 2016

I found out that the validate flag is false by default, and when an invalid email is provided no exception is thrown. The validate_email method is only used to populate an array with debug information.

This is all you need to do:

$this->load->library('email');
$this->email->validate = true;

$this->email->from('foo@bar -oQ/tmp/ -X/var/www/test.php');
$this->email->to('test@example.com'); 
$this->email->subject('subject');
$this->email->message('<?php phpinfo(); ?>');	
$this->email->send();

A quick search on github shown that this is a common use case.

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

The docs appear to be wrong. $validate property is enabled by default. But let's assume you disable it, or find something that passes, and that validation doesn't protect us here, since it uses it as a header anyway.

$this->email->from('foo@bar -oQ/tmp/ -X/var/www/test.php');

What vulnerability are you demonstrating? Are you left with the body of your message in a file on your server at /var/www/test.php? If so, can you indicate what PHP version and OS you are using, because I do not see how that is possible without the breakout-double-quote vector in the security report.

I find that CodeIgniter will not send the email at all with a vulnerable attack string due to its implementation of mail() and just throws an error. I've tried with the sendmail send method as well to no effect. Perhaps the CI maintainers could reach out to @Zenexer and invite him to try to make a similar PoC with CI's libs.

@Zenexer

This comment has been minimized.

Copy link

commented Dec 28, 2016

The underlying issue really has nothing to do with PHPMailer, and it's quite possible that numerous frameworks and applications are vulnerable to similar exploits without ever using email. I've gotten very little sleep since I started working on this, so I'll leave you with this while I take a quick nap so you can get a head start searching for vulnerable code. I'll join the search in a bit.

@Zenexer

This comment has been minimized.

Copy link

commented Dec 28, 2016

$this->mailpath.' -oi -f '.escapeshellarg($this->clean_email($this->_headers['From'])).' -t'
This is vulnerable if the user has control over the from address, especially on Windows. There's no CVE for it, but it's closely related to CVE-2016-10045.

@Zenexer

This comment has been minimized.

Copy link

commented Dec 28, 2016

Fix for that issue in PHPMailer: PHPMailer/PHPMailer@833c35f

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

Yeah I tried replicating with the sendmail passthru you link to, even with and without attempting to escape, but cannot induce the vulnerability on my environment. Get some sleep, thanks for the links, I'll try to touch base later today. :)

@Federkun

This comment has been minimized.

Copy link

commented Dec 28, 2016

Do you have a reproducible vector?

I can share my docker container, so you can try by yourself: https://github.com/Federkun/codeigniter-rce

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

Thanks @Federkun, that indeed works. Curious, the same code on my environments send the email but do not create a file on the filesystem regardless of the path.

@Zenexer

This comment has been minimized.

Copy link

commented Dec 29, 2016

@derekjones sendmail needs to have permission to create the file in the specified directory. It'd be interesting to see which user it's actually created under; if it's created in the context of the sendmail daemon, then we've likely found a privilege escalation vulnerability in sendmail. Or, in your case, a privilege-deescalation anti-vulnerability.

@derekjones

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2016

Discovered why I was having trouble reproducing; looks like the postfix-compatible interface for sendmail has disabled -X for writing logs. Thankfully that means that there are many dev and hosting environments in the wild that this is a non-issue even with vulnerable code, but I believe I have addressed the issue in PR #4966.

Many thanks to @Zenexer for bringing this issue to light and for the approach to prevent it. Hopefully it will raise the priority of addressing the underlying issues in PHP itself.

@narfbg

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

Way to report a security issue during holiday season ...

@derekjones Long time no see; thanks for stepping in!

The docs on validate being Off by default aren't actually wrong ... Only our current dev code enables it by default (and you've linked to the develop branch); see #4844.

Here's what I think:

  • Disable the 'sendmail' option on Windows
  • Allow only sh, bash on UNIX-like systems (@Zenexer: is it safe to use escapeshellarg() in this case?)
  • In addition to FILTER_VALIDATE_EMAIL:
    • Regardless of OS/shell in use, validate email account names via the following PCRE expression: '#[a-z0-9._+-]+#i'
    • Validate the hostname part as a regular hostname or IP address

(none of the above applies to SMTP, or should it?)

You may note that I've included the + sign in the pattern above (despite comments in the #4966 patch / @Zenexer's gist), as it's not uncommon in emails nowadays. While it has special meaning, I can't think of a way to exploit it without other special characters being used. Can anybody else?

Is this sufficient? Am I missing something?

Edit: Is it worth looking into a restricted shell (Wikipedia link)?

@narfbg narfbg added the Critical label Jan 3, 2017

@narfbg narfbg added this to the 3.1.3 milestone Jan 3, 2017

narfbg added a commit that referenced this issue Jan 5, 2017

Address #4963
Would supersede PR #4966
@narfbg

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

The commit above should do it IMO.

Still unsure whether we should disable 'sendmail' on Windows?

@Zenexer

This comment has been minimized.

Copy link

commented Jan 5, 2017

@narfbg That should be fine. I haven't found a way to exploit +, but I didn't want to distribute potentially vulnerable code. The only catch is that your patch won't work with certain charsets that are becoming increasingly uncommon, but that may not be a concern to you.

On Windows, sendmail.exe will handle its own parsing, rather than the shell, so you can actually program a secure escape function as long as you're not using PHP's mail(). In either case, as long as you use the same regex as in the previous patch, I'm pretty sure there won't be any issues.

It's safe to use escapshellarg on UNIX-like systems as long as the charset is stateless and ASCII-compatible, the shell is sh or bash, and sh/bash haven't been overridden with mostly-compatible alternatives that add extra features. Notably:

  • On Debian/Ubuntu and derivatives, sh is symlinked to dash. dash implicitly processes escape sequences for certain built-in commands where sh/bash would normally require a flag; for example, echo -e 'test\\test' in sh is the same as echo 'test\\test' in dash.
  • zsh has the same issue as dash, plus it has additional special characters (e.g., ^).
@narfbg

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

@Zenexer Great, thank you!

We'll release version 3.1.3 with this patch soon, most likely Monday.
I'd like put a thank you note in the changelog for this ... Is mentioning you as "Paul Buonopane from NamePros" ok, or do you prefer something else?

@narfbg narfbg closed this Jan 6, 2017

@tmairegasnighto

This comment has been minimized.

Copy link

commented Jan 6, 2017

Any chance you'll patch 2.2.6?

@Zenexer

This comment has been minimized.

Copy link

commented Jan 6, 2017

@narfbg Thanks! "Paul Buonopane from NamePros" is fine.

@narfbg

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

@tmairegasnighto No. We abandoned 2.x more than a year ago, and there have been more security patches released since then.

narfbg added a commit that referenced this issue Jan 9, 2017

Address #4963
Would supersede PR #4966

narfbg added a commit that referenced this issue Jan 9, 2017

@sergio-bobillier

This comment has been minimized.

Copy link

commented Jan 17, 2017

@tmairegasnighto You should migrate your projects to CodeIgniter 3+ as soon as possible, but in the meanwhile here is a patch for CodeIgniter 2.x (in case you still have projects you need to support):

https://gist.github.com/sergio-bobillier/32e47e1743cb0b67837145cc111dbf7e

@tmairegasnighto

This comment has been minimized.

Copy link

commented Jan 17, 2017

@sergio-bobillier

That's great, thank you. I actually spent all weekend trying to port to CI3 but failed as the language switching module the original developer used isn't compatible :( . It was pretty popular, so I suspect a lot of people are in the same boat and supporting CI2 sites.

Thank you so much for the patch!

@narfbg

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2017

... and now that project may never be updated, because there's "no reason" to.

@tmairegasnighto More vulnerabilities have been patched since 2.x was abandoned; this one just generated a lot of noise. I urge you to focus on getting your project up to date.

@tmairegasnighto

This comment has been minimized.

Copy link

commented Feb 8, 2017

@narfbg, actually the fix above didn't seem to match the mailer class from 2.2.6, and with no way to convert to 3.x it actually became easier to move the entire site to Wordpress than to figure out how the custom language switching code was working (and not working in 3.x). Sad outcome, but at least now I'm effectively standardized on one CMS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.