Page MenuHomePhabricator

Implement a first-party SMTP client
Open, NormalPublic

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.

Event Timeline

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

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

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.

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.