1746639 - Crash in [@ mime_crypto_write_base64] Crash Address 0xe5e5e5e9

Status: RESOLVED FIXED

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

Description

[Wayne Mery (:wsmwk)]

Crash report: https://crash-stats.mozilla.org/report/index/8d4f7d32-28a8-4cac-bf0e-c4afa0211109

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mime_crypto_write_base64 comm/mailnews/extensions/smime/nsMsgComposeSecure.cpp:1050
1 nss3.dll nss_cms_encoder_out security/nss/lib/smime/cmsencode.c:88
2 nss3.dll SEC_ASN1EncoderUpdate_Util security/nss/lib/util/secasn1e.c:1223
3 nss3.dll NSS_CMSEncoder_Cancel security/nss/lib/smime/cmsencode.c:691
4 xul.dll nsCMSEncoder::~nsCMSEncoder comm/mailnews/extensions/smime/nsCMS.cpp:801
5 xul.dll MailnewsLoadContextInfo::Release comm/mailnews/base/src/MailnewsLoadContextInfo.cpp:17
6 xul.dll nsCOMPtr_base::assign_from_helper xpcom/base/nsCOMPtr.cpp:112
7 xul.dll nsMsgComposeSecure::MimeInitEncryption comm/mailnews/extensions/smime/nsMsgComposeSecure.cpp:543
8 xul.dll nsMsgComposeSecure::BeginCryptoEncapsulation comm/mailnews/extensions/smime/nsMsgComposeSecure.cpp:375
9 xul.dll NS_InvokeByIndex 

Comment 1

[Kai Engert (:KaiE:)]

Is this a one-time crash or a frequent crash?

Comment 2

[Kai Engert (:KaiE:)]

That C code in cmsencode.c does dangerous stuff...

Analysis notes:
We are in the context of composing an S/MIME message with encryption.
We're still processing the code that prepares the message encryption.
We're constructing an instance of NS_CMSENCODER_CONTRACTID.

This triggers cleanup of a previous instance of nsCMSEncoder, which is surprising.
And that nsCMSEncoder is apparently active, it hasn't been completed yet, because it is being destroyed although we haven't gone through nsCMSEncoder::Finish yet (which would have set m_ecx to null, but m_ecx is currently non-null).

If nsMsgComposeSecure::mEncryptionContext is already initialized while we assign a new instance to it, it means that we're calling nsMsgComposeSecure::MimeInitEncryption twice for the same instance of nsMsgComposeSecure - while the previous use of this instance hasn't been completed. That seems unexpected, and it's surprising how it could happen. Maybe the user clicked "send" twice very quickly? Or we have a bug that doesn't reliably reset the state of the instance in a failure scenario.

Comment 3

[Kai Engert (:KaiE:)]

It looks like we don't reset the failed mEncryptionContext instance in a failure scenario. This at least explains why we get the nsCMSEncoder cleanup at construction time of a new nsCMSEncoder instance.

It doesn't yet explain the crash, I think.

Comment 4

[Kai Engert (:KaiE:)]

I think the problem is with function NSS_CMSEncoder_Cancel.

It seems this function does more than it should. The function is very similar to NSS_CMSEncoder_Finish. And NSS_CMSEncoder_Cancel hasn't seen any functional changes since year 2000.

It isn't clear why NSS_CMSEncoder_Cancel needs to call SEC_ASN1EncoderUpdate (which triggers the crash), when we're intending to cancel processing.

Also the function has a comment
/* XXX do this right! */
which could mean that this function has never gotten the attention to perform the cleanup correctly.

I think this function should be changed to perform as few actions as possible.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to Kai Engert (:KaiE:) from comment #1)

Is this a one-time crash or a frequent crash?

Not one-time judging by the "installations" column of https://crash-stats.mozilla.org/signature/?signature=mime_crypto_write_base64&date=%3E%3D2021-07-07T12%3A57%3A00.000Z&date=%3C2022-01-07T12%3A57%3A00.000Z

But it's not frequent, but is consistent, until crash-stats stopped accepting reports https://crash-stats.mozilla.org/signature/?signature=mime_crypto_write_base64&date=%3E%3D2021-07-07T12%3A57%3A00.000Z&date=%3C2022-01-07T12%3A57%3A00.000Z#graphs

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Bug 1746639 - Fix a crash if S/MIME message sending fails and the user retries. r=mkmelin

Comment 7

[Kai Engert (:KaiE:)]

I haven't found a clear scenario for crashing, it's just theory so far.

However, I've suggested two patches, here and in bug 1749010, which could potentially avoid a crash.

Comment 8

[Kai Engert (:KaiE:)]

Ok, I found the cause of the crash!
See the comment in the updated patch attached to this bug. (I closed the separate NSS bugzilla ticket because it's unnecessary.)

We crash if creating an S/MIME message fails during the final parts of processing, and afterwards the user tries again.

I can simulate the bug by adding a failure return near the end of nsMsgComposeSecure::MimeInitEncryption.
Then the crash can be triggered by attempting to send an encrypted/signed S/MIME message, get a failure dialog, and try to send again.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: 1746639-esr91-v1.patch

merged to esr91

Comment 10

[Kai Engert (:KaiE:)]

This is a use-after-free crash, but on locally created data. I think this isn't exploitable and the bug could be made public.

Comment 13

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/b7983f333845c584d8fc376e43cfba2290d003b2

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Kai Engert (:KaiE:) from comment #10)

This is a use-after-free crash, but on locally created data. I think this isn't exploitable and the bug could be made public.

I don't know what you mean by "locally created data". It doesn't matter who created the freed data, but how replacement data would be used if someone could reliably cause something else to be reallocated into that emply slot.

More relevant is that composing mail is not scriptable so I don't see a way for an exploit to groom memory or have any control over the timing. Yeah, you can make this public.

Comment 15

[Kai Engert (:KaiE:)]

Comment on attachment 9258160 [details] [diff] [review]
1746639-esr91-v1.patch

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

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9258160 [details] [diff] [review]
1746639-esr91-v1.patch

[Triage Comment]
Approved for esr91

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 91.5.1:
https://hg.mozilla.org/releases/comm-esr91/rev/01fe5565f593