Bug #74005 [Csd]: mail.add_x_header causes RFC-breaking lone line feed, loss of subsequent header

Edit report at https://bugs.php.net/bug.php?id=74005&edit=1 ID: 74005 Updated by: ab@php.net Reported by: andy_schmidt at HM-Software dot com Summary: mail.add_x_header causes RFC-breaking lone line feed, loss of subsequent header Status: Closed Type: Bug Package: Mail related Operating System: Windows PHP Version: 7.0.15 Assigned To: ab Block user comment: N Private report: N New Comment: For now, it's just the same behavior in both 5 and 7. This way we simply keep BC, and further improvements can base on this. But, as for me, there's no reason we should invest more into this. In any form currently, it is not RFC complaint, and the suggestion doesn't makes it such. Reading the header related sections https://www.heise.de/netze/rfc/rfcs/rfc5322.shtml#page-8 - either way the folding/unfolding is not and won't be supported by mail(). Either it enforces CRLF, or allows lone LF. IMHO, the PHP frameworks available are far better choice, as it would need much more to support the full RFC implementation). But also, the real world differences in MTAs are the big factor, as is to see from the previous issues. With the security concern - yes, a negligent programming could cause some issue. According to our current security classification - it is not a security issue. Otherwise in general - some program that accepts headers from the userspace is rather a very improbable or very special case, that needs an extra care anyway. For just accepting an email and to compose the header string the precaution is rather trivial. Thanks. Previous Comments: ------------------------------------------------------------------------ [2017-02-02 08:21:20] yohgaki@php.net Sorry, I thought my [2017-02-01 09:15 UTC] post was't went though. Anyway, line ending is mess... Some sendmail accepts "\n". We may better to have INI setting that normalize line ending chars. We shouldn't convert "\n","\r" to "\r\n", but we can convert "\r\n" to "\n" safely. mail.convert_crlf_to_lf = On/Off. Then accept only CR/LF line ending. This would resolve issues on almost all mail systems. It breaks existing code, though. ------------------------------------------------------------------------ [2017-02-02 01:46:24] yohgaki@php.net I should have noticed when I modify mail.c ext/standard/mail.c if (PG(mail_x_header)) { const char *tmp = zend_get_executed_filename(); zend_string *f; f = php_basename(tmp, strlen(tmp), NULL, 0); if (headers != NULL && *headers) { spprintf(&hdr, 0, "X-PHP-Originating-Script: " ZEND_LONG_FMT ":%s\n%s", php_getuid(), ZSTR_VAL(f), headers); } else { spprintf(&hdr, 0, "X-PHP-Originating-Script: " ZEND_LONG_FMT ":%s", php_getuid(), ZSTR_VAL(f)); } zend_string_release(f); } BTW, I don't like line ending conversions. Suppose user script validate invalid "\r\n" in mail header string, e.g. mail address, but forgot to check "\r" and/or "\n", then line ending conversion by PHP would allow mail header injection. Recent mail() is made to reject multiple or broken line ending in extra headers, but it still allows "\n" for compatibility purpose. Mixing line ending chars is mess. It seems we are better to deprecate string extra headers and force users to use array extra headers someday. And/Or disallow "\n" in header. We may better to disallow "\n" in 7.2. ------------------------------------------------------------------------ [2017-02-02 00:14:54] ab@php.net Many thanks for checking the snaps and actually investigating the cause of the issue :) The headers passed are now normalized internally same way they are in 5.x And of course, we merge fixes through all the branches, so it'll be in all the stable 7.x and later. Unfortunately too late for the upcoming finals, but those after them will get it. Thanks. ------------------------------------------------------------------------ [2017-02-01 22:40:15] andy_schmidt at HM-Software dot com Tested successfully with snapshot using the minimal test, even with mail.add_x_header ON (and also confirmed with virgin WordPress): $result = mail( "recipient@domain.com", "test subject", "test content", "X-PHP: ".phpversion()."\nFrom: sender@domain.com\n" ); Result now matches result from PHP 5: Date: Wed, 01 Feb 2017 17:19:53 -0500\r\n Subject: test subject\r\n To: recipient@domain.com\r\n X-PHP-Originating-Script: 0:andytest.php\r\n X-PHP: 7.0.17-dev\r\n From: sender@domain.com\r\n Thank you for taking care of that. I trust this will become part of 7.1 as well. ------------------------------------------------------------------------ [2017-02-01 18:24:43] ab@php.net Yeah, that's the exact point why it was done with CRLF and then reverted. The porting mistake in 7.x was fixed in the aforementioned commit, thus the status quo with 5.x should be now restored. Please fetch the latest http://windows.php.net/snapshots/ to ensure. Otherwise, the legacy code should not be touched in the stable branch, IMO. Furthermore, nowadays where things like Horde and other exist, significantly changing the legacy functions probably is not very big priority. Thanks. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=74005 -- Edit this bug report at https://bugs.php.net/bug.php?id=74005&edit=1