Skip to content
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

Jump to bottom

fix DKIM issue in 6.1.2 - update PHPMailer.php #1962

Closed
wants to merge 1 commit into from
Closed

fix DKIM issue in 6.1.2 - update PHPMailer.php #1962

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions src/PHPMailer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,7 +1554,7 @@ public function preSend()
$this->MIMEBody
);
$this->MIMEHeader = rtrim($this->MIMEHeader, "\r\n ") . static::$LE .
static::normalizeBreaks($header_dkim) . static::$LE;
Copy link
Member

Choose a reason for hiding this comment

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

Can we double check whether this would be better fixed by having $header_dkim not include an LE to start with, which is done on line 4671?

Copy link
Member

Choose a reason for hiding this comment

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

Looking further into this - PHPMailer generally doesn't trust its own assembly of headers, and pretty much always strips breaks and readds them, for example in smtpSend and getSentMIMEMessage(). That said, MIMEHeader is expected to have a trailing break as it often gets mailHeader concatenated directly to it, and when we are DKIM signing, the signature header is appended to MIMEHeader, so it does need the trailing break here.

static::normalizeBreaks($header_dkim);
}

return true;
Expand Down Expand Up @@ -4476,9 +4476,11 @@ public function DKIM_HeaderC($signHeader)
//@see https://tools.ietf.org/html/rfc5322#section-2.2
//That means this may break if you do something daft like put vertical tabs in your headers.
//Unfold header lines
$signHeader = preg_replace('/\r\n[ \t]+/', ' ', $signHeader);
$signHeader = str_replace(["\r\n","\r"], ["\n","\n"], $signHeader);
$signHeader = preg_replace('/\n[ \t]+/', ' ', $signHeader);

Copy link
Member

Choose a reason for hiding this comment

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

I don't think these changes are really appropriate. As the comment on this function says:

Canonicalized headers should always use CRLF, regardless of mailer setting

PHPMailer tries to handle everything as CRLF until it is not required to do so (e.g. to use mail()). Inconsistent internal line break handling is something that was a major problem in 5.2 and was addressed in 6. So while this might fix your current issue (though I think that's strange because CRLF is required to calculate a DKIM signature correctly, even if the message is sent using mail()), I think it's likely to cause problems in other situations. A receiver will always recalculate the signature using CRLF, so not doing the same when sending is asking for trouble.

//Break headers out into an array
$lines = explode("\r\n", $signHeader);
$lines = explode("\n", $signHeader);
foreach ($lines as $key => $line) {
//If the header is missing a :, skip it as it's invalid
//This is likely to happen because the explode() above will also split
Expand Down