4 ms·

TL;DR: PHPmailer fails to properly sanitize input. When configured to use CLI sendmail, this can lead to arbitrary command execution. But then again, why would

by annnnd 10y ago

TL;DR: PHPmailer fails to properly sanitize input. When configured to use CLI sendmail, this can lead to arbitrary command execution. But then again, why would you send e-mail to just any address someone enters, without validating it for correctness? I know it is tough to validate all addresses according to RFC, but I'd rather block some legitimate users (which btw. probably know more about RFC than me and I'm sure can find a "nicer" e-mail address if they want to) than let some attacker use some vulnerability like this. Always whitelist valid input, never (just) blacklist it. Also, I am baffled that frameworks I encountered never demanded from developer to specify exactly what kind of input it expects via POST & co.. It is trivial to write a set of functions like this: function input_post_email($field_name, $default_value) function input_post_string($field_name, $validation_regex, $default_value) ... The point here is that framework should DEMAND from developer to specify format of each and every input var it needs. It should be difficult to bypass these restrictions, to demotivate developers doing it. This is a first thing I made in every PHP project I started. Combined with Content Security Policy and output filtering it's... well, better than most other solutions. :)

systematical 10y ago
This is true, I couldn't figure out why this wasn't working, then realized I had to comment out this validation: filter_var($app->request()->post('email'), FILTER_VALIDATE_EMAIL)
thaumaturgy 10y ago
Misguided developers incorrectly "validating" email addresses is the reason that one of my clients couldn't use one of the newer TLDs, ".place", and ended up scrapping it and trying to find something they wanted a lot less in the much more crowded .com space. It's also why I far-too-often run into forms that won't let me use a "+" in the username part of my email address, which I use to track who's responsible for sending my email account off to third parties (e.g., "rob+paypal@...."). Some kinds of email validation are better than others. Using regular expressions and strictly adhering to the RFC is the one that developers are usually talking about when they say not to do it. filter_var(..., FILTER_VALIDATE_EMAIL) is sort of okay, although there are lots of edge cases that it doesn't handle correctly.
- adjkant 10y ago
  I somehow went through life not knowing the + trick. Thanks, and commenting to hopefully highlight this for anyone else who didn't know.
  - thaumaturgy 10y ago
    The specific character depends on your MTA configuration. In Postfix this is the recipient_delimiter setting. I went with a "+" because that's the same character Gmail uses. Other mail services may not support this feature. Hope this helps!
- danieltillett 10y ago
  Sometimes not allowing a "+" is an anti-abuse feature. If you have a service tied to a customer providing a unique email address then you don't want them able to use a "+".
  - jackweirdy 10y ago
    That seems like a strange place to verify that a user is only allowed to create one account. After all, they could just create a second email account on a free email provider and use that to sign up. To get around that, the service provider would have to verify identity further down the line, making the email format restriction redundant.
    - danieltillett 10y ago
      I have been down this route and you also have to disable the use of free email accounts. If the service value is not too high then even making people who want to abuse the service go through the process of registering a free email account works. Put a little bit of friction into the process and the script kiddies move onto an easier target.
      - iopq 10y ago
        > I have been down this route and you also have to disable the use of free email accounts. I don't have any other email. Seems like a really bad idea.
DCoder 10y ago
> When configured to use CLI sendmail, That's not correct. PHPMailer can be configured to send mail through raw SMTP, by directly invoking sendmail, or by calling PHP's mail() function (which is itself a wrapper around sendmail). This vulnerability affects only the last mode, when PHP's mail() calls sendmail. If you have PHPMailer configured to call sendmail directly, this vulnerability does not apply.
- annnnd 10y ago
  I stand corrected, thank you for clarifying.
neotek 10y ago
>I know it is tough to validate all addresses according to RFC Understatement of the year! If you were to try and actually validate according to the RFC, chances are you'll introduce new vectors of attack in your code simply because of the complexity of what you're trying to achieve. Honestly, the email address RFC is a litany of insane choices and kludged-together standards to account for the wild west free-for-all that existed before the modern internet emerged, and most of it has no bearing on the reality of how people use and create email addresses today. Better to just check if the address supplied has an @ in it somewhere and be done with it.
- x0 10y ago
  That's how I do it. Check for an @. Anything past that is sendmail's problem.
  - nkozyra 10y ago
    Even though it false positives some valid emails, I've always felt requiring a @ and a . is a good check for public facing validation.
    - Piskvorrr 10y ago
      Technically valid e-mail addresses - but nonsensical ones. I haven't seen an e-mail without a domain cross a network in decades ("cross a network" excepts "user@localhost", and I'm not old enough to have seen any IP-adressed mails ;)). So yes, I believe that in 2016, a match on ^[^@]+@([^.@]+\.)+[^.@]+\.?$ does not have any actual false negatives, although allowing false positives. That's not entirely helpful in this CVE, though.
