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.

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

DKIM replace boundary tab with whitespace #1714

Closed
wants to merge 1 commit into from

Conversation

arnebr
Copy link

@arnebr arnebr commented Apr 26, 2019

The boundary for DKIM is not correct, at the moment it´s a tab it should be a whitespace.
This fixes #1352 and fixes #1655 .

The boundary for DKIM is not correct, at the moment it´s a tab it should be a witespace.
This fixes PHPMailer#234 and PHPMailer#1655 .
@arnebr arnebr changed the title DKIM replace boundary tab with witespace DKIM replace boundary tab with whitespace Apr 26, 2019
@Synchro
Copy link
Member

Synchro commented Apr 26, 2019

Do you have a reference for this? The DKIM RFC does not say this, neither in the definition of simple body canonicalization nor in the examples, which show tabs in headers and body remaining untouched in canonicalized versions.

@arnebr
Copy link
Author

arnebr commented Apr 26, 2019

UPDATE: RFC Reference in the next comment

The error lies in the bodyhash, I did test it against http://www.appmaildev.com/en/dkim (they add a header for the Expected-Body-Hash, so you can check ), https://www.mail-tester.com/ and gmail.

This is the body that gets tested in DKIM_Add($this->MIMEBody)

$body='This is a multi-part message in MIME format.
--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: multipart/alternative;
	boundary="b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs"

--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/plain; charset=us-ascii

Test

--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/html; charset=us-ascii

Test


--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs--

--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/plain; name="My uploaded file.pdf"
Content-Transfer-Encoding: base64
Content-ID: <My uploaded file.pdf>
Content-Disposition: attachment; filename="My uploaded file.pdf"

dGVzdA==

--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs--
';

These is the body that got send and the validators match against:

$body2='This is a multi-part message in MIME format.
--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: multipart/alternative;
 boundary="b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs"

--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/plain; charset=us-ascii

Test

--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/html; charset=us-ascii

Test


--b2_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs--

--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs
Content-Type: text/plain; name="My uploaded file.pdf"
Content-Transfer-Encoding: base64
Content-ID: <My uploaded file.pdf>
Content-Disposition: attachment; filename="My uploaded file.pdf"

dGVzdA==

--b1_4fKYfSeUT5Ii361M2SfMbZkpKoT4yZVeSTiqOLCMRs--
';

Here my little test script, the function normalizeBreaks() got called from the PHPMailer.php

$body = normalizeBreaks($body, "\r\n");
$body=rtrim($body, "\r\n") . "\r\n";

$body2 = normalizeBreaks($body2, "\r\n");
$body2=rtrim($body2, "\r\n") . "\r\n";

echo 'Original:'. base64_encode(hash('sha256', $body, true)); //OiFuRQ+40oO3GvcwDPxhrS9s3JrLTFoPrw3RRhd8Dl8=
echo 'Send body:'.base64_encode(hash('sha256', $body2, true)); //7v41Dkvet0JqKXqWWaP9YSOPEMXrV0+RQ3I1xreR0LY=


$body=preg_replace('/\t+boundary/', ' boundary', $body);

echo 'Fixed body:'. base64_encode(hash('sha256', $body, true)); // 7v41Dkvet0JqKXqWWaP9YSOPEMXrV0+RQ3I1xreR0LY=

@arnebr
Copy link
Author

arnebr commented Apr 26, 2019

Okay I had another look.

As stated in PHPMailer.php on Line 4296 we are using: $DKIMcanonicalization = 'relaxed/simple';

So if we take a look in RFC 6376 3.4.4. a) (2)

Reduce all sequences of WSP within a line to a single SP character.

WSP represents simple whitespace, i.e., a space or a tab character (formal definition in [RFC5234]).
WSP = SP / HTAB

@Synchro
Copy link
Member

Synchro commented Apr 26, 2019

We are not using relaxed body canonicalization, so that does not apply. The relaxed/simple name means we are using relaxed for the headers (which do indeed normalise all whitespace to a space - see DKIM_HeaderC), and simple for the body, which has no requirement for replacing tabs in the body (in fact it should cause it to fail).

@arnebr
Copy link
Author

arnebr commented Apr 26, 2019

Well than I really do not know.

The question for you all then is, why the $body2 without a tab gets send and not $bodywith the tab? The place where the tabs got deleted, that will then be the solution, wouldn´t it?!

@Synchro
Copy link
Member

Synchro commented Apr 26, 2019

Being very keen on Postel's law, I'm also very averse to generating contra-RFC output. If DKIM evaluators are not handling tabs for folding correctly, I think a better approach would perhaps be to not use tabs for folding at all. Email RFCs generally assume that whitespace is [ \t], so switching to spaces everywhere should not cause any issues or contraventions, but may help this practical DKIM evaluation problem, and it would be a better solution that the band-aid approach here.

What would be extra cool is to have some DKIM evaluation built into the unit tests, perhaps using something like this...

@arnebr arnebr closed this Apr 26, 2019
@Synchro
Copy link
Member

Synchro commented Apr 26, 2019

I've just pushed a switch from tabs to spaces in header folding and param separation in master - please give it try and see if it helps.

@arnebr
Copy link
Author

arnebr commented Apr 28, 2019

It works indeed @SynchroM, thanks for that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DKIM signature fails with attachment Dkim fails with Altbody
2 participants