Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH-8086: Introduce mail.mixed_lf_and_crlf INI #10191

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Dec 30, 2022

When this INI option is enabled, it reverts the line separator for headers and message to LF which was a non conformant behavior in PHP 7. It is done because some non conformant MTAs fail to parse CRLF line separator for headers and body.

This targets PHP 8.2 even though as for some it might seem more like a feature because it requires new INI. On the other side this might have been omission when the bug fix was introduced in PHP 8.0 as it wasn't realized that there are many non conformant MTAs. Also seems like this is an issue for many users.

@Girgias
Copy link
Member

Girgias commented Dec 30, 2022

Seems sensible, just as a reminder to update the UPGRADING document and open an issue on doc-en (or PR) to document this.

@cmb69
Copy link
Member

cmb69 commented Dec 30, 2022

We probably also want to counter-act #7907, i.e. use the ini setting and a variable instead of the CRLF macro there (sorry, I've missed that earlier).

And while I'm not strictly against this change, we need to be very careful wrt. security (especially if we want to target PHP-8.1 or PHP-8.2). There are header injection checks which may only assume CRLF (e.g. 37962c6).

When this INI option is enabled, it reverts the line separator for
headers and message to LF which was a non conformant behavior in PHP 7.
It is done because some non conformant MTAs fail to parse CRLF line
separator for headers and body.

This is used for mail and mb_send_mail functions.
@bukka
Copy link
Member Author

bukka commented Dec 30, 2022

We probably also want to counter-act #7907, i.e. use the ini setting and a variable instead of the CRLF macro there (sorry, I've missed that earlier).

Makes sense - done.

And while I'm not strictly against this change, we need to be very careful wrt. security (especially if we want to target PHP-8.1 or PHP-8.2). There are header injection checks which may only assume CRLF (e.g. 37962c6).

I don't really plan to change imap as it is for people who should understand MTAs and can probably handle this just fine. Unless I'm missing something and the current change already impacts it somehow?

@cmb69
Copy link
Member

cmb69 commented Dec 30, 2022

I don't really plan to change imap as it is for people who should understand MTAs and can probably handle this just fine. Unless I'm missing something and the current change already impacts it somehow?

I guess that imap stuff was a bad example. I'm not really concerned about possibly having some cases where CRLF may slip through (we could still improve if necessary), but rather that there might be internal handling which would not properly detect header injection due to LF only. From a quick look, it seems that mail() handles this fine.

Regarding which branch to target: @ramsey, @patrickallaert, what do you think about targeting PHP-8.1?

@bukka
Copy link
Member Author

bukka commented Dec 30, 2022

I chose PHP-8.2 as a base branch because I thought that it might get confusing if INI is available when added to some later version in 8.1. But if others and mainly RM's think that 8.1 is fine I would be cool with that ofc.

@alexdowad
Copy link
Contributor

Is this likely to land soon?

@alexdowad
Copy link
Contributor

I am just working on mb_send_mail, and if I land my changes first, this patch won't apply and will have to be rewritten. If @bukka can land this change soon, I don't mind waiting; but otherwise, I will go ahead.

@bukka
Copy link
Member Author

bukka commented Jan 18, 2023

I plan to merge it tomorrow to 8.2+.

@alexdowad
Copy link
Contributor

OK, thanks.

I don't think I have made any conflicting changes on master.

@bukka
Copy link
Member Author

bukka commented Jan 19, 2023

Merged in cc931af

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

Successfully merging this pull request may close these issues.

None yet

4 participants