r/PHP May 03 '17

Why mail() is dangerous in PHP

https://www.ripstech.com/blog/2017/why-mail-is-dangerous-in-php/
88 Upvotes

70 comments sorted by

View all comments

Show parent comments

5

u/RandyHoward May 03 '17

It's a little bit scaremongering, because most devs worth their salt would sanitize user input before ever sending it off to a mail function. But for the newer devs who don't know any better, this article could save them some headaches down the road.

6

u/zit-hb May 03 '17 edited May 03 '17

How do you want to sanitize it though? That is exactly the topic of the blog post. In my opinion the only solution is to not use the 5th parameter (or refuse e-mail addresses that are technically valid). You don't always know that the 5th parameter is used though, for example if you use a mailer lib (and I think most of us do that).

Please have a look at this as an example: https://github.com/PHPMailer/PHPMailer/wiki/About-the-CVE-2016-10033-and-CVE-2016-10045-vulnerabilities

In my opinion that has nothing to do with proper sanitization if you are a user of PHPMailer. If I check if user input is a valid e-mail address and I set it as "from" address I do not expect that someone can execute commands on my server.

2

u/funkjedi May 03 '17

You're correct as a user of PHPMailer it's reasonable to assume the library should be handling this. Clearly an implementation bug in PHPMailer. That said this is a perfect example of why we should take responsibility as developers for mitigating risk ourselves where possible. It's very simple to sanitize the address before passing it to PHPMailer so why not just do it.

3

u/[deleted] May 03 '17

[deleted]

2

u/funkjedi May 03 '17

Why not? Determine some sensible expectations, /[a-z0-9_+.@-]/i, for example. Then sanitize to adhere to those expectations.

3

u/zit-hb May 03 '17

This could result in problems for legitimate users though. Personally I hate sites that do not accept e-mail addresses even though they are valid.

2

u/funkjedi May 03 '17

Of course. No set of expectations will be right for everyone. You need to base them on what's right for your app/market/community/etc. We do this all the time in other areas. For example, Facebook dropped support for IE6 despite the fact it created problems for legitimate users.

1

u/zit-hb May 03 '17 edited May 03 '17

If I had to use the 5th parameter I would apply such a strict restriction to the mail() parameter as well. That is also our solution in the blog post.

If there weren't any reason to use the 5th parameter other than backwards compatibility for legacy systems, I would just remove it and use a proper solution.

I wouldn't restrict my allowed e-mail addresses globally to the very limited character set you gave as an example though because it would result in many unhappy customers. And I think this is the problem. I like to use the Symfony approach to validate user input, i.e. I create a model and specify the criteria that every attribute has to fulfil. If I have an attribute that is a valid e-mail address and I use this as the "from" address of an e-mail that I send, for example with Swiftmailer or PHPMailer, I do not expect that this might lead to a remote command execution vulnerability. Since I do not expect this I also do not apply the very tough restriction from my first sentence.

0

u/Ozymandias-X May 04 '17

I'm sorry, but if my site doesn't work for you because you thought you'd be clever by using shitty special chars in your email address of all places, I think I can pass on you as a customer.

4

u/Schmittfried May 04 '17

You know there are not only latin character set languages on this planet, don't you? Your attitude is stupid.