1940605 - Regression in TB 128.6.0: Drafts and templates which are S/MIME signed lose their content on reuse

Status: VERIFIED FIXED

(MailNews Core :: Security: S/MIME, defect, P1)

Description

[Francesco]

Set up an IMAP account using S/MIME and configure it so messages are signed by default.
Create a draft or template, then try to edit the draft or create a new message from the template.

Result: The compose window is empty.

This worked in 128.5.2, so some of the backports must have caused it:
https://hg.mozilla.org/releases/comm-esr128/pushloghtml?fromchange=c489b0433bc2&tochange=86c514fafdab,
likely bug 1852662 or bug 1919290.

I haven't tested with a POP3 account or using PGP.

Comment 1

[Kai Engert [:KaiE:]]

Thanks for the report, I can reproduce, it's caused by the new checks from bug 1919290.

Comment 2

[Kai Engert [:KaiE:]]

We fail here:
https://searchfox.org/comm-esr128/rev/f7ecd1ad34d07dc62c3eaac96952d2e789291522/mailnews/mime/src/mimemcms.cpp#134

We try to interpret the stream_closure as mime_stream_data, but that fails, because it has type mime_draft_data.

In the past, this can have coincidentally worked, because:

• the first data member that is accessed by the following code is mime_stream_data::channel
• in type mime_draft_data, in the memory layout of the structure, there is a pointer member "stream" which is currently unused per comments, and always zero.
• apparently that null value might have caused the check for a non-null nsCOMPtr to have to decide "empty pointer", and consequently, no other data members were accessed.

In this specific code location, if we don't return on the unexpected type, but let the remainder of the function continue, the draft is loaded correctly.

Comment 3

[Kai Engert [:KaiE:]]

I've reviewed all places that use AsMimeStreamData and AsMimeDraftData.

I found one additional place that would be affected by the same change of behavior.

I suggest to revert both places to the previous behavior, which is, let the code continue if we find the wrong type (which would previously happen when the mime_stream_data->channel member was null, which it was always when the pointer was pointing to an instance of type mime_draft_data).

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Bug 1940605 - Fix a regression, allow S/MIME signed drafts to load. r=#thunderbird-back-end-reviewers

Comment 5

[Kai Engert [:KaiE:]]

(In reply to Kai Engert [:KaiE:] from comment #2)

In the past, this can have coincidentally worked, because:

• the first data member that is accessed by the following code is mime_stream_data::channel
• in type mime_draft_data, in the memory layout of the structure, there is a pointer member "stream" which is currently unused per comments, and always zero.

I missed to explain:
The mime_stream_data::channel members and the mime_draft_data::stream data members seem to be at the same memory offset in the object, which is the reason for the following behavior:

• apparently that null value might have caused the check for a non-null nsCOMPtr to have to decide "empty pointer", and consequently, no other data members were accessed.

In this specific code location, if we don't return on the unexpected type, but let the remainder of the function continue, the draft is loaded correctly.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/32f816740a60
Fix a regression, allow S/MIME signed drafts to load. r=#thunderbird-back-end-reviewers

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/911132303135
follow-up - Fix clang-format error. rs=clang-format DONTBUILD

Comment 9

[Francesco]

Working fine for S/MIME-signed drafts and templates, thanks for addressing this quickly.

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 9446536 [details]
Bug 1940605 - Fix a regression, allow S/MIME signed drafts to load. r=#thunderbird-back-end-reviewers

[Approval Request Comment]
Regression caused by (bug #): 1919290
User impact if declined: forwarding or draft/template editing of s/mime messages unusable
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): very low

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment 20

[Corey Bryant]

Comment on attachment 9446536 [details]
Bug 1940605 - Fix a regression, allow S/MIME signed drafts to load. r=#thunderbird-back-end-reviewers

[Triage Comment]
Approved for beta
Approved for esr128

Comment 35

[Magnus Melin [:mkmelin]]

Got landed for 128.6.1 (but no link here yet, so here it is)
https://hg.mozilla.org/releases/comm-esr128/rev/e103cf9cc8b363704fc21816121b39a4fd89e848

Comment 36

[Daniel Darnell [:dandarnell]]

Thunderbird 135.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/bf7fcf794fa1
https://hg.mozilla.org/releases/comm-beta/rev/7bdf1f7c598b

Comment 37

[Daniel Darnell [:dandarnell]]

Thunderbird 128.6.1esr:
https://hg.mozilla.org/releases/comm-esr128/rev/e103cf9cc8b3
https://hg.mozilla.org/releases/comm-esr128/rev/10c4c53bf555