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.
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.
> 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.
I think you are taking my argument too far. Valid input doesn't mean it is safe to use in any context. It just means that anything which is invalid at the start shouldn't even have the chance to get in the system.
You should still properly escape data when passed to another context, no doubt about it.
I am also not suggesting "dirty" practices like replacing double quotes (Wordpress) or magically escaping quotes on input (PHP prior to... 5?). But if you know the input should be a number, allow only digits and '.' and clean everything else. And validate the range too! If you need a database ID, validate form and existence. If you need an e-mail address, run it through filter_var. Always. There is no reason not to.
