Fixed bug #68776 #1273

yohgaki · 2015-05-13T00:21:18Z

https://bugs.php.net/bug.php?id=68776

smalyshev · 2015-06-01T01:10:57Z

ext/standard/mail.c

This may be dangerous if the *hdr string ends on \r.

Right. I'm being sloppy here by assuming caller trimmed leading/ending \r, \n.
I may get rid of this assumption and previously applied trim code.

smalyshev · 2015-06-01T02:09:48Z

Please try to build on TSRM, this patch doesn't even build there.

smalyshev · 2015-06-04T21:24:00Z

@yohgaki If we want to have it in next 5.4 release, we don't have too much time as it is planned for the next week.

…not removed for better compatibility. Add missing TSRMLS_CC.

…ines

yohgaki · 2015-06-08T03:24:25Z

Sorry for the delay.
There is silly typo in commit message. I guess new merge will show it :(

smalyshev · 2015-06-08T07:06:06Z

ext/standard/mail.c

It looks like this still can hop over \0 if *hdr ends with \r\0.

Thank you. Reasonable anxiety.
There is NULL byte check before this function call in PHP_FUNCTION(mail),

if (headers) { MAIL_ASCIIZ_CHECK(headers, headers_len); headers_trimmed = php_trim(headers, headers_len, NULL, 0, NULL, 2 TSRMLS_CC); }

but I agree that it would be better to detect such case also.

Since php_mail() does not get string length, null in hdr cannot be detected because we cannot change php_mail() signature. Do you have suggestion for the time being? I think current code is OK for released versions, but we may change API for PHP7.

How about use this patch for released version?
I'll prepare another patch to clean up mail mess, including mb_send_mail, for master.
IMHO, we are better to raise error when user supply strings contain NULL char rather than replacing NULL to ' '.

PHP_FUNCTION(mail) is not enough, as php_mail can be called from other code (it's PHPAPI) so assuming the string passed to it never ends in \r, without even documenting it, is not a good idea. And it string ends in \r, then hdr+=2 hops right over end of the string and proceeds to read into unallocated memory (or allocated for other things, even worse). The check should be added so that it never reads past the end of the string.

Right, but current API does not pass the length to php_mail().
Nothing we can do without modifying API.
That's the reason why I propose updating API and fix the mess. (NULL byte check/replacement and invalid newlines)

yohgaki · 2015-06-09T08:41:28Z

@smalyshev Thank you for catching pointer error. And I was lazy for trailing newline conditions because PHP_FUNCTION(mail)/PHP_FUNCTION(mb_send_mail) applies php_trim(). This one is not.

I agree php_mail() should be as secure as PHP_FUNCTION(mail/mb_send_mail), but it does NULL char check/replace in them. php_trim() should be removed also. php_mail() does not have string length now. I would like to address this issue by using zend_string in PHP7 and leave as it is now for released versions. What do you think?

smalyshev · 2015-06-10T04:41:51Z

merged

yohgaki · 2015-06-11T00:33:48Z

@smalyshev
Thanks.
I've opened new bug for mail()
https://bugs.php.net/bug.php?id=69791
I'm going to add array support for additional headers. If you have comments, I appreciate it.

M66B · 2015-06-12T08:08:43Z

ext/standard/mail.c

This breaks sending multipart attachments, which need a double CR LF

@M66B Could you provide a test case which is broken? /cc @yohgaki

$headers .= '--' . $separator . $eol; $headers .= "Content-Type: application/pdf; name=\"" . $filename . "\"" . $eol; $headers .= 'Content-Transfer-Encoding: base64' . $eol; $headers .= "Content-Disposition: attachment; filename=\"" . $filename . "\"" . $eol . $eol; $headers .= chunk_split(base64_encode($invoicePDF)); $headers .= '--' . $separator . '--';

Change

$headers .= "Content-Disposition: attachment; filename=\"" . $filename . "\"" . $eol . $eol;

into

$headers .= "Content-Disposition: attachment; filename=\"" . $filename . "\"" . $eol;

and you end up with an empty attachment.

Tested with mandrill

Complete code:

$eol = "\n"; // Main headers $headers = 'From: ' . $data['ap_name'] . ' <' . $data['ap_email'] . '>' . $eol; $headers .= 'Reply-To: ' . $data['ap_name'] . ' <' . $data['ap_email'] . '>' . $eol; $headers .= 'MIME-Version: 1.0' . $eol; $headers .= "Content-Type: multipart/mixed; boundary=\"" . $separator . "\"" . $eol; $headers .= 'Content-Transfer-Encoding: 8bit' . $eol . $eol; $headers .= $subject . $eol; // Message headers $headers .= "--" . $separator . $eol; $headers .= "Content-Type: text/html; charset=UTF-8" . $eol; $headers .= 'Content-Transfer-Encoding: 8bit' . $eol . $eol; $headers .= $message . $eol; // Attachment headers $headers .= '--' . $separator . $eol; $headers .= "Content-Type: application/pdf; name=\"" . $filename . "\"" . $eol; $headers .= 'Content-Transfer-Encoding: base64' . $eol; $headers .= "Content-Disposition: attachment; filename=\"" . $filename . "\"" . $eol . $eol; $headers .= chunk_split(base64_encode($invoicePDF)); $headers .= '--' . $separator . '--'; // Send if (!mail($data['email'], $subject, $message, $headers)) throw new Exception('e-mail failed, invoice=' . $invoice);

Note that this is a serious bug, which could have catastrophic consequences for PHP users.

The message body (text) should maybe in the message body (parameter), but you cannot send a message (text) with one or more attachments in this way. An attachment needs to specify a content type and other things, which can be done using headers only. So, back to the problem, Mandrill is widely used to send messages in a reliable way and this change breaks sending attachments using Mandrill and I couldn't find a workaround for this. How can this be fixed?

@M66B I'm not sure I understand you. You can put necessary headers in the header field, and the body (including multi-part message, which included necessary headers for MIME parts) into the body, I see absolutely no problem in that. I have no idea why Mandrill puts whole body of the message in the headers, but if it does so, it's time for it to be fixed and use the proper way of doing it. As I said, the workaround is putting the headers in headers and body in body. I don't see anything preventing it.

Supposed we have a message like this:

To: you From: me Subject: text Please find my thoughts in the attachment [[attachment.pdf]]]

The text Please find my thoughts in the attachment should go into the message parameter and attachment.pdf should go into the headers.

http://php.net/manual/en/function.mail.php

message Message to be sent. Each line should be separated with a CRLF (\r\n). Lines should not be larger than 70 characters.

and attachment.pdf should go into the headers.

No it should not. Both should be part of multipart MIME message that goes in the body of the message. I am sorry, but I get a suspicion that you are not completely familiar with MIME message format and how headers and body of the multipart message work. In this case, I would advise inspecting some of the multipart messages as the source (most mail clients have this option) and reading about MIME. If I am mistaken, than I must still misunderstand you as I have no idea why you think attachments go into headers. Could you explain me what is the basis of such claim?

Am I understanding correctly that attachment headers should go into the message (body) parameter?

It is not about claiming something, but this is not (clearly) documented:
http://php.net/manual/en/function.mail.php

Looking around for examples I see both methods (using the message and the headers parameters for attachments) being used.

In any case if you are right I see a workaround, but I still believe this change shouldn't break things.

smalyshev · 2015-06-13T20:57:45Z

@M66B I'm not sure I understand why you need double EOL after the Content-Transfer-Encoding header. I've just checked sending multi-attachment email and I see that there is no double EOL in the headers. Header and body are separated by multiple EOL but inside headers there's no extra EOLs. Also, there are two EOLs separating header from body within each part of multipart message, but multipart messages are not part of the header, they are part of the body. I'm not sure why you're putting attachment headers and whole $message into headers, it doesn't sound right - maybe I miss something in your code.

Could you provide a full stand-alone script that worked before and doesn't work now? The produced mail source would also be helpful. I'd also advise submitting bug to bugs.php.net - it's much more convenient to track it there.

M66B · 2015-06-14T05:26:28Z

Leaving out the double EOL when sending an attachment using Mandril will result in an empty attachment. I spent an hour trying to workaround this, but I couldn't find any way. Don't ask me why an EOL is necessary, but it simply is. I have reverted back to PHP 5.6.9 to solve this problem for an unhappy client. Just try the script I have already provided with Mandril and you'll see. Note that Mandril is quite often used to send mail from servers.

smalyshev · 2015-06-15T00:14:58Z

@M66B I suspect that you are trying to send malformed messages, putting message body into header, and by some miracle it worked before. However, it does not seem to be the right thing to do. I'm not sure what "Mandril" is but if a stand-alone script is provided I could look deeper into this. I still suggest to submit a bug on bugs.php.net.

rmccullagh · 2015-06-15T02:33:42Z

ext/standard/mail.c

You've got a potential buffer overflow here. (*hdr+2) could very well go past the buffer and not equal \0

I assume this relies on the fact that strings are implicitly 0-terminated in PHP. But if you have counter-example, please submit the code.

HonorMan · 2015-06-21T19:47:47Z

Hello,
Thank you for looking into this very serious issue. Several of us would like specific direction on how to fix this issue. Upgrading to 5.6.10 does not seem to fix the scripting errors we are facing. we are still getting "Multiple or malformed newlines found in additional_header" errors and cannot send attachments.

$headers .= "Reply-to: " . $name . " <" . $Email . ">" .$eol;
$headers .= "MIME-Version: 1.0".$eol;
$headers .= "Content-Type: multipart/mixed; boundary=\"".$separator."\"".$eol.$eol;
$headers .= "Content-Transfer-Encoding: 7bit".$eol;
$headers .= "This is a MIME encoded message.".$eol.$eol;

// To send HTML mail, the Content-type header must be set
$headers .= "--".$separator.$eol;
$headers .= "Content-Type: text/html; charset=\"UTF-8\"".$eol;
$headers .= "Content-Transfer-Encoding: 8bit".$eol.$eol;
//$headers .= "To:  <" . $Email . ">" . "\r\n";
//$headers .= "From: ourserver@ourserver.com>" . "\r\n";   
$headers .= $messageAll.$eol.$eol;

// attachment
$headers .= "--".$separator.$eol;
$headers .= "Content-Type: application/octet-stream; name=\"".$filename."\"".$eol;
$headers .= "Content-Transfer-Encoding: base64".$eol;
$headers .= "Content-Disposition: attachment".$eol.$eol;
$headers .= $attachment.$eol.$eol;
$headers .= "--".$separator."--";

Has worked for years. Now broken.

smalyshev · 2015-06-21T19:49:26Z

@HonorMan I would recommend putting message in the body of the message, not in the header.

HonorMan · 2015-06-22T22:43:40Z

Thank you I misspoke. Emails with attachments are my issue. Cannot seem to send messages with attachments and it is not clear where to separate the message from the header when using attachments. Any examples?

HonorMan · 2015-06-22T23:04:20Z

/**************************************************************************
* Build complete email *
**************************************************************************/

$messageAll = '<table style="background-color: #fff; table-layout:fixed; overflow: hidden; word-wrap: break-word;" cellpadding="0" cellspacing="0" border="0" width="600px" >';
$messageAll .= '<tr><td>' . $message . '</td></tr>';
$messageAll .= '<tr style="background-color: #FFF;"><td align="left">' . $closing . '</td></tr>';
$messageAll .= '<tr><td align="center">' . $footer . '</td></tr></table><br /><br />';

/*************************************************************************
* Send Email                                                             *   
*************************************************************************/

$attachment = chunk_split(base64_encode($pdfdoc)); // encode data (puts attachment in proper format)
$separator = 'Commented out for forum purpose'
$eol = PHP_EOL; // carriage return type (we use a PHP end of line constant)
$filename = "estimate.pdf"; // attachment name  


$to  = $mail;
$from = $name .  "<noreply@server.com>" ;

$subject = $cName . " Estimate";

// main header (multipart mandatory)
$headers = "From: ".$from.$eol;
$headers .= "Cc: " . $name . " <" . $coachEmail . ">" .$eol;
$headers .= "Reply-to: " . $name . " <" . $cEmail . ">" .$eol;
$headers .= "MIME-Version: 1.0".$eol;
$headers .= "Content-Type: multipart/mixed; boundary=\"".$separator."\"".$eol.$eol;
$headers .= "Content-Transfer-Encoding: 7bit".$eol;
$headers .= "This is a MIME encoded message.".$eol.$eol;

// To send HTML mail, the Content-type header must be set
$headers .= "--".$separator.$eol;
$headers .= "Content-Type: text/html; charset=\"UTF-8\"".$eol;
$headers .= "Content-Transfer-Encoding: 8bit".$eol.$eol;
//$headers .= "To:  <" . $cEmail . ">" . "\r\n";
//$headers .= "From: server name<invoice@servername.com>" . "\r\n";   
//$headers .= $messageAll.$eol.$eol; // old code that no longer works

// attachment
$headers .= "--".$separator.$eol;
$headers .= "Content-Type: application/octet-stream; name=\"".$filename."\"".$eol;
$headers .= "Content-Transfer-Encoding: base64".$eol;
$headers .= "Content-Disposition: attachment".$eol.$eol;
$headers .= $attachment.$eol.$eol;
$headers .= "--".$separator."--";

if (mail($to, $subject, $messageAll, $headers)){ // blah blah blah after this

HonorMan · 2015-06-26T14:33:25Z

I figured it out... I am glad this happened because we definitely had our header's set up wrong based on some examples we saw out there. Here is what we did to fix our problem. It was quite simple when we did it right.

/**************************************************************************

Build complete email *

**************************************************************************/

$messageAll = '

';
$messageAll .= '';
$messageAll .= '';
$messageAll .= '

' . $message . '
' . $closing . '
' . $footer . '

';

/*************************************************************************

Send Email *
*************************************************************************/

$attachment = chunk_split(base64_encode($pdfdoc)); // encode data (puts attachment in proper format)
$separator = 'Commented out for forum purpose'
$eol = PHP_EOL; // carriage return type (we use a PHP end of line constant)
$filename = "estimate.pdf"; // attachment name

$mailbody = "";

$to = $mail;
$from = $name . "noreply@server.com" ;

$subject = $cName . " Estimate";

// main header (multipart mandatory)
$headers = "From: ".$from . $eol;
$headers .= "Cc: " . $name . " <" . $coachEmail . ">" . $eol;
$headers .= "Reply-to: " . $name . " <" . $cEmail . ">" . $eol;
$headers .= "MIME-Version: 1.0" . $eol;

// We removed the extra $eol from the next statement.

$mailbody = "Content-Type: multipart/mixed; boundary="" . $separator . """ . $eol;
$mailbody .= "Content-Transfer-Encoding: 7bit".$eol;
$mailbody .= "This is a MIME encoded message.".$eol.$eol;

$mailbody .= "--".$separator.$eol;
$mailbody .= "Content-Type: text/html; charset="UTF-8"".$eol;
$mailbody .= "Content-Transfer-Encoding: 8bit".$eol.$eol;
$mailbody .= $messageAll . $eol; // Added our formatted message to body

// attachment
$mailbody .= $eol . "--" . $separator.$eol;
$mailbody .= "Content-Type: application/octet-stream; name="" . $filename . """ . $eol;
$mailbody .= "Content-Transfer-Encoding: base64" . $eol;
$mailbody .= "Content-Disposition: attachment" . $eol . $eol;
$mailbody .= $attachment . $eol . $eol;
$mailbody .= "--" . $separator . "--";

if (mail($to, $subject, $$mailbody, $headers)){ // blah blah blah after this

Jimbo99 · 2015-07-02T10:11:45Z

HonorMan - Just used your code above and works great! But to work for me I needed to add one line to your code, at least with Outlook Express 2007 as the target email client:

At the end of your "main header (multipart mandatory)" section I had to add this line
$headers .= "Content-Type: multipart/mixed; boundary="" . $separator . """ . $eol;

Without that, Outlook Express displays the mime encoding of the message in the message body, not as an attachment. It may be particular to that mail client. Posting here in case it helps anyone else.

HonorMan · 2015-07-10T20:52:31Z

That works too! We had that statement in the body and it works with Outlook.

simplified our headers.

$mailbody = "Content-Type: multipart/mixed; boundary="" . $separator . """ . $eol;

HonorMan · 2015-07-10T20:57:05Z

Sorry read too fast. Outlook Express may need it in the header.

HonorMan · 2015-07-10T21:00:18Z

Seems like there are known issues: https://support.microsoft.com/en-us/kb/833004

Yasuo Ohgaki added 2 commits May 13, 2015 09:18

Fixed bug #68776

ec6a881

Remove multiply applied patch

8fd3c7b

jpauli added the Bug label May 13, 2015

smalyshev reviewed Jun 1, 2015
View reviewed changes

Yasuo Ohgaki added 2 commits June 8, 2015 11:52

Remove assumption that php_trim() is applied, but php_trim() code is …

fc0266b

…not removed for better compatibility. Add missing TSRMLS_CC.

Fixed leading newline detection. Add cases for readling/trailing newl…

89def69

…ines

Simplify and better standard confirmance

500b70d

smalyshev reviewed Jun 8, 2015
View reviewed changes

Yasuo Ohgaki added 2 commits June 9, 2015 17:28

Fixed possible over read and consolidate trailing newline check

bbdb669

Add one more null char check.

f46fdd7

smalyshev closed this Jun 10, 2015

M66B reviewed Jun 12, 2015
View reviewed changes

s0ph1e mentioned this pull request Jun 13, 2015

Remove excess variable in mail.c #1340

Closed

rmccullagh reviewed Jun 15, 2015
View reviewed changes

Fixed bug #68776 #1273

Fixed bug #68776 #1273

Uh oh!

Conversation

yohgaki commented May 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smalyshev commented Jun 1, 2015

Uh oh!

smalyshev commented Jun 4, 2015

Uh oh!

yohgaki commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yohgaki commented Jun 9, 2015

Uh oh!

smalyshev commented Jun 10, 2015

Uh oh!

yohgaki commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smalyshev commented Jun 13, 2015

Uh oh!

M66B commented Jun 14, 2015

Uh oh!

smalyshev commented Jun 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HonorMan commented Jun 21, 2015

Uh oh!

smalyshev commented Jun 21, 2015

Uh oh!

HonorMan commented Jun 22, 2015

Uh oh!

HonorMan commented Jun 22, 2015

Uh oh!

HonorMan commented Jun 26, 2015

Uh oh!

Jimbo99 commented Jul 2, 2015

Uh oh!

HonorMan commented Jul 10, 2015

Uh oh!

HonorMan commented Jul 10, 2015

Uh oh!

HonorMan commented Jul 10, 2015

Uh oh!