-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Mime] Fix invalid DKIM signature with multiple parts #46863
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
Conversation
Can you add a test to prevent regressions ? |
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). |
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
be
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?) |
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)
That's why using a property |
Thank you @brokensourcecode. |
…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
@fabpot Yesterday, I saw that this test didn't pass. Won't that break something? |
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
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:
Backtrace of the second boundary:
The fix is to use a single instance of
AbstractMultipartPart
. I suggest to use a cache system for the methodEmail->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).
I hope this solution will suit you, I couldn't think of an easier one.