Skip to content

Commit

Permalink
Merge pull request from GHSA-f7hx-fqxw-rvvj
Browse files Browse the repository at this point in the history 
* Initial fixes, tests, and bump to 6.1.6

* Add CVE number
  • Loading branch information
@Synchro
Synchro committed May 27, 2020
1 parent a7f1b23 commit c2796cb
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 23 deletions.
2 changes: 2 additions & 0 deletions SECURITY.md
View file
Expand Up @@ -2,6 +2,8 @@

Please disclose any vulnerabilities found responsibly - report any security problems found to the maintainers privately.

PHPMailer versions 6.1.5 and earlier contain an output escaping bug that occurs in `Content-Type` and `Content-Disposition` when filenames passed into `addAttachment` and other methods that accept attachment names contain double quote characters, in contravention of RFC822 3.4.1. No specific vulnerability has been found relating to this, but it could allow file attachments to bypass attachment filters that are based on matching filename extensions. Recorded as [CVE-2020-13625](https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2020-13625). Reported by Elar Lang of Clarified Security.

PHPMailer versions prior to 6.0.6 and 5.2.27 are vulnerable to an object injection attack by passing `phar://` paths into `addAttachment()` and other functions that may receive unfiltered local paths, possibly leading to RCE. Recorded as [CVE-2018-19296](https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2018-19296). See [this article](https://knasmueller.net/5-answers-about-php-phar-exploitation) for more info on this type of vulnerability. Mitigated by blocking the use of paths containing URL-protocol style prefixes such as `phar://`. Reported by Sehun Oh of cyberone.kr.

PHPMailer versions prior to 5.2.24 (released July 26th 2017) have an XSS vulnerability in one of the code examples, [CVE-2017-11503](https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2017-11503). The `code_generator.phps` example did not filter user input prior to output. This file is distributed with a `.phps` extension, so it it not normally executable unless it is explicitly renamed, and the file is not included when PHPMailer is loaded through composer, so it is safe by default. There was also an undisclosed potential XSS vulnerability in the default exception handler (unused by default). Patches for both issues kindly provided by Patrick Monnerat of the Fedora Project.
Expand Down
2 changes: 1 addition & 1 deletion VERSION
View file
@@ -1 +1 @@
6.1.5
6.1.6
4 changes: 3 additions & 1 deletion changelog.md
View file
@@ -1,9 +1,11 @@
# PHPMailer Change Log

## Version 6.1.6 (May 27th, 2020)
* **SECURITY** Fix insufficient output escaping bug in file attachment names. CVE-2020-13625. Reported by Elar Lang of Clarified Security.
* Correct Armenian ISO language code from `am` to `hy`, add mapping for fallback
* Use correct timeout property in debug output

## Version 6.1.5 (March 14th, 2020
## Version 6.1.5 (March 14th, 2020)
* Reject invalid custom headers that are empty or contain breaks
* Various fixes for DKIM issues, especially when using `mail()` transport
* Drop the `l=` length tag from DKIM signatures; it's a mild security risk
Expand Down
47 changes: 30 additions & 17 deletions src/PHPMailer.php
View file
Expand Up @@ -745,7 +745,7 @@ class PHPMailer
*
* @var string
*/
const VERSION = '6.1.5';
const VERSION = '6.1.6';

/**
* Error severity: message only, continue processing.
Expand Down Expand Up @@ -2607,7 +2607,7 @@ public function createBody()
$altBodyEncoding = static::ENCODING_QUOTED_PRINTABLE;
}
//Use this as a preamble in all multipart message types
$mimepre = 'This is a multi-part message in MIME format.' . static::$LE . static::$LE;
$mimepre = 'This is a multi-part message in MIME format.' . static::$LE . static::$LE;
switch ($this->message_type) {
case 'inline':
$body .= $mimepre;
Expand Down Expand Up @@ -3064,9 +3064,9 @@ protected function attachAll($disposition_type, $boundary)
//Only include a filename property if we have one
if (!empty($name)) {
$mime[] = sprintf(
'Content-Type: %s; name="%s"%s',
'Content-Type: %s; name=%s%s',
$type,
$this->encodeHeader($this->secureHeader($name)),
static::quotedString($this->encodeHeader($this->secureHeader($name))),
static::$LE
);
} else {
Expand All @@ -3086,24 +3086,14 @@ protected function attachAll($disposition_type, $boundary)
$mime[] = 'Content-ID: <' . $this->encodeHeader($this->secureHeader($cid)) . '>' . static::$LE;
}

// If a filename contains any of these chars, it should be quoted,
// but not otherwise: RFC2183 & RFC2045 5.1
// Fixes a warning in IETF's msglint MIME checker
// Allow for bypassing the Content-Disposition header totally
// Allow for bypassing the Content-Disposition header
if (!empty($disposition)) {
$encoded_name = $this->encodeHeader($this->secureHeader($name));
if (preg_match('/[ ()<>@,;:"\/\[\]?=]/', $encoded_name)) {
$mime[] = sprintf(
'Content-Disposition: %s; filename="%s"%s',
$disposition,
$encoded_name,
static::$LE . static::$LE
);
} elseif (!empty($encoded_name)) {
if (!empty($encoded_name)) {
$mime[] = sprintf(
'Content-Disposition: %s; filename=%s%s',
$disposition,
$encoded_name,
static::quotedString($encoded_name),
static::$LE . static::$LE
);
} else {
Expand Down Expand Up @@ -3163,6 +3153,7 @@ protected function encodeFile($path, $encoding = self::ENCODING_BASE64)
if ($this->exceptions) {
throw $exc;
}

return '';
}
}
Expand Down Expand Up @@ -4727,6 +4718,28 @@ public static function hasLineLongerThanMax($str)
return (bool) preg_match('/^(.{' . (self::MAX_LINE_LENGTH + strlen(static::$LE)) . ',})/m', $str);
}

/**
* If a string contains any "special" characters, double-quote the name,
* and escape any double quotes with a backslash.
*
* @param string $str
*
* @return string
*
* @see RFC822 3.4.1
*/
public static function quotedString($str)
{
if (preg_match('/[ ()<>@,;:"\/\[\]?=]/', $str)) {
//If the string contains any of these chars, it must be double-quoted
//and any double quotes must be escaped with a backslash
return '"' . str_replace('"', '\\"', $str) . '"';
}

//Return the string untouched, it doesn't need quoting
return $str;
}

/**
* Allows for public read access to 'to' property.
* Before the send() call, queued addresses (i.e. with IDN) are not yet included.
Expand Down
2 changes: 1 addition & 1 deletion src/POP3.php
View file
Expand Up @@ -45,7 +45,7 @@ class POP3
*
* @var string
*/
const VERSION = '6.1.5';
const VERSION = '6.1.6';

/**
* Default POP3 port number.
Expand Down
2 changes: 1 addition & 1 deletion src/SMTP.php
View file
Expand Up @@ -34,7 +34,7 @@ class SMTP
*
* @var string
*/
const VERSION = '6.1.5';
const VERSION = '6.1.6';

/**
* SMTP line break constant.
Expand Down
60 changes: 58 additions & 2 deletions test/PHPMailerTest.php
View file
Expand Up @@ -1214,7 +1214,7 @@ public function testHTMLAttachment()
}

//Make sure that trying to attach a nonexistent file fails
$filename = __FILE__ . md5(microtime()). 'nonexistent_file.txt';
$filename = __FILE__ . md5(microtime()) . 'nonexistent_file.txt';
$this->assertFalse($this->Mail->addAttachment($filename));
//Make sure that trying to attach an existing but unreadable file fails
touch($filename);
Expand All @@ -1227,6 +1227,62 @@ public function testHTMLAttachment()
$this->assertTrue($this->Mail->send(), $this->Mail->ErrorInfo);
}

/**
* Attachment naming test.
*/
public function testAttachmentNaming()
{
$this->Mail->Body = 'Attachments.';
$this->Mail->Subject .= ': Attachments';
$this->Mail->isHTML(true);
$this->Mail->CharSet = 'UTF-8';
$this->Mail->addAttachment(
realpath($this->INCLUDE_DIR . '/examples/images/phpmailer_mini.png'),
'phpmailer_mini.png";.jpg'
);
$this->Mail->addAttachment(
realpath($this->INCLUDE_DIR . '/examples/images/phpmailer.png'),
'phpmailer.png'
);
$this->Mail->addAttachment(
realpath($this->INCLUDE_DIR . '/examples/images/PHPMailer card logo.png'),
'PHPMailer card logo.png'
);
$this->buildBody();
$this->Mail->preSend();
$message = $this->Mail->getSentMIMEMessage();
$this->assertContains(
'Content-Type: image/png; name="phpmailer_mini.png\";.jpg"',
$message,
'Name containing double quote should be escaped in Content-Type'
);
$this->assertContains(
'Content-Disposition: attachment; filename="phpmailer_mini.png\";.jpg"',
$message,
'Filename containing double quote should be escaped in Content-Disposition'
);
$this->assertContains(
'Content-Type: image/png; name=phpmailer.png',
$message,
'Name without special chars should not be quoted in Content-Type'
);
$this->assertContains(
'Content-Disposition: attachment; filename=phpmailer.png',
$message,
'Filename without special chars should not be quoted in Content-Disposition'
);
$this->assertContains(
'Content-Type: image/png; name="PHPMailer card logo.png"',
$message,
'Name with spaces should be quoted in Content-Type'
);
$this->assertContains(
'Content-Disposition: attachment; filename="PHPMailer card logo.png"',
$message,
'Filename with spaces should be quoted in Content-Disposition'
);
}

/**
* Test embedded image without a name.
*/
Expand Down Expand Up @@ -1687,7 +1743,7 @@ public function testAddressSplitting()
['name' => 'Jill User', 'address' => 'jill@example.net'],
['name' => '', 'address' => 'frank@example.com'],
],
$this->Mail->parseAddresses(
PHPMailer::parseAddresses(
'Joe User <joe@example.com>,'
. 'Jill User <jill@example.net>,'
. 'frank@example.com,'
Expand Down

2 comments on commit c2796cb

@dellasorte
Copy link

Choose a reason for hiding this comment

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

what about backslash + double quoted input? is it covered or ? " => \" ?

@Synchro
Copy link
Member Author

@Synchro Synchro commented on c2796cb Jun 8, 2020

Choose a reason for hiding this comment

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

As far as email goes, the only char that matters is the double quote - I don't know that backslashes themselves need escaping, however, it's likely there are client issues related to this. Got a test case?

Please sign in to comment.