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: yohgaki@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: 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. Previous Comments: ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ [2017-02-01 15:04:58] andy_schmidt at HM-Software dot com I probably wouldn't backport this to PHP 5, because in PHP 5 you were still fixing lone line feeds in the win32 implementation. So the bad X-PHP... header had no ill effect. Under PHP 7 there is need to action because now the headers are "passed through" under win32 - so they HAVE to be SMTP compliant. I have reviewed the earlier patch you cited: https://bugs.php.net/bug.php?id=48620 It shows the original code to have been: spprintf(&hdr, 0, "%s\r\nX-PHP-Originating-Script: %ld:%s\n", headers, php_getuid(), f); so it unconditionally bracketed the X-PHP header with a leading CRLF but trailing lone LF!? This was REALLY bad code - because it was following NEITHER convention. With an old Linux mailer, the leading CRLF would be treated as two line feeds. It also caused the other reported problem if there were no user supplied headers in which case the (sole) X-PHP header would start with the CRLF (essentially creating an empty header)! The attempted fix only sought to address the second scenario by NOT starting with a CRLF if a previous header existed. It still kept the inconsistency of a leading CRLF but a trailing lone LF. Your "legacy" challenge is reconciling two directly opposing requirements: - mail() under Linux was talking to a mailer, some of which had required the input to be operating system line end (single line feed) and rejected/mangled any SMTP compliant CR/LF. - mail() under win32 on the other hand is talking directly to SMTP, which absolutely mandates RFC compliant line-ends. Before PHP 7 you had taken the logical/pragmatic step of simply adapting the win32 implementation to fix any lone LF (or CR or LFCR?) to CRLF (which is a simple RegEx and kept everyone happy). If you no longer want to do that for PHP 7, then you would need to reject non-RFC compliant additionalheaders (including your own broken X-PHP... header!). But, I suspect you'd effect a lot of existing applications that relied on single line feeds. Alternatively you would have to reinstate the PHP 5 behavior for win 32 thus once again aligning the behavior for both operating system families until you are ready to complete change the additionalheaders from "string" to "array". ------------------------------------------------------------------------ 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