1876931 - S/MIME Encryption fails when user has at least one unsupported cert type.

Status: RESOLVED FIXED

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

Description

[Jan Nordqvist]

Attachment: Screenshot from 2024-01-27 12-52-26.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:122.0) Gecko/20100101 Firefox/122.0

Steps to reproduce:

Send two signed messages from sender A to recipient B where one is associated with a certificate with an unsupported algorithm such as ecPublicKey and the other with a supported algorithm such as RSA.

Actual results:

When attempting to send an encrypted message back from B to A, sending fails even though a note at the bottom of the composer window suggests that "S/MIME end-to-end encryption is possible."

Expected results:

The proper certificate, in this case the RSA certificate, should have been selected for encryption (and the EC cert ignored) and sending should have succeeded.
The code segment in comm/mailnews/extensions/smime/nsCMS.cpp starting at line 786 loops through the available certificates for A, apparently in an attempt to find a usable certificate, but instead fails if one unsuitable certificate is encountered.

Attached is a screenshot of a potential fix to this code that has been verified to work as intended per this description.

Comment 1

[Jan Nordqvist]

BTW, one additional note: it would be very nice (and in compliance with RFC-8550) if TB had encryption support for ecPublicKey and potentially also edDsa25519. Be great to know if this is on the roadmap and at what point such support could be implemented.

Comment 2

[Kai Engert [:KaiE:]]

Hello Jan, thanks a lot for your investigation.
I think we need a different fix than the one you have suggested.

You suggest to return from that loop, as soon as we have succeeded to add a recipient to the message.
I think this will not work for messages that have more than one recipient.

The fix will likely be required in function nsMsgComposeSecure::MimeCryptoHackCerts

When looking for the encryption certificate to use for each recipient, we must consider the scenario that there may be alternative certificates available, and must select the one that we can use.

Comment 3

[Kai Engert [:KaiE:]]

Jan, are you able to send me the alternative certificates? This would simplify my testing.
You may either send me two separate signed messages, or just the saved certificates, please use what's easier for you to do.
Thanks in advance!

Comment 4

[Kai Engert [:KaiE:]]

(In reply to Jan Nordqvist from comment #0)

Attached is a screenshot of a potential fix to this code that has been verified to work as intended per this description.

I don't understand yet how this change could have fixed the bug for you.

The call to NSS_CMSEnvelopedData_AddRecipient doesn't perform any verification of the certificate, it seems to simply add the cert to an array.

Comment 5

[Jan Nordqvist]

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

Jan, are you able to send me the alternative certificates? This would simplify my testing.
You may either send me two separate signed messages, or just the saved certificates, please use what's easier for you to do.
Thanks in advance!

Hello Kai,
I just packaged up all pertinent certs and can email them to you if you don't mind sharing an email address.
As part of this I unfortunately think I noticed another glitch. There are two save links in the cert view window in TB; "Download
PEM (cert) PEM (chain)", but both links save just the EE cert with the only difference that the "chain" link tacks on the word "chain" at the end of the base name.

Comment 6

[Jan Nordqvist]

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

(In reply to Jan Nordqvist from comment #0)

Attached is a screenshot of a potential fix to this code that has been verified to work as intended per this description.

I don't understand yet how this change could have fixed the bug for you.

The call to NSS_CMSEnvelopedData_AddRecipient doesn't perform any verification of the certificate, it seems to simply add the cert to an array.

That is the exact assumption I would have made as well, but not knowing the code I blindly added instrumentation (printf) as a means to zero in on the spot and this is where it failed. I wouldn't be able to tell you why, but the change did indeed fix it for me. Chances are of course that my code is not a proper fix, but at least it exposed the problem.

Comment 7

[Kai Engert [:KaiE:]]

Now I understand how your patch suggestion works.

Somehow I had missed that you had added the "continue" if NSS_CMSRecipientInfo_Create fails.
That's the code that fails.

So what your suggestion does, it would skip the one recipient, and continue encrypting to everyone else.

This would result in messages being sent, that cannot be decrypted by the recipients for whom we have unsupported certificates.

I wonder why you had the impression that it worked for you.
I wonder if you ran into a very specific scenario:

• you had both certs imported
• you sent an encrypted message to yourself as the only recipient
• initially, it selected your configured RSA cert for encrypting to yourself
• then, when looking for a cert for the recipient, it found your ECC cert, failed, but you ignored the failure
• as a result, you received a message that you could decrypt with your RSA cert.

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Bug 1876931 - Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

Comment 9

[Jan Nordqvist]

From what you are telling me it sounds like the code in question iterates through all certificates for all recipients with the intention to break out if it fails to encrypt for at least one of the recipients - which makes sense. The problem here is that one of the recipients happens to have two certificates of which one has an unsupported algorithm, i.e. ecPublicKey.
Let me also clarify another thing: The sender, in this case jan@..., at first had the ecPublicKey certificate installed, and once I determined that the receiver, i.e. test@... failed to encrypt I went ahead and replaced jan's ecPublicKey certificate with an RSA certificate only to discover that encryption still failed.
This indicates that the receiving client, i.e. the one serving test, found both certificates for jan (maybe because the previous "ec" message was still in the inbox) and entered the failing loop with one recipient with two certificates.
I think what is needed here is something like this pseudo-code:

Map<Recipient, Cert> checkEligibility(Map<Recipient, List<Cert>>) {
Map<Recipient, Cert> recipient2Cert
for ((Recipient, List<Cert>) {
val cert = findOneUsableCert(List<Cert>)
if (cert) {
recipient2Cert[recipient] = cert
} else {
throw("recipient x has no usable cert")
}
}
return recipient2Cert
}

Comment 10

[Kai Engert [:KaiE:]]

I have attached a fix, which has the following effect:

• if we have only a unsupported certificate for the recipient,
  then TB will no longer claim that encryption is possible

• if we have two alternative certificates for the same email address available,
  where one certificate is unsupported,
  TB will ignore the unsupported one, and only consider to use the
  supported cert.

Ideally the raw implementation for this fix should be added to the NSS library.

However, this would add a complication for releasing this fix into the stable Thunderbird release.
(It would be necessary to release a new NSS version and then ensure all distribution channels have the updated NSS library available.)

As a simplification, my suggestion is:

• initially add this code to Thunderbird
• backport to comm-esr115
• wait for the the updated NSS
• only change comm-central to use the new NSS API

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/572120cf4be3
Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/054a0147cb01
follow-up, fix clang-format. rs=lint DONTBUILD

Comment 13

[Wayne Mery (:wsmwk)]

Wanted for beta? (that could put it on track for 115.8.0)

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 9377411 [details]
Bug 1876931 - Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: broken s/mime for some users
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9377411 [details]
Bug 1876931 - Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

[Triage Comment]
Approved for beta

Comment 16

[Daniel Darnell [:dandarnell]]

Thunderbird 123.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/365ca750acf9
https://hg.mozilla.org/releases/comm-beta/rev/4d30fabee6ba

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 9377411 [details]
Bug 1876931 - Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

see comment 14

The patch is simple, and should be fine to uplift to esr115 early.

Because it blocks some users from sending messages, it might be useful to uplift early.

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9377411 [details]
Bug 1876931 - Detect and ignore unusable S/MIME encryption certificates. r=mkmelin

[Triage Comment]
Approved for esr115

Comment 19

[Daniel Darnell [:dandarnell]]

Thunderbird 115.8.1:
https://hg.mozilla.org/releases/comm-esr115/rev/2da45d2d55c5
https://hg.mozilla.org/releases/comm-esr115/rev/d7ea65041b33