[Mime] Fix invalid DKIM signature with multiple parts #46863

BrokenSourceCode · 2022-07-05T21:32:56Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #42407, #40131, #39354
License	MIT

After many, many hours of investigations, I finally isolated the issue. When sending an email with a DKIM signature, 2 boundaries are generated because 2 instances of AbstractMultipartPart (AlternativePart in my case) are created for 1 email.

Backtrace of the first boundary:

Symfony\Component\Mime\Part\AbstractMultipartPart->getBoundary()
Symfony\Component\Mime\Part\AbstractMultipartPart->bodyToIterable()
Symfony\Component\Mime\Crypto\DkimSigner->hashBody()
Symfony\Component\Mime\Crypto\DkimSigner->sign()

Backtrace of the second boundary:

Symfony\Component\Mime\Part\AbstractMultipartPart->getBoundary()
Symfony\Component\Mime\Part\AbstractMultipartPart->getPreparedHeaders()
Symfony\Component\Mime\Part\AbstractPart->toIterable()
Symfony\Component\Mime\Message->toIterable()
Symfony\Component\Mime\RawMessage->toIterable()
Symfony\Component\Mailer\Transport\Smtp\Stream\AbstractStream::replace()
Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->doSend()
Symfony\Component\Mailer\Transport\AbstractTransport->send()
Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->send()
Symfony\Component\Mailer\Mailer->send()

The fix is to use a single instance of AbstractMultipartPart. I suggest to use a cache system for the method Email->getBody() with a property named $cachedBody. In this way, only one instance will be created. The property $cachedBody will be reset whenever necessary.

Thanks to @calebsolano for putting me on the right way with his comment #39354 (comment).

My instincts tell me it has something to do with the multipart boundary, in particular with it being random...but I can't determine where in the code it instantiates a second one that would have a different value between creating the body hash and the actual body

I hope this solution will suit you, I couldn't think of an easier one.

stof · 2022-07-06T07:35:45Z

Can you add a test to prevent regressions ?

BrokenSourceCode · 2022-07-06T10:46:48Z

Can you add a test to prevent regressions ?

@stof I think this should do the trick d470523.

src/Symfony/Component/Mime/Tests/EmailTest.php

src/Symfony/Component/Mime/Email.php

calebsolano · 2022-07-06T17:02:53Z

I hope this solution will suit you, I couldn't think of an easier one.

Maybe I still don't quite understand the root of the issue, but wouldn't it be easier to cache the boundary instead the body?

BrokenSourceCode · 2022-07-06T17:33:33Z

Maybe I still don't quite understand the root of the issue, but wouldn't it be easier to cache the boundary instead the body?

@calebsolano This is what I wanted to do first instead of the current commits:

Before

    private function getBoundary(): string
    {
        if (null === $this->boundary) {
            $this->boundary = strtr(base64_encode(random_bytes(6)), '+/', '-_');
        }

        return $this->boundary;
    }

After

    private function getBoundary(): string
    {
        static $boundary = null;
        if (null === $boundary) {
            $boundary = strtr(base64_encode(random_bytes(6)), '+/', '-_');
        }

        return $boundary;
    }

But it seems to me that the boundary must be unique for each e-mail, if I understand the rules correctly. It's right that this solution would be simpler, but I don't really know if having the same boundary for multiple emails can cause real problems. Maybe do you have any information about this?

And you, @stof @fabpot, do you have any views on this?

EDIT: https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

I don't see any rules for the unicity for each email. So maybe we should use this simple solution, by increasing the number of generated bytes from 6 to 12 (base64 encoded this is from 8 to 16).

calebsolano · 2022-07-06T18:29:47Z

I now see why the body caching seems to be the best solution.

Based on some of the comments above about "The two bodies must not reference the same object.", shouldn't

if (null !== $this->cachedBody) {
    return $this->cachedBody;
}

be

if (null !== $this->cachedBody) {
    return clone $this->cachedBody;
}

Also, couldn't the same functionality of the "cachedBody" be implemented by calling setBody instead? (Although that would require a call to setBody(null) in all the email modification functions, which may have side effect?)

BrokenSourceCode · 2022-07-06T18:41:12Z

I now see why the body caching seems to be the best solution.

Based on some of the comments above about "The two bodies must not reference the same object.", shouldn't
if (null !== $this->cachedBody) {
    return $this->cachedBody;
}
be
if (null !== $this->cachedBody) {
    return clone $this->cachedBody;
}

No, the comment you are talking about is just intended to comment the test assertion. We need to keep the same object to resolve the issue.

But I wonder if the boundary caching solution (with a static property) is not easier. Like that: #46863 (comment)

Also, couldn't the same functionality of the "cachedBody" be implemented by calling setBody instead? (Although that would require a call to setBody(null) in all the email modification functions, which may have side effect?)

That's why using a property cachedBody is easier than a method setBody(), because we don't need to changes all the functions everywhere

fabpot · 2022-07-07T07:35:14Z

Thank you @BrokenSourceCode.

@calebsolano

