From 27d3632a4d26111b36d3fc1b9c10599981b13b39 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 26 Feb 2021 16:11:59 +0100 Subject: [PATCH 1/5] Fix #80751: Comma in recipient name breaks email delivery So far, `SendText()` simply separates potential email address lists at any comma, disregarding that commas inside a quoted-string do not delimit addresses. We fix that by introducing an own variant of `strtok_r()` which caters to quoted-strings. We also make `FormatEmailAddress()` aware of quoted strings. We do not cater to email address comments, and potentially other quirks of RFC 5322 email addresses, but catering to quoted-strings is supposed to solve almost all practical use cases. --- ext/standard/tests/mail/bug80751.phpt | 93 +++++++++++++++++++++++++++ win32/sendmail.c | 84 +++++++++++++++++++----- 2 files changed, 161 insertions(+), 16 deletions(-) create mode 100644 ext/standard/tests/mail/bug80751.phpt diff --git a/ext/standard/tests/mail/bug80751.phpt b/ext/standard/tests/mail/bug80751.phpt new file mode 100644 index 0000000000000..0b1519c9611f2 --- /dev/null +++ b/ext/standard/tests/mail/bug80751.phpt @@ -0,0 +1,93 @@ +--TEST-- +Bug #80751 (Comma in recipient name breaks email delivery) +--SKIPIF-- + +--INI-- +SMTP=localhost +smtp_port=25 +--FILE-- + 0) { + // sleep for a while to allow msg to be delivered + sleep(1); + + $num_messages = imap_check($imap_stream)->Nmsgs; + for ($i = $num_messages; $i > 0; $i--) { + $info = imap_headerinfo($imap_stream, $i); + if ($info->subject === $subject) { + $header = imap_fetchheader($imap_stream, $i); + echo "Return-Path header found: "; + var_dump(strpos($header, 'Return-Path: joe@example.com') !== false); + echo "To header found: "; + var_dump(strpos($header, 'To: "" ') !== false); + echo "From header found: "; + var_dump(strpos($header, 'From: "" ') !== false); + echo "Cc header found: "; + var_dump(strpos($header, 'Cc: "Lastname, Firstname" ') !== false); + imap_delete($imap_stream, $i); + $found = true; + break; + } + } + $repeat_count--; + } + + imap_close($imap_stream, CL_EXPUNGE); + return $found; +} + +$to = "\"\" <{$users[1]}@$domain>"; +$subject = bin2hex(random_bytes(16)); +$message = 'hello'; +$headers = "From: \"\" \r\n" + . "Cc: \"Lastname, Firstname\" <{$users[2]}@$domain>\r\n" + . "Bcc: \"Firstname \\\"Ni,ck\\\" Lastname\" <{$users[3]}@$domain>\r\n"; + +$res = mail($to, $subject, $message, $headers); +if ($res !== true) { + die("TEST FAILED : Unable to send test email\n"); +} else { + echo "Message sent OK\n"; +} + +foreach ([$users[1], $users[2], $users[3]] as $user) { + if (!find_and_delete_message("$user@$domain", $subject)) { + echo "TEST FAILED: email not delivered\n"; + } else { + echo "TEST PASSED: Message sent and deleted OK\n"; + } +} +?> +--EXPECT-- +Message sent OK +Return-Path header found: bool(true) +To header found: bool(true) +From header found: bool(true) +Cc header found: bool(true) +TEST PASSED: Message sent and deleted OK +Return-Path header found: bool(true) +To header found: bool(true) +From header found: bool(true) +Cc header found: bool(true) +TEST PASSED: Message sent and deleted OK +Return-Path header found: bool(true) +To header found: bool(true) +From header found: bool(true) +Cc header found: bool(true) +TEST PASSED: Message sent and deleted OK diff --git a/win32/sendmail.c b/win32/sendmail.c index 78409b53a8b3b..373e82b4674b9 100644 --- a/win32/sendmail.c +++ b/win32/sendmail.c @@ -333,6 +333,31 @@ PHPAPI char *GetSMErrorText(int index) } } +/* strtok_r like, but recognizes quoted-strings */ +static char *find_address(char *list, char **state) +{ + zend_bool in_quotes = 0; + char *p = list; + + if (list == NULL) { + if (*state == NULL) { + return NULL; + } + p = list = *state; + } + *state = NULL; + while ((p = strpbrk(p, ",\"")) != NULL) { + if (*p == '\"' && (p <= list ||*(p - 1) != '\\')) { + in_quotes = !in_quotes; + } else if (*p == ',' && !in_quotes) { + *p = '\0'; + *state = p + 1; + break; + } + p++; + } + return list; +} /********************************************************************* // Name: SendText @@ -357,7 +382,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char { int res; char *p; - char *tempMailTo, *token, *pos1, *pos2; + char *tempMailTo, *token, *token_state, *pos1, *pos2; char *server_response = NULL; char *stripped_header = NULL; zend_string *data_cln; @@ -410,7 +435,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char tempMailTo = estrdup(mailTo); /* Send mail to all rcpt's */ - token = strtok(tempMailTo, ","); + token = find_address(tempMailTo, &token_state); while (token != NULL) { SMTP_SKIP_SPACE(token); @@ -424,14 +449,14 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char efree(tempMailTo); return (res); } - token = strtok(NULL, ","); + token = find_address(NULL, &token_state); } efree(tempMailTo); if (mailCc && *mailCc) { tempMailTo = estrdup(mailCc); /* Send mail to all rcpt's */ - token = strtok(tempMailTo, ","); + token = find_address(tempMailTo, &token_state); while (token != NULL) { SMTP_SKIP_SPACE(token); @@ -445,7 +470,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char efree(tempMailTo); return (res); } - token = strtok(NULL, ","); + token = find_address(NULL, &token_state); } efree(tempMailTo); } @@ -471,7 +496,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char tempMailTo = estrndup(pos1, pos2 - pos1); } - token = strtok(tempMailTo, ","); + token = find_address(tempMailTo, &token_state); while (token != NULL) { SMTP_SKIP_SPACE(token); @@ -485,7 +510,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char efree(tempMailTo); return (res); } - token = strtok(NULL, ","); + token = find_address(NULL,&token_state); } efree(tempMailTo); } @@ -496,7 +521,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char if (mailBcc && *mailBcc) { tempMailTo = estrdup(mailBcc); /* Send mail to all rcpt's */ - token = strtok(tempMailTo, ","); + token = find_address(tempMailTo, &token_state); while (token != NULL) { SMTP_SKIP_SPACE(token); @@ -510,7 +535,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char efree(tempMailTo); return (res); } - token = strtok(NULL, ","); + token = find_address(NULL, &token_state); } efree(tempMailTo); } @@ -544,7 +569,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char } } - token = strtok(tempMailTo, ","); + token = find_address(tempMailTo, &token_state); while (token != NULL) { SMTP_SKIP_SPACE(token); @@ -558,7 +583,7 @@ static int SendText(char *RPath, char *Subject, char *mailTo, char *mailCc, char efree(tempMailTo); return (res); } - token = strtok(NULL, ","); + token = find_address(NULL, &token_state); } efree(tempMailTo); @@ -978,6 +1003,34 @@ static unsigned long GetAddr(LPSTR szHost) return (lAddr); } /* end GetAddr() */ +/* returns the contents of an angle-addr (caller needs to efree) or NULL */ +static char *get_angle_addr(char *address) +{ + zend_bool in_quotes = 0; + char *p1 = address, *p2; + + while ((p1 = strpbrk(p1, "<\"")) != NULL) { + if (*p1 == '\"' && (p1 <= address ||*(p1 - 1) != '\\')) { + in_quotes = !in_quotes; + } else if (!in_quotes) { + break; + } + p1++; + } + if (p1 == NULL) return NULL; + p2 = ++p1; + while ((p2 = strpbrk(p2, ">\"")) != NULL) { + if (*p2 == '\"' && (*(p2 - 1) != '\\')) { + in_quotes = !in_quotes; + } else if (!in_quotes) { + break; + } + p2++; + } + if (p2 == NULL) return NULL; + + return estrndup(p1, p2 - p1); +} /********************************************************************* // Name: int FormatEmailAddress @@ -993,13 +1046,12 @@ static unsigned long GetAddr(LPSTR szHost) // History: //********************************************************************/ static int FormatEmailAddress(char* Buf, char* EmailAddress, char* FormatString) { - char *tmpAddress1, *tmpAddress2; + char *tmpAddress; int result; - if( (tmpAddress1 = strchr(EmailAddress, '<')) && (tmpAddress2 = strchr(tmpAddress1, '>')) ) { - *tmpAddress2 = 0; // terminate the string temporarily. - result = snprintf(Buf, MAIL_BUFFER_SIZE, FormatString , tmpAddress1+1); - *tmpAddress2 = '>'; // put it back the way it was. + if ((tmpAddress = get_angle_addr(EmailAddress)) != NULL) { + result = snprintf(Buf, MAIL_BUFFER_SIZE, FormatString , tmpAddress); + efree(tmpAddress); return result; } return snprintf(Buf, MAIL_BUFFER_SIZE , FormatString , EmailAddress ); From d4d9ba6423d5661fc3b23abd17231dd2adc9bffa Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 1 Mar 2021 13:44:45 +0100 Subject: [PATCH 2/5] Skip character after backslash in quoted-string instead of looking back --- ext/standard/tests/mail/bug80751.phpt | 4 ++-- win32/sendmail.c | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/ext/standard/tests/mail/bug80751.phpt b/ext/standard/tests/mail/bug80751.phpt index 0b1519c9611f2..d90be8385175f 100644 --- a/ext/standard/tests/mail/bug80751.phpt +++ b/ext/standard/tests/mail/bug80751.phpt @@ -39,7 +39,7 @@ function find_and_delete_message($username, $subject) { echo "From header found: "; var_dump(strpos($header, 'From: "" ') !== false); echo "Cc header found: "; - var_dump(strpos($header, 'Cc: "Lastname, Firstname" ') !== false); + var_dump(strpos($header, 'Cc: "Lastname, Firstname\\\\" ') !== false); imap_delete($imap_stream, $i); $found = true; break; @@ -56,7 +56,7 @@ $to = "\"\" <{$users[1]}@$domain>"; $subject = bin2hex(random_bytes(16)); $message = 'hello'; $headers = "From: \"\" \r\n" - . "Cc: \"Lastname, Firstname\" <{$users[2]}@$domain>\r\n" + . "Cc: \"Lastname, Firstname\\\\\" <{$users[2]}@$domain>\r\n" . "Bcc: \"Firstname \\\"Ni,ck\\\" Lastname\" <{$users[3]}@$domain>\r\n"; $res = mail($to, $subject, $message, $headers); diff --git a/win32/sendmail.c b/win32/sendmail.c index 373e82b4674b9..31240b35bc954 100644 --- a/win32/sendmail.c +++ b/win32/sendmail.c @@ -346,8 +346,10 @@ static char *find_address(char *list, char **state) p = list = *state; } *state = NULL; - while ((p = strpbrk(p, ",\"")) != NULL) { - if (*p == '\"' && (p <= list ||*(p - 1) != '\\')) { + while ((p = strpbrk(p, ",\"\\")) != NULL) { + if (*p == '\\' && in_quotes) { + p++; + } else if (*p == '\"') { in_quotes = !in_quotes; } else if (*p == ',' && !in_quotes) { *p = '\0'; @@ -1009,8 +1011,10 @@ static char *get_angle_addr(char *address) zend_bool in_quotes = 0; char *p1 = address, *p2; - while ((p1 = strpbrk(p1, "<\"")) != NULL) { - if (*p1 == '\"' && (p1 <= address ||*(p1 - 1) != '\\')) { + while ((p1 = strpbrk(p1, "<\"\\")) != NULL) { + if (*p1 == '\\' && in_quotes) { + p1++; + } else if (*p1 == '\"') { in_quotes = !in_quotes; } else if (!in_quotes) { break; @@ -1019,8 +1023,10 @@ static char *get_angle_addr(char *address) } if (p1 == NULL) return NULL; p2 = ++p1; - while ((p2 = strpbrk(p2, ">\"")) != NULL) { - if (*p2 == '\"' && (*(p2 - 1) != '\\')) { + while ((p2 = strpbrk(p2, ">\"\\")) != NULL) { + if (*p2 == '\\' && in_quotes) { + p2++; + } else if (*p2 == '\"') { in_quotes = !in_quotes; } else if (!in_quotes) { break; From 418d688705c7cbb159e7d30a60c216515e531ea5 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 1 Mar 2021 14:49:30 +0100 Subject: [PATCH 3/5] Handle case where backslash in quoted-string is last char That is an invalid email address, but we must not read beyond the string. Other than bailing out, we do not especially handle that case, but let the SMTP figure that out as usual. --- win32/sendmail.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/win32/sendmail.c b/win32/sendmail.c index 31240b35bc954..a3c371c675170 100644 --- a/win32/sendmail.c +++ b/win32/sendmail.c @@ -348,6 +348,10 @@ static char *find_address(char *list, char **state) *state = NULL; while ((p = strpbrk(p, ",\"\\")) != NULL) { if (*p == '\\' && in_quotes) { + if (p[1] == '\0') { + /* invalid address; let SMTP server deal with it */ + break; + } p++; } else if (*p == '\"') { in_quotes = !in_quotes; @@ -1013,6 +1017,10 @@ static char *get_angle_addr(char *address) while ((p1 = strpbrk(p1, "<\"\\")) != NULL) { if (*p1 == '\\' && in_quotes) { + if (p1[1] == '\0') { + /* invalid address; let SMTP server deal with it */ + return NULL; + } p1++; } else if (*p1 == '\"') { in_quotes = !in_quotes; @@ -1025,6 +1033,10 @@ static char *get_angle_addr(char *address) p2 = ++p1; while ((p2 = strpbrk(p2, ">\"\\")) != NULL) { if (*p2 == '\\' && in_quotes) { + if (p2[1] == '\0') { + /* invalid address; let SMTP server deal with it */ + return NULL; + } p2++; } else if (*p2 == '\"') { in_quotes = !in_quotes; From b5eedff82ab8213d3b9da734a62ea652b265981b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 1 Mar 2021 16:17:04 +0100 Subject: [PATCH 4/5] Don't break out of loop on backslash outside of quoted-strings --- win32/sendmail.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/win32/sendmail.c b/win32/sendmail.c index a3c371c675170..54cf07166cafe 100644 --- a/win32/sendmail.c +++ b/win32/sendmail.c @@ -1024,7 +1024,7 @@ static char *get_angle_addr(char *address) p1++; } else if (*p1 == '\"') { in_quotes = !in_quotes; - } else if (!in_quotes) { + } else if (*p1 == '<' && !in_quotes) { break; } p1++; @@ -1040,7 +1040,7 @@ static char *get_angle_addr(char *address) p2++; } else if (*p2 == '\"') { in_quotes = !in_quotes; - } else if (!in_quotes) { + } else if (*p2 == '>' && !in_quotes) { break; } p2++; From c92dd662f66112173bf68c2421a5ddf25fe25f25 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 1 Mar 2021 16:18:40 +0100 Subject: [PATCH 5/5] Fix minor issues Co-authored-by: Nikita Popov --- win32/sendmail.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/win32/sendmail.c b/win32/sendmail.c index 54cf07166cafe..ee017374f4923 100644 --- a/win32/sendmail.c +++ b/win32/sendmail.c @@ -353,7 +353,7 @@ static char *find_address(char *list, char **state) break; } p++; - } else if (*p == '\"') { + } else if (*p == '"') { in_quotes = !in_quotes; } else if (*p == ',' && !in_quotes) { *p = '\0'; @@ -1022,7 +1022,7 @@ static char *get_angle_addr(char *address) return NULL; } p1++; - } else if (*p1 == '\"') { + } else if (*p1 == '"') { in_quotes = !in_quotes; } else if (*p1 == '<' && !in_quotes) { break; @@ -1038,7 +1038,7 @@ static char *get_angle_addr(char *address) return NULL; } p2++; - } else if (*p2 == '\"') { + } else if (*p2 == '"') { in_quotes = !in_quotes; } else if (*p2 == '>' && !in_quotes) { break; @@ -1068,7 +1068,7 @@ static int FormatEmailAddress(char* Buf, char* EmailAddress, char* FormatString) int result; if ((tmpAddress = get_angle_addr(EmailAddress)) != NULL) { - result = snprintf(Buf, MAIL_BUFFER_SIZE, FormatString , tmpAddress); + result = snprintf(Buf, MAIL_BUFFER_SIZE, FormatString, tmpAddress); efree(tmpAddress); return result; }