Page MenuHomePhabricator
Create Task
Maniphest T12404

Implement a first-party SMTP client
Open, NormalPublic
Actions

Description

We currently use PHPMailer (and PHPMailerLite, which is basically the same thing but without SMTP wire code) to build SMTP message bodies for mail.

Over time we've made changes to these classes to defuse security issues, work around bugs, or add capabilities, and it feels like we've reached a position where the cost of these externals (which are not very large, but have proven rich with issues) outweighs the benefit. Notable issues:

  • T12046, which is fairly egregious.
  • T12372, which uncovered some pretty questionable behavior in basic message construction in a realistic environment.
  • T5969 isn't exactly related but would fall in line here.
  • General issues with /e on regexps, switching to ExecFuture, etc.
  • The SES vs PHPMailer vs PHPMailerLite thing is a mess.

Revisions and Commits

rP Phabricator
D19961rP43a6f34e7f2b Update the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and…

Related Objects
Search...

Event Timeline

epriestley created this task.Mar 16 2017, 1:06 AM
Herald added a subscriber: eadler. · View Herald TranscriptMar 16 2017, 1:06 AM
epriestley added subtasks: T12046: PHPMailer RCE [CVE-2016-10033 and CVE-2016-10045], T12372: Long list of reviewers breaks to: field when metamta.one-mail-per-recipient set to false.Mar 16 2017, 1:06 AM
epriestley mentioned this in D17501: Apply the wordwrap() hack for "To" to PHPMailerLite.Mar 16 2017, 1:10 AM
epriestley closed subtask T12372: Long list of reviewers breaks to: field when metamta.one-mail-per-recipient set to false as Resolved.Mar 16 2017, 5:56 PM
epriestley mentioned this in rP19af10df3706: Apply the wordwrap() hack for "To" to PHPMailerLite.
ftdysa added a subscriber: ftdysa.Mar 17 2017, 1:38 AM
hskiba added a subscriber: hskiba.Mar 24 2017, 1:13 PM
epriestley mentioned this in T12525: amckinley's Onboarding.Apr 9 2017, 1:46 PM
joshuaspence added a subscriber: joshuaspence.May 3 2017, 12:36 PM
epriestley mentioned this in T12677: Support multiple mail delivery services for automatic failover.May 5 2017, 3:45 PM
epriestley added a comment.Jul 27 2017, 2:55 PM

This doesn't affect us, but more fuel on the fire -- XSS in example code which ships with PHPMailer (we don't distribute this code):

https://nvd.nist.gov/vuln/detail/CVE-2017-11503

epriestley mentioned this in T13053: Plans: Mail Tags and Failover.Jan 27 2018, 9:52 PM
epriestley moved this task from Backlog to Future on the Mail board.Jan 27 2018, 9:54 PM
• pasik added a subscriber: • pasik.May 12 2018, 2:11 PM
epriestley mentioned this in T920: Provide SMS Support.Jan 2 2019, 3:56 AM
epriestley mentioned this in D19956: Update Postmark adapter for multiple mail media.Jan 5 2019, 12:12 AM
epriestley added a revision: D19961: Update the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and "mailer".Jan 5 2019, 3:25 PM
epriestley moved this task from Future to Infrastructure on the Mail board.Jan 14 2019, 4:55 PM
amckinley mentioned this in D19955: Refactor mail to produce an intermediate "bag of strings" object in preparation for SMS.Jan 16 2019, 6:52 PM
epriestley added a commit: rP43a6f34e7f2b: Update the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and….Jan 16 2019, 9:11 PM
epriestley mentioned this in T13570: Amazon is retiring SMTP V3 Signatures.Aug 13 2020, 4:00 PM
aleb added a subscriber: aleb.Jan 14 2021, 6:45 PM

Recently smtp-relay.gmail.com stopped accepting email from our Phabricator instance because it turns out Phabricator was sending HELO localhost.localdomain instead of HELO smtp-relay.gmail.com when doing the SMTP connection.

This is the fix:

diff --git a/externals/phpmailer/class.phpmailer.php b/externals/phpmailer/class.phpmailer.php
index 5ddbddc144..be7a84f9f4 100644
--- a/externals/phpmailer/class.phpmailer.php
+++ b/externals/phpmailer/class.phpmailer.php
@@ -1941,6 +1943,8 @@ class PHPMailer {
   private function ServerHostname() {
     if (!empty($this->Hostname)) {
       $result = $this->Hostname;
+    } elseif (!empty($this->Host)) {
+      $result = $this->Host;
     } elseif (isset($_SERVER['SERVER_NAME'])) {
       $result = $_SERVER['SERVER_NAME'];
     } else {

The first error I saw was: Language string failed to load: tls which is caused by Lang('tls') because there is no 'tls' message.

Looking at the code it was clear that smtp->StartTLS() is failing, but it turns out that the previous call smtp->Hello() was also failing, without it being checked.

These could be turned into a proper fix:

diff --git a/externals/phpmailer/class.phpmailer.php b/externals/phpmailer/class.phpmailer.php
index 5ddbddc144..be7a84f9f4 100644
--- a/externals/phpmailer/class.phpmailer.php
+++ b/externals/phpmailer/class.phpmailer.php
@@ -781,11 +781,13 @@ class PHPMailer {
         if ($this->smtp->Connect(($ssl ? 'ssl://':'').$host, $port, $this->Timeout)) {
 
           $hello = ($this->Helo != '' ? $this->Helo : $this->ServerHostname());
-          $this->smtp->Hello($hello);
+          if (!$this->smtp->Hello($hello)) {
+              throw new phpmailerException('failed to hello: '.json_encode($this->smtp->getError()));
+          }
 
           if ($tls) {
             if (!$this->smtp->StartTLS()) {
-              throw new phpmailerException($this->Lang('tls'));
+              throw new phpmailerException($this->Lang('connect_host').' '.json_encode($this->smtp->getError()));
             }
 
             //We must resend HELO after tls negotiation
avivey added a subscriber: avivey.Sep 28 2021, 7:55 AM

From https://discourse.phabricator-community.org/t/sending-emails-causes-an-exception/4966, it appears that both class.smtp.php and class.phpmailer-lite.php have calls to each which is removed in PHP 8.

epriestley added a comment.Sep 28 2021, 2:40 PM

each is usually easy to replace and I'm happy to accept a change to replace it if someone wants to reproduce/test it. I believe this (totally ridiculous) construction:

while(list(,$line) = @each($lines)) {

...should be equivalent to this more straightforward construction, without each():

foreach ($lines as $line_parts) {
  list($ignored, $line) = $line_parts;

...but am not likely to have time to set up an SMTP server and actually test this change any time soon.

epriestley mentioned this in T13588: PHP 8 Compatibility.Sep 28 2021, 2:41 PM
Phabricator · Built by Phacility