…kenSourceCode) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Mime] Fix invalid DKIM signature with multiple parts | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #42407, #40131, #39354 | License | MIT After many, many hours of investigations, I finally isolated the issue. When sending an email with a DKIM signature, **2** boundaries are generated because **2** instances of [`AbstractMultipartPart`](https://github.com/symfony/symfony/blob/04ae33a1f92bd76e17c7f0e7bbaacb5248cde79e/src/Symfony/Component/Mime/Part/AbstractMultipartPart.php) ([`AlternativePart`](https://github.com/symfony/symfony/blob/04ae33a1f92bd76e17c7f0e7bbaacb5248cde79e/src/Symfony/Component/Mime/Part/Multipart/AlternativePart.php) in my case) are created for **1** email. Backtrace of the first boundary: ``` Symfony\Component\Mime\Part\AbstractMultipartPart->getBoundary() Symfony\Component\Mime\Part\AbstractMultipartPart->bodyToIterable() Symfony\Component\Mime\Crypto\DkimSigner->hashBody() Symfony\Component\Mime\Crypto\DkimSigner->sign() ``` Backtrace of the second boundary: ``` Symfony\Component\Mime\Part\AbstractMultipartPart->getBoundary() Symfony\Component\Mime\Part\AbstractMultipartPart->getPreparedHeaders() Symfony\Component\Mime\Part\AbstractPart->toIterable() Symfony\Component\Mime\Message->toIterable() Symfony\Component\Mime\RawMessage->toIterable() Symfony\Component\Mailer\Transport\Smtp\Stream\AbstractStream::replace() Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->doSend() Symfony\Component\Mailer\Transport\AbstractTransport->send() Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->send() Symfony\Component\Mailer\Mailer->send() ``` The fix is to use a single instance of [`AbstractMultipartPart`](https://github.com/symfony/symfony/blob/04ae33a1f92bd76e17c7f0e7bbaacb5248cde79e/src/Symfony/Component/Mime/Part/AbstractMultipartPart.php). I suggest to use a cache system for the method [`Email->getBody()`](https://github.com/symfony/symfony/blob/04ae33a1f92bd76e17c7f0e7bbaacb5248cde79e/src/Symfony/Component/Mime/Email.php#L409-L416) with a property named `$cachedBody`. In this way, only one instance will be created. The property `$cachedBody` will be reset whenever necessary. Thanks to @calebsolano for putting me on the right way with his comment #39354 (comment). > My instincts tell me it has something to do with the multipart boundary, in particular with it being random...but I can't determine where in the code it instantiates a second one that would have a different value between creating the body hash and the actual body I hope this solution will suit you, I couldn't think of an easier one. Commits ------- f09f960 [Mime] Fix invalid DKIM signature with multiple parts

BrokenSourceCode · 2022-07-07T10:01:43Z

Thank you @BrokenSourceCode.

@fabpot Yesterday, I saw that this test didn't pass. Won't that break something?

xabbuh · 2022-07-11T15:09:22Z

@BrokenSourceCode see #46908

BrokenSourceCode · 2022-07-11T15:30:28Z

@BrokenSourceCode see #46908

Great! This is what I was talking about

…r equality (xabbuh) This PR was merged into the 4.4 branch. Discussion ---------- [Mime] ignore the cached body when comparing e-mails for equality | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | #46863 (comment) | License | MIT | Doc PR | Commits ------- bebdd61 ignore the cached body when comparing e-mails for equality

carsonbot added Status: Needs Review Bug Mime labels Jul 5, 2022

carsonbot added this to the 4.4 milestone Jul 5, 2022

This was referenced Jul 5, 2022

[Mailer] Signing email is not working with templated email #42407

Closed

[MAILER] DKIM signing not properly working with text part ? #40131

Closed

[Mailer] Symfony 5.2 DkimSigner not working (DKIM lookup failure) #39354

Closed

stof reviewed Jul 6, 2022

View changes

src/Symfony/Component/Mime/Tests/EmailTest.php Outdated Show resolved Hide resolved

fabpot reviewed Jul 6, 2022

View changes

src/Symfony/Component/Mime/Email.php Outdated Show resolved Hide resolved

fabpot approved these changes Jul 7, 2022

View changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 7, 2022

fabpot closed this Jul 7, 2022

fabpot force-pushed the fix-dkim-signature-multipart branch from ebfb9c8 to f09f960 Compare Jul 7, 2022

stloyd mentioned this pull request Jul 8, 2022

[Mime] [5.4] Fixed tests after merge up from 4.4 branch #46891

Closed

xabbuh mentioned this pull request Jul 11, 2022

[Mime] ignore the cached body when comparing e-mails for equality #46908

Merged

fjahn mentioned this pull request Jul 26, 2022

Drop plain text in Mailer::parseView simonschaufi/laravel-dkim#13

Closed

fabpot mentioned this pull request Jul 29, 2022

Release v4.4.44 #47114

Merged

This was referenced Jul 29, 2022

Release v5.4.11 #47115

Merged

Release v6.0.11 #47116

Merged

Release v6.1.3 #47117

Merged

[Mime] Fix invalid DKIM signature with multiple parts #46863

[Mime] Fix invalid DKIM signature with multiple parts #46863

BrokenSourceCode commented Jul 5, 2022 •

edited

stof commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 •

edited

calebsolano commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 •

edited

calebsolano commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 •

edited

fabpot commented Jul 7, 2022

BrokenSourceCode commented Jul 7, 2022 •

edited

xabbuh commented Jul 11, 2022

BrokenSourceCode commented Jul 11, 2022 •

edited

[Mime] Fix invalid DKIM signature with multiple parts #46863

[Mime] Fix invalid DKIM signature with multiple parts #46863

Conversation

BrokenSourceCode commented Jul 5, 2022 • edited

stof commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 • edited

calebsolano commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 • edited

calebsolano commented Jul 6, 2022

BrokenSourceCode commented Jul 6, 2022 • edited

fabpot commented Jul 7, 2022

BrokenSourceCode commented Jul 7, 2022 • edited

xabbuh commented Jul 11, 2022

BrokenSourceCode commented Jul 11, 2022 • edited

BrokenSourceCode commented Jul 5, 2022 •

edited

BrokenSourceCode commented Jul 6, 2022 •

edited

BrokenSourceCode commented Jul 6, 2022 •

edited

BrokenSourceCode commented Jul 6, 2022 •

edited

BrokenSourceCode commented Jul 7, 2022 •

edited

BrokenSourceCode commented Jul 11, 2022 •

edited