Synchro 10y ago
This is incorrect. Input sanitisation is not the problem here - PHPMailer sanitises and validates email addresses well enough. The problem is that a valid, sanitised email address can also be an attack in an inappropriate shell context. Much as an SQL injection attack string may be harmless in a shell context but lethal in SQL. Also the vuln does not apply to CLI sendmail, only when sending via the PHP mail() function, and it's at least partly due to bugs in PHP itself.
- annnnd 10y ago
  I am not blaming lack of input sanitization, the bug is clearly still there. What I am saying is that input sanitization would prevent this attack because it will only allow valid characters to get to your mailer, which will exclude spaces, tabs, double and single quotes. Thus I fail to see an exploit vector which would bypass proper input sanitization. Do you see it? In general I agree that PHP mail() function should take a part of blame here for not exposing a sane interface. Spammers have abused many bugs where programmers failed to sanitize "To:" or "From:" addresses and attackers could pass \r and \n characters (which allowed adding BCC fields, effectively sending e-mails to arbitrary addresses). > Also the vuln does not apply to CLI sendmail... Sure it does: >> ...will result in the followig list of arguments passed to sendmail program: EDIT: you probably meant to say that the vulnerability is only triggered when CLI sendmail is called via mail(), which is true. Configuring PHPMail to use sendmail via CLI directly apparently doesn't expose this vulnerability.
  - pilif 10y ago
    > What I am saying is that input sanitization would prevent this attack because it will only allow valid characters to get to your mailer, you have to be careful with the meaning of "valid" though: What's valid in one context might not be in another context. Do you want to limit the character set to the least common denominator of non-special characters valid in all possible contexts your address might be used? What if you don't exclude some character because it's not special in any of the current contexts but then you later add another thing to the mix that uses in-band signalling? Now you need to update your whitelist and remove even more "invalid" characters. This won't end well for you. I would recommend you don't put a restriction on the input (aside of what the RFC defines as valid) and instead correctly escape (or throw if your context doesn't support escaping) when moving the raw data to a new context. In this case, I think the problem is in PHP's `mail()` function that put stuff in shell context without any escaping. The fix should be happing in `mail()` (which is internally shelling out and thus switching context), not in the caller and certainly not in the frontend controller when it needs to decide whether a given email address is valid or not.

by annnnd 10y ago

systematical 10y ago

This is true, I couldn't figure out why this wasn't working, then realized I had to comment out this validation: filter_var($app->request()->post('email'), FILTER_VALIDATE_EMAIL)

thaumaturgy 10y ago

Misguided developers incorrectly "validating" email addresses is the reason that one of my clients couldn't use one of the newer TLDs, ".place", and ended up scrapping it and trying to find something they wanted a lot less in the much more crowded .com space. It's also why I far-too-often run into forms that won't let me use a "+" in the username part of my email address, which I use to track who's responsible for sending my email account off to third parties (e.g., "rob+paypal@...."). Some kinds of email validation are better than others. Using regular expressions and strictly adhering to the RFC is the one that developers are usually talking about when they say not to do it. filter_var(..., FILTER_VALIDATE_EMAIL) is sort of okay, although there are lots of edge cases that it doesn't handle correctly.

adjkant 10y ago
I somehow went through life not knowing the + trick. Thanks, and commenting to hopefully highlight this for anyone else who didn't know.
- thaumaturgy 10y ago
  The specific character depends on your MTA configuration. In Postfix this is the recipient_delimiter setting. I went with a "+" because that's the same character Gmail uses. Other mail services may not support this feature. Hope this helps!
