Bug #74005 [Com]: 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 Comment by: quentin dot lengele at gmail dot com 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: My bad, it seems the bug is somewhere else. Took me days to figures out. I'm sorry. Previous Comments: ------------------------------------------------------------------------ [2017-03-04 16:08:11] quentin dot lengele at gmail dot com Hi guys, I have similar problems on PhP 7.1.2 (nts x64 for windows). 550-5.7.1 not RFC 5322 compliant: 550-5.7.1 Multiple 'From' headers found. My server is Windows 2008R2 IIS 7.5, running PhP with FastCGI All was running OK with PhP 5.x Is this really fixed? ------------------------------------------------------------------------ [2017-02-02 12:50:17] ab@php.net 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. ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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