Repository navigation
mail() could provide better protection against mail header injections #13402
Description
Description
The mail() function has an optional fourth parameter that allows setting additional mail headers. This can either be a newline-separated string or structured as an associative array.
It creates an obvious risk of header injection if one adds possibly untrusted content to the headers, and this is also documented.
In the string case this is more or less unavoidable. I.e. if you have something like this which could be used in a contact form:
mail($to, $subject, $msg, "Reply-To: ".$_POST['email']);
If $_POST['email'] is not validated before, you can add something like "foo@example.com\r\nCc: hacker@example.com", injecting another recipient of that mail. This is, as said, unavoidable for this interface, and kinda expected.
However, in the array case, this still works, and this is more surprising, and could be avoided by PHP:
mail($to, $subject, $msg, ["Reply-To" => $_POST['email']]);
The same injection attack as above works here. (Update: slightly modified with '\n' instead of '\r\n', see comment below.) Given that each header line is its own entry in the array, PHP could avoid allowing a header injection by either not allowing newlines in a header entry or by filtering them out. According to the comments in the docs, it already does so for the to and subject parameter, it would be logical to do so for headers as well.
I think this is a simple change that would make the function more secure.
Activity
SakiTakamachi commentedon Feb 16, 2024
@hannob
Hi,
Does that mean code like this?
I tried this and also checked the implementation and it already doesn't allow line breaks. When I run this code I get an error like this:
hannob commentedon Feb 16, 2024
Oh, thanks for looking at it, it appears this is subtle than I first thought. You are correct that your example gets rejected:
However, this one works:
So while it does reject windows-style linebreaks (\r\n), it does not reject unix-style line breaks (\n). Given that this is passed to the sendmail wrapper, and sendmail command line tools tend to accept unix newlines, we have an injection again.
So probably the already existing code rejecting newlines should be changed to reject any kind of newline (i.e. ideally both '\r' and '\n') as an invalid character.
SakiTakamachi commentedon Feb 16, 2024
I see, although rfc2822 does not allow standalone use
\n, indeed there are certainly cases where it is accepted.Fixing this makes sense to me.
SakiTakamachi commentedon Mar 4, 2024
I forgot to write "fixes". It has been committed.