danieltillett 10y ago
Sometimes not allowing a "+" is an anti-abuse feature. If you have a service tied to a customer providing a unique email address then you don't want them able to use a "+".
- jackweirdy 10y ago
  That seems like a strange place to verify that a user is only allowed to create one account. After all, they could just create a second email account on a free email provider and use that to sign up. To get around that, the service provider would have to verify identity further down the line, making the email format restriction redundant.
  - danieltillett 10y ago
    I have been down this route and you also have to disable the use of free email accounts. If the service value is not too high then even making people who want to abuse the service go through the process of registering a free email account works. Put a little bit of friction into the process and the script kiddies move onto an easier target.
    - iopq 10y ago
      > I have been down this route and you also have to disable the use of free email accounts. I don't have any other email. Seems like a really bad idea.

DCoder 10y ago

> When configured to use CLI sendmail, That's not correct. PHPMailer can be configured to send mail through raw SMTP, by directly invoking sendmail, or by calling PHP's mail() function (which is itself a wrapper around sendmail). This vulnerability affects only the last mode, when PHP's mail() calls sendmail. If you have PHPMailer configured to call sendmail directly, this vulnerability does not apply.

annnnd 10y ago
I stand corrected, thank you for clarifying.

neotek 10y ago

>I know it is tough to validate all addresses according to RFC Understatement of the year! If you were to try and actually validate according to the RFC, chances are you'll introduce new vectors of attack in your code simply because of the complexity of what you're trying to achieve. Honestly, the email address RFC is a litany of insane choices and kludged-together standards to account for the wild west free-for-all that existed before the modern internet emerged, and most of it has no bearing on the reality of how people use and create email addresses today. Better to just check if the address supplied has an @ in it somewhere and be done with it.

x0 10y ago
That's how I do it. Check for an @. Anything past that is sendmail's problem.
- nkozyra 10y ago
  Even though it false positives some valid emails, I've always felt requiring a @ and a . is a good check for public facing validation.
  - Piskvorrr 10y ago
    Technically valid e-mail addresses - but nonsensical ones. I haven't seen an e-mail without a domain cross a network in decades ("cross a network" excepts "user@localhost", and I'm not old enough to have seen any IP-adressed mails ;)). So yes, I believe that in 2016, a match on ^[^@]+@([^.@]+\.)+[^.@]+\.?$ does not have any actual false negatives, although allowing false positives. That's not entirely helpful in this CVE, though.

Synchro 10y ago

This is incorrect. Input sanitisation is not the problem here - PHPMailer sanitises and validates email addresses well enough. The problem is that a valid, sanitised email address can also be an attack in an inappropriate shell context. Much as an SQL injection attack string may be harmless in a shell context but lethal in SQL. Also the vuln does not apply to CLI sendmail, only when sending via the PHP mail() function, and it's at least partly due to bugs in PHP itself.

annnnd 10y ago
I am not blaming lack of input sanitization, the bug is clearly still there. What I am saying is that input sanitization would prevent this attack because it will only allow valid characters to get to your mailer, which will exclude spaces, tabs, double and single quotes. Thus I fail to see an exploit vector which would bypass proper input sanitization. Do you see it? In general I agree that PHP mail() function should take a part of blame here for not exposing a sane interface. Spammers have abused many bugs where programmers failed to sanitize "To:" or "From:" addresses and attackers could pass \r and \n characters (which allowed adding BCC fields, effectively sending e-mails to arbitrary addresses). > Also the vuln does not apply to CLI sendmail... Sure it does: >> ...will result in the followig list of arguments passed to sendmail program: EDIT: you probably meant to say that the vulnerability is only triggered when CLI sendmail is called via mail(), which is true. Configuring PHPMail to use sendmail via CLI directly apparently doesn't expose this vulnerability.
- pilif 10y ago
  > What I am saying is that input sanitization would prevent this attack because it will only allow valid characters to get to your mailer, you have to be careful with the meaning of "valid" though: What's valid in one context might not be in another context. Do you want to limit the character set to the least common denominator of non-special characters valid in all possible contexts your address might be used? What if you don't exclude some character because it's not special in any of the current contexts but then you later add another thing to the mix that uses in-band signalling? Now you need to update your whitelist and remove even more "invalid" characters. This won't end well for you. I would recommend you don't put a restriction on the input (aside of what the RFC defines as valid) and instead correctly escape (or throw if your context doesn't support escaping) when moving the raw data to a new context. In this case, I think the problem is in PHP's `mail()` function that put stuff in shell context without any escaping. The fix should be happing in `mail()` (which is internally shelling out and thus switching context), not in the caller and certainly not in the frontend controller when it needs to decide whether a given email address is valid or not.