fix DKIM issue in 6.1.2 - update PHPMailer.php

[ghost]

DKIM signature does not work when static::$LE=="\n"
It appends when $this->Mailer!='smtp' AND PHP_OS!="WIN".

Here are the messages I got, using PHPMailer/DKIMValidator:

Computed body hash does not match signature body hash
DKIM signature did not verify

It works fine after the fix, using "mail" (verified with PHPMailer/DKIMValidator and some online DKIM services)

[Synchro]

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?

[Synchro]

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.

[Synchro]

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.

[KlaasBonnema]

I can confirm that this also fails in 6.1.4. Same message sent through SMTP is ok, sent using mail on Linux the DKIM signature is incorrect.
I believe that the root cause is static::$LE which defaults to CRLF but with sending through mail on linux is changed to a single 0xA. The DKIM RFC states that all line breaks used with DKIM should be CRLF, even is the mail system is using different line endings. However function DKIM_Add refers to static::$LE instead of using hardcoded CRLF sequences.

[KlaasBonnema]

When you use PHP mail() on linux then static::$LE will be changed from CRLF to LF to accomodate the native line ending represented by PHP_EOL. Where mail expects LF, the MTA behind it will convert all LF to CRLF. On Windows this is different since the native line ending is already CRLF and PHP mail() expects CRLFs. If you use SMTP then it always has to be CRLF.
DKIM canonicalization has to be done with CRLF, even if the data is still using LF. The body text is correctly handled by DKIM_BodyC(). The header is where this goes wrong in DKIM_HeaderC(). The code to unfold the header lines expects CRLF and does not work for LF line endings. But then there should not be any LF line endings here.
The solution is to convert LF to CRLF if static::$LE is not equal to CRLF at the top of DKIM_HeaderC(). Then all following code including the unfold will always work with CRLFs.

Suggested and tested fix - insert as first code lines in DKIM_HeaderC():
if (static::$LE!="\r\n")
$signHeader = str_replace(static::$LE, "\r\n", $signHeader);

[ghost]

The solution is to convert LF to CRLF if static::$LE is not equal to CRLF at the top of DKIM_HeaderC(). Then all following code including the unfold will always work with CRLFs.

Suggested and tested fix - insert as first code lines in DKIM_HeaderC():
if (static::$LE!="\r\n")
$signHeader = str_replace(static::$LE, "\r\n", $signHeader);

I approve.

It has the same result as my fix, but is simpler to understand.

Regarding line 1557, I still believe this concatenated static::$LE can generate problem by breaking the MIME header. At least it can be replaced by a "\r\n" as the last character of $signHeader in DKIM_HeaderC().

[Synchro]

There is a built-in function for normalizing line breaks, so I used that instead, plus I added a couple of constants to make it clear when we are using static::$LE vs self::CRLF. Please give the DKIM-tweaks branch a try.

[KlaasBonnema]

"Please give the DKIM-tweaks branch a try."
I had a look at the code in this branch. I like the way you added the stripTrailingWSP function and self::CRLF.
However my suggested fix is not in and this code does not generate a valid DKIM signature.
You must first convert LF to CRLF to make the unfold work properly.
The l= length field has gone :-)

[Synchro]

Ah, I was already doing an equivalent of what you proposed (that's what normalizeBreaks() does), but I was doing it in the wrong place; I just pushed a fix for that, please retry.

[KlaasBonnema]

Yes, now I get a pass.

[ghost]

It still does not pass the test when checking with PHPMailer/DKIMValidator:

Computed body hash does not match signature body hash
DKIM signature did not verify

But works when:

1568: static::normalizeBreaks($header_dkim);// . static::$LE;

[KlaasBonnema]

This means that the validation of the body part fails. Strange, this code version is working ok for me.
Remember that PHPMailer is doing a 'simple' canonicalization on the body meaning it will not be forgiving when there is an extra line ending.
Do you have a multipart/content as I am testing?
Things to do - send your mail to [email protected] and the return mail will give you a detailed response including the original message. The question is if there is an extra line ending between the headers and the body - this is what your code fix would imply.
In my case the MTA's in between have added headers between the PHPMailer generated headers and the body and I do not have an extra line ending. There is one blank line between the last header and "This is a multi-part message in MIME format."

[ghost]

This means that the validation of the body part fails. Strange, this code version is working ok for me.
Remember that PHPMailer is doing a 'simple' canonicalization on the body meaning it will not be forgiving when there is an extra line ending.
Do you have a multipart/content as I am testing?

The problem is not related to the body content.

This extra static:$LE breaks the header in two parts. When I checked the body content (still using DKIMValidator) I found "To:" and "Subject:" as part of it, and of course they are not present in the MIME Header part.
Because "To:" and "Subject:" are positionned after "DKIM-Signature:"

See here the extra break (the content of the body is just "test"):

 +63V6D3SV+q1e4sta0Yl5zGmG3rEumJQnupm9URLjsZVxnArPff/BD+P4x5+++NFcsxv9gddv
 CQ5qCGNXrmTLA+KMsIF/sStsHhUBUEOvfy5NRPUyP9pcjA==

To: [email protected]
Subject: my subject

test

Please make your own test with DKIMValidator.
Maybe it works with some online DKIM verifiers, but probably not all.

[Synchro]

The header reordering is a thing that mail() does. It adds the To and Subject headers itself, so they need to be removed from the message when calling mail(), but present beforehand when you calculate the DKIM signature, and you just have to hope that PHP formats the added headers in exactly the same way.

The bug here looks like the extra line break is appearing between $this->MIMEHeader and $this->mailHeader; they are joined in the DKIM signature on line 1563, where no break is added (suggesting that MIMEHeader already ends in one). I can't see anywhere that adds another break between the two - it always joins them together directly, so the fix should be to find where the extra break is being appended to MIMEHeader before it gets to this point.

[Synchro]

I found where that happens, as you can see in the last commit. I think it's better to leave the other line break where it is, so it's clear when breaks are added during structural operations. Please give this a try with both mail() and SMTP!

[KlaasBonnema]

I did the test with both SMTP and mail() and both look ok and validate DKIM.
I put in some debugs in PHPMailer to see what goes along:
presend() after DKIM_Add() call - The DKIM header has no training line end (CRLF or LF), the MIME headers do have a trailing line end. The merged mime headers + dkim headers do have a trailing line end after the normalize call. This means that a line end after the DKIM header from DKIM_Add should be irrelevant because first the stripTrailingWSP takes it away after the merge (if it is there) and normalize puts it back.

mailPassThru() call - The to and subject headers have been removed from the mime headers before. They are now specified separately on the mail() call:
to, subject have no trailing line end. The merged mime headers, including dkim have a double trailing line end.

In the resulting email sent by mail() the To:, Subject: and X-PHP-Originating-Script: headers come first, followed by the merged mime headers. After that followed by X-... headers inserted by the mail server. I see no mixup of headers to body or incorrect line ends.

For reference, the mail() test was on Linux with PHP 7.2. I did the SMTP test om Windows with PHP 7.3.

[ghost]

I did the test with local mail() on macOS / PHP 7.4.2 / Composer (using DKIM-tweaks branch) and PHPMailer\DKIMValidator returned:

DKIM signature verified successfully!

Also checked with smtp(), using both PHPMailer\DKIMValidator and https://dkimvalidator.com: it is successfull.

[Synchro]

Thanks for opening this PR; I have now merged the other branch that fixes the same things as this.