CVE-2016-10033, CVE-2016-10045: More patches, stricter anti-escaping #930

Merged
merged 1 commit into from Dec 28, 2016

Projects

None yet

3 participants

@Zenexer
Contributor
Zenexer commented Dec 28, 2016 edited

Needs manual testing, particularly on Windows.

@Zenexer Zenexer CVE-2016-10033, CVE-2016-10045: More patches, stricter anti-escaping 833c35f
}
}
+
+ // TODO: If possible, this should be changed to escapeshellarg. Needs thorough testing.
+ $sendmail = sprintf($sendmailFmt, escapeshellcmd($this->Sendmail), $this->Sender);
@Zenexer
Zenexer Dec 28, 2016 Contributor

Ideally this should be escapeshellarg rather than escapeshellcmd, but that would break any applications that rely on the existing behavior to pass their own parameters to sendmail via sendmail_path.

@Synchro
Synchro Dec 28, 2016 Member

Isn't escapeshellcmd appropriate here since it is a command, not an arg?

@Zenexer
Zenexer Dec 28, 2016 Contributor

Given its actual implementation, I'm not really sure what the PHP developers were thinking when they created it. I doubt there's ever a legitimate reason to use it. Each argument--including the command/binary itself--should be escaped as a separate argument. At least, that would be the best solution if escapeshellarg were actually complete.

As it stands, escapeshellcmd's effect in this statement is unintuitive and probably not what anyone would expect. For example, it will allow the path to contain spaces, but only if the path is quoted (e.g., set_ini('sendmail_path', '"/some dir/sendmail"')) It's also going to have weird effects on certain characters in the pathname, including the backslashes commonly found on Windows.

@Zenexer
Zenexer Dec 28, 2016 Contributor

Here's an adaptation of PHP's escapeshellcmd and escapeshellarg code that will help you test different scenarios: https://gist.github.com/Zenexer/c123604d57914970ac297413751c3f21 Note that argument parsing in Windows is nothing like it is in bash. For example, commas, equals signs, and semicolons are argument separators, not just whitespace, and escaping highly dependent on context.

@Synchro
Synchro Dec 28, 2016 Member

I don't know what to do with that, but it looks like it would form the basis of a good PR for PHP itself.

@@ -1444,8 +1480,8 @@ protected function mailSend($header, $body)
//This sets the SMTP envelope sender which gets turned into a return-path header by the receiver
if (!empty($this->Sender) and $this->validateAddress($this->Sender)) {
// CVE-2016-10033, CVE-2016-10045: Don't pass -f if characters will be escaped.
- if (escapeshellcmd($this->Sender) === $this->Sender && in_array(escapeshellarg($this->Sender), array("'$this->Sender'", "\"$this->Sender\""))) {
- $params = sprintf('-f%s', escapeshellarg($this->Sender));
@Zenexer
Zenexer Dec 28, 2016 Contributor

Forgot to remove escapeshellarg here, although it shouldn't have had much effect.

@@ -1364,20 +1364,24 @@ public function postSend()
*/
protected function sendmailSend($header, $body)
{
- // CVE-2016-10033, CVE-2016-10045: Don't pass -f if characters will be escaped by escapeshellcmd when popen is called due to safe mode.
- if (!empty($this->Sender) || ini_get('safe_mode')) {
+ // CVE-2016-10033, CVE-2016-10045: Don't pass -f if characters will be escaped.
@Zenexer
Zenexer Dec 28, 2016 Contributor

I've completely removed the safe_mode test because it's still insecure even without safe mode. escapeshellcmd and escapeshellarg miss far too many cases, especially on Windows.

@Synchro
Synchro Dec 28, 2016 Member

Do you have further commits to clean up these small things before I merge?

@Zenexer
Zenexer Dec 28, 2016 Contributor

Probably not. I was trying to fix the CVEs without giving away too much about the underlying issues, hence the incompleteness, but I've pretty much given up on that with this latest pull request. However, it's 7 AM here and I haven't slept, so it's certainly possible I'm missing something.

@Synchro
Synchro Dec 28, 2016 Member

I think the cat was out of the bag some years ago: https://bugs.php.net/search.php?cmd=display&search_for=escapeshellarg, but it doesn't look like much has been done.

@Zenexer
Zenexer Dec 28, 2016 Contributor

That's a bit concerning. I'll see if I can come up with some sort of hackish long-term solution when I wake up. This should do for now, though.

@Synchro Synchro referenced this pull request Dec 28, 2016
@Zenexer @Synchro Zenexer + Synchro Fix for CVE-2016-10045 and CVE-2016-10033 (#929)
* Fix for CVE-2016-10045 and CVE-2016-10033

* CVE-2016-10033: Also check escapeshellarg
9743ff5
@Synchro Synchro merged commit 833c35f into PHPMailer:master Dec 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Synchro Synchro added a commit that referenced this pull request Dec 28, 2016
@Synchro Synchro Merge PR #930 from @Zanaxer 06aae8b
@Synchro
Member
Synchro commented Dec 28, 2016

Merged, via a slightly roundabout route, thanks

@beat

Wouldn't this exclude valid used email addresses like o'dowd@ireland.example.com ? Per RFC 5322 https://tools.ietf.org/html/rfc5322#section-3.4.1 in addition of alpha-numerical characters, following characters are allowed in the local part (the one before the @:

  • Characters ! # $ % & ' * + - / = ? ^ _ { | } ~`
  • Character . (dot) provided that it is not the first or last character, and provided also that it does not appear two or more times consecutively.
Contributor

Yes. There's no way around it at the moment.

Member
Synchro replied Dec 28, 2016 edited

Yes, it's a deliberate RFC contravention that may be improved in future, but remember that you don't need to be able to stick any old address in here - it's only the envelope sender that this applies to. The problem is that many valid email addresses are also valid attack strings, so we have to cut down what's allowed in the absence of reliable escaping. You can still send to and from any valid address you like, but the envelope sender has been cut back.

beat replied Dec 28, 2016

I understand, but don't most users of PHPMailer have envelope sender and from address being the same ? Most users are not even aware that they must not match (and in fact they should probably match for inbox-deliverability).

Imho, as it's only the "From" address that is attackable, most real-world PHPMailer use scenarios are not vulnerable. The only time you have user-provided From addresses is when a site sends on behalf of a user, and even then, putting the user-address in the envelope and "From:" field to send from the web-server is wrong and bad practice. The user address should imho be put in the "Reply-to:" field, which is not concerned by the RSE vulnerability, and the site's email put in the envelope and From fields. Sites who don't do that experience catastrophic inbox-deliverability, because all properly configured email domains would make the email fail the SPF, DKIM and DMARC tests and either make the emails refused or going straight to the spam box.

Thus, do you also think that the urgency and panic-level of this vulnerability could be lowered, and proper escaping put in place without rush ?

Member

Yes, all that has been recommended practice for years - any viable use case for user-provided from died with the advent of SPF. I recommend having a read of the doc I put in the wiki. Proper escaping isn't a simple problem, and it requires changes in PHP itself. I have no control over the panic level...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment