Skip to content

Fixed bug #68776 #1273

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Fixed bug #68776 #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec6a881
Fixed bug #68776
May 13, 2015
8fd3c7b
Remove multiply applied patch
May 13, 2015
fc0266b
Remove assumption that php_trim() is applied, but php_trim() code is …
Jun 8, 2015
89def69
Fixed leading newline detection. Add cases for readling/trailing newl…
Jun 8, 2015
500b70d
Simplify and better standard confirmance
Jun 8, 2015
bbdb669
Fixed possible over read and consolidate trailing newline check
Jun 9, 2015
f46fdd7
Add one more null char check.
Jun 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ext/standard/mail.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,44 @@ void php_mail_log_to_file(char *filename, char *message, size_t message_size TSR
}


static int php_mail_detect_multiple_crlf(char *hdr) {
/* This function detects multiple/malformed multiple newlines. */
size_t len;

if (!hdr) {
return 0;
}

/* Should not have any newlines at the beginning. */
/* RFC 2822 2.2. Header Fields */
if (*hdr < 33 || *hdr > 126 || *hdr == ':') {
return 1;
}

while(*hdr) {
if (*hdr == '\r') {
if (*(hdr+1) == '\0' || *(hdr+1) == '\r' || (*(hdr+1) == '\n' && (*(hdr+2) == '\0' || *(hdr+2) == '\n' || *(hdr+2) == '\r'))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/* Malformed or multiple newlines. */
return 1;
} else {
hdr += 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ' '.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
} else if (*hdr == '\n') {
if (*(hdr+1) == '\0' || *(hdr+1) == '\r' || *(hdr+1) == '\n') {
/* Malformed or multiple newlines. */
return 1;
} else {
hdr += 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - what if the string ends with \n?

}
} else {
hdr++;
}
}

return 0;
}


/* {{{ php_mail
*/
PHPAPI int php_mail(char *to, char *subject, char *message, char *headers, char *extra_cmd TSRMLS_DC)
Expand Down Expand Up @@ -266,6 +304,7 @@ PHPAPI int php_mail(char *to, char *subject, char *message, char *headers, char

efree(tmp);
}

if (PG(mail_x_header)) {
const char *tmp = zend_get_executed_filename(TSRMLS_C);
char *f;
Expand All @@ -281,6 +320,11 @@ PHPAPI int php_mail(char *to, char *subject, char *message, char *headers, char
efree(f);
}

if (hdr && php_mail_detect_multiple_crlf(hdr)) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Multiple or malformed newlines found in additional_header");
MAIL_RET(0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

        $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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (!sendmail_path) {
#if (defined PHP_WIN32 || defined NETWARE)
/* handle old style win smtp sending */
Expand Down
329 changes: 329 additions & 0 deletions ext/standard/tests/mail/mail_basic6.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,329 @@
--TEST--
Test mail() function : basic functionality
--INI--
sendmail_path=tee mailBasic.out >/dev/null
mail.add_x_header = Off
--SKIPIF--
<?php
if(substr(PHP_OS, 0, 3) == "WIN")
die("skip Won't run on Windows");
?>
--FILE--
<?php
/* Prototype : int mail(string to, string subject, string message [, string additional_headers [, string additional_parameters]])
* Description: Send an email message with invalid addtional_headers
* Source code: ext/standard/mail.c
* Alias to functions:
*/

echo "*** Testing mail() : basic functionality ***\n";


// Valid header
$to = 'user@example.com';
$subject = 'Test Subject';
$message = 'A Message';
$additional_headers = "HEAD1: a\r\nHEAD2: b\r\n";
$outFile = "mailBasic.out";
@unlink($outFile);

echo "-- Valid Header --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo file_get_contents($outFile);
unlink($outFile);

// Valid header
$additional_headers = "HEAD1: a\nHEAD2: b\n";
@unlink($outFile);

echo "-- Valid Header --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Valid header
// \r is accepted as valid. This may be changed to invalid.
$additional_headers = "HEAD1: a\rHEAD2: b\r";
@unlink($outFile);

echo "-- Valid Header --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

//===============================================================================
// Invalid header
$additional_headers = "\nHEAD1: a\nHEAD2: b\n";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "\rHEAD1: a\nHEAD2: b\r";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "\r\nHEAD1: a\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "\r\n\r\nHEAD1: a\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "\n\nHEAD1: a\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "\r\rHEAD1: a\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - preceeding newline--\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\r\n\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\r\n\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\n\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\r\rHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\n\rHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
$additional_headers = "HEAD1: a\n\r\nHEAD2: b\r\n";
@unlink($outFile);

echo "-- Invalid Header - multiple newlines in the middle --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
// Invalid, but PHP_FUNCTION(mail) trims newlines
$additional_headers = "HEAD1: a\r\nHEAD2: b\r\n\n";
@unlink($outFile);

echo "-- Invalid Header - trailing newlines --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
// Invalid, but PHP_FUNCTION(mail) trims newlines
$additional_headers = "HEAD1: a\r\nHEAD2: b\n\n";
@unlink($outFile);

echo "-- Invalid Header - trailing newlines --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
// Invalid, but PHP_FUNCTION(mail) trims newlines
$additional_headers = "HEAD1: a\r\nHEAD2: b\n";
@unlink($outFile);

echo "-- Invalid Header - trailing newlines --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

// Invalid header
// Invalid, but PHP_FUNCTION(mail) trims newlines
$additional_headers = "HEAD1: a\r\nHEAD2: b\r";
@unlink($outFile);

echo "-- Invalid Header - trailing newlines --\n";
// Calling mail() with all additional headers
var_dump( mail($to, $subject, $message, $additional_headers) );
echo @file_get_contents($outFile);
@unlink($outFile);

?>
===DONE===
--EXPECTF--
*** Testing mail() : basic functionality ***
-- Valid Header --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
-- Valid Header --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
-- Valid Header --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: aHEAD2: b

A Message
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - preceeding newline--

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - multiple newlines in the middle --

Warning: mail(): Multiple or malformed newlines found in additional_header in %s/mail_basic6.php on line %d
bool(false)
-- Invalid Header - trailing newlines --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
-- Invalid Header - trailing newlines --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
-- Invalid Header - trailing newlines --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
-- Invalid Header - trailing newlines --
bool(true)
To: user@example.com
Subject: Test Subject
HEAD1: a
HEAD2: b

A Message
===DONE===
Expand Down