1519093 - No prompt for PIN when digitally signing a message with digital certificate on a smart card

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Richard[cz]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

Thunderbird 60.4.0 (32b) on Win 10 x64, a digital certificate on a smart card, the smart card present in a reader, Security Device (.dll) for the particilar smart card loaded in Security Settings, the digital certificate and its root autority certificates loaded (visible) in Thunderbird Security Settings.

Write -> Message -> filled in "To:", "Subject" and "Message"
Security -> Digitally Sign This Message
Send

Actual results:

The message was not sent, Thunderbird gives an error saying
"Sending of the message failed. Unable to sign message. Please check that the certificates specified in Mail & Newsgroups Accounts Settings for this email account are valid and trusted for mail."

Expected results:

To send the message signed with the digital certificate.

Comment 1

[Richard[cz]]

When I enter Thunderbird's Security Settings and use Manage Certificates, a PIN dialog is elevated and available certificate(s) is/are shown. Now if I create a new message, fill in a recipient, subject and message, and choose Security -> Digitally Sign This Message and Send, the message is sent signed.
This shows that all the required components work fine - the smart card, the digital certificate and authority root certificates and security device.
The problem is that Digitally Sign This Message feature doesn't elevate a PIN dialog itself or it doesn't use the Security Device properly. The Security Device must be "woken up" first through Security Settings otherwise it won't work.

Comment 2

[Jorg K (CEST = GMT+2)]

Similar to bug 1517408?

Comment 3

[Richard[cz]]

I'm not sure since I am able to sign emails while guys reporting bug 1517408 are not.

Comment 4

[Jan Horak [:jhorak]]

In Thunderbird 52 the PIN prompt dialog is shown when FindEmailEncryptionCert is called:
if (!mSelfEncryptionCert) {
certdb->FindEmailEncryptionCert(mEncryptionCertName,
getter_AddRefs(mSelfEncryptionCert));
}

This call has been removed by changeset [1][2] in TB 60 and it seems that it has not been replaced by some other code.

We have also a report where customer is unable to use their smart card for signing the email:
https://bugzilla.redhat.com/show_bug.cgi?id=1661651

[1] https://hg.mozilla.org/comm-central/rev/e47002b53b5f9294a331d8b84e291ed6c362fb01#l2.34
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1519093

Comment 5

[Jan Horak [:jhorak]]

Could some nss guys help us here?

Comment 6

[Jan Horak [:jhorak]]

Kai, are you able to help with that?

Comment 7

[Kai Engert [:KaiE:]]

Attachment: 1519093-v1.patch

This patch restores code that was removed in bug 857627, and restores the calls to those APIs that were removed in bug 1319185.

Comment 8

[Kai Engert [:KaiE:]]

This code MIGHT fix it, but I don't have any setup with smart cards, so I cannot test it.

Jan, are you able to test, or get feedback, if this patch fixes the issue?

Comment 9

[Jan Horak [:jhorak]]

Kai, thanks for the patch. After a lot of testing, it seems the problem most likely lies somewhere else. With the nickname removal [1] the code from your patch would work for those who migrated from TB 52 to TB 60 (and not for those who set certificates on TB 60+).

But the problem is in fact in VerifyCert [2] which fails for some certificates (for various reasons it seems - for me it is a unknown issuer) and the removed code saved users from failure (because it does not verify the certificate).

Also the code [3] which deletes the keydb entry without any notice while keeping the signing_cert_name is kind of confusing, because in account settings/Security everything looks reasonable (apart from keydb disappears).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1319185
[2] https://dxr.mozilla.org/comm-central/source/comm/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#914
[3] https://dxr.mozilla.org/comm-central/source/comm/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#922

Comment 10

[Kai Engert [:KaiE:]]

(In reply to Jan Horak [:jhorak] from comment #9)

Kai, thanks for the patch. After a lot of testing, it seems the problem most likely lies somewhere else. With the nickname removal [1] the code from your patch would work for those who migrated from TB 52 to TB 60 (and not for those who set certificates on TB 60+).

But the problem is in fact in VerifyCert [2] which fails for some certificates (for various reasons it seems - for me it is a unknown issuer) and the removed code saved users from failure (because it does not verify the certificate).

Given that the patch can help some users, and that it helped you to uncover other problems, it might be reasonable to restore that old code.

Also the code [3] which deletes the keydb entry without any notice while keeping the signing_cert_name is kind of confusing, because in account settings/Security everything looks reasonable (apart from keydb disappears).

I don't know/remember the exact intention of that code, but here's a possible explanation:

A DB key identifies one specific certificate. It might have been valid and usable in the past, but might no longer be. So it's reasonable to stop using it, once that is identified.

The settings store both a db key and a nickname. The same nickname might potentially refer to a renewed certificate, which can be found by using the functions that search by nickname. So it seems reasonable to keep that as a fallback option.

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 9060821 [details] [diff] [review]
1519093-v1.patch

Magnus, you had removed parts of this code in bug 1319185, I suggest to restore it.

@jorgk: Commit comment: "Restore code (that was removed in bug 1319185) to lookup configured S/MIME certificates by nickname".

Comment 12

[Jan Horak [:jhorak]]

Kai, the problem is that nickname is no longer stored to the identity db [1], the displayName is used instead. And looking up cert by displayName does not work, see PK11_FindCertFromNickname and subsequent find_certs_from_nickname. So that would solve problems for users which set certificate in TB 52 and updated to 60, but not for those who set/changed the certificate in 60.

[1] https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/extensions/smime/content/am-smime.js#87

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9060821 [details] [diff] [review]
1519093-v1.patch

Review of attachment 9060821 [details] [diff] [review]:
-----------------------------------------------------------------

I guess if it helps some users we could take it, but it would need some more inline comments explaining what jhorak describes

::: mailnews/compose/public/nsIMsgComposeSecure.idl
@@ +93,5 @@
> +
> +    /**
> +     *  Given a nickname,
> +     *  locate the matching certificate.
> +     *

should fit on one line

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +1250,5 @@
> +
> +NS_IMETHODIMP
> +nsMsgComposeSecure::FindEmailEncryptionCert(const nsAString& aNickname,
> +                                            nsIX509Cert** _retval)
> +{

This and ::FindEmailSigningCert look the same except for the one line. Could you parametrize that and use an internal helper

@@ +1262,5 @@
> +  char *asciiname = nullptr;
> +  NS_ConvertUTF16toUTF8 aUtf8Nickname(aNickname);
> +  asciiname = const_cast<char*>(aUtf8Nickname.get());
> +
> +  /* Find a good cert in the user's database */

nit: prefer // comments

Comment 14

[Kai Engert [:KaiE:]]

(In reply to Jan Horak [:jhorak] from comment #12)

Kai, the problem is that nickname is no longer stored to the identity db [1], the displayName is used instead. And looking up cert by displayName does not work, see PK11_FindCertFromNickname and subsequent find_certs_from_nickname. So that would solve problems for users which set certificate in TB 52 and updated to 60, but not for those who set/changed the certificate in 60.

Thanks for pointing that out, I had missed that. That obsoletes my patch.

(Given that Thunderbird 52.x is no longer supported, and that lifetime period of Thunderbird 60.x is almost over, we must assume that the majority of users has already migrated to TB 60.x. Now we can no longer distinguish which users have stored nicknames and which have stored display names in that pref. That means that pref has become irrelevant. It's no longer used, we should probably remove it. In the past it was probably used for dual-key certificates, see bug 1549709.)

Back to this bug. So it's not about certain certificates not being found, but about the PIN prompt not showing up.

In order for the prompt to work, NSS needs to be given a context pointer to the windowing system.

The old, removed API (find-by-nickname) did do that. The remaining API (find-by-dbkey) doesn't.

We need to change the lookup code to use an API that passes the UI context.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: 1519093-v2.patch

It might be as simple as this.

(This change is in general Firefox code.)

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 9063200 [details] [diff] [review]
1519093-v2.patch

Pass the interface callback when searching by database key (=issuer/sn).

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9063200 [details] [diff] [review]
1519093-v2.patch

Review of attachment 9063200 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/nsNSSCertificateDB.cpp
@@ +140,5 @@
>    reader += issuerLen;
>    MOZ_ASSERT(reader == decoded.EndReading());
>  
> +  nsCOMPtr<nsIInterfaceRequestor> cxt = new PipUIContext();
> +  cert.reset(CERT_FindCertByIssuerAndSNCX(CERT_GetDefaultCertDB(), &issuerSN, cxt));

`CERT_FindCertByIssuerAndSNCX` calls `common_FindCertByIssuerAndSN`, which calls `PK11_FindCertByIssuerAndSN`, passing along the context. However, `PK11_FindCertByIssuerAndSN` does nothing with it. So how will adding this help?

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #17)

PK11_FindCertByIssuerAndSN does nothing with it. So how will adding this help?

It won't help. :(
I trusted the parameter would be used, I should have checked.

Comparing with code in PK11_FindCertFromNickname, we probably must enhance PK11_FindCertByIssuerAndSN to iterate all tokens, search by issuer/SN on each of them, and call pk11_AuthenticateUnfriendly as needed.

The developer working on this should have a smartcard setup to test it.

Comment 19

[Jan Horak [:jhorak]]

(In reply to Kai Engert (:kaie:) from comment #14)

Back to this bug. So it's not about certain certificates not being found, but about the PIN prompt not showing up.

In order for the prompt to work, NSS needs to be given a context pointer to the windowing system.

The old, removed API (find-by-nickname) did do that. The remaining API (find-by-dbkey) doesn't.

We need to change the lookup code to use an API that passes the UI context.

Ah, I see.

The developer working on this should have a smartcard setup to test it.

I use following setup to emulate virtual smart card by using softhsm:

sudo dnf install softhsm
sudo usermod -a -G ods $USER
newgrp ods

softhsm2-util --init-token --free --label "Mine soft token"
<set pins>
openssl req -newkey rsa:2048 -new -nodes -x509 -days 3650 -keyout key.pem -out cert.pem
softhsm2-util --import key.pem --token "Mine soft token" --label aKey --id a1b1
p11tool --list-all 
# get the <SOFT_HSM_URL> from the output of previous command
# look for something like: pkcs11:model=SoftHSM%20v2;manufacturer=SoftHSM%20project;serial=c8d74ac3c4cca8df;token=Mine%20soft%20token
p11tool --login --write "<SOFT_HSM_URL>" --load-certificate cert.pem

Now after TB restart and assuring the new ods group is active I'm able to select certificate in the account settings/Security. In TB 52 I also get a PIN prompt when sending signed message when not previously logged in.

Comment 20

[Jan Horak [:jhorak]]

Attachment: pin-prompt-when-sending.patch

Attaching simple patch, which shows pin prompt for not already authenticated tokens before sending the message. It would be better to use pk11_TraverseAllSlots instead but that seems to be a private nss symbol.

Comment 21

[Kai Engert [:KaiE:]]

I think we should reach out to either Daiki Ueno or Bob Relyea to confirm that this code achieves the intended effect.

Instead of adding this code locally to Thunderbird, we should consider to add this functionality with a new exported function to NSS?

E.g. it could be called PK11_EnsureAllTokensAreLoggedIn

Also, the third parameter to that function is for a window-context parameter, which is probably required by NSS to open prompts for a PIN, so probably it shouldn't be null?

If yes, that new function probably requires this parameter, too.

Comment 22

[Daiki Ueno [:ueno]]

I don't have much knowledge of this part of the code, but my assumption is that NSS calls pk11_TraverseAllSlots() to ensure that all the tokens are authenticated. For example, it is called from CERT_GetCertNicknames() etc.

Doesn't it help if you call one of those public functions?

Comment 23

[Kai Engert [:KaiE:]]

Good idea, CERT_GetCertNicknames() takes a window parameter, that seems like an improvement over the attached patch.

Jan, I'm still unsure how the patch you had attached helps you. Did you get any prompts for the not-yet-logged-in smartcard with your patch?

Jan, would you be able to test Daiki's alternative? Instead of "PK11_TraverseSlotCerts":

// Calling CERT_GetCertNicknames has the desired side effect of
// traversing all tokens, and brining up prompts to unlock.
nsCOMPtr<nsIInterfaceRequestor> cxt = new PipUIContext();
CERTCertNicknames *result_unused =
CERT_GetCertNicknames(CERT_GetDefaultCertDB(),
SEC_CERT_NICKNAMES_USER, cxt);
CERT_FreeNicknames(result_unused);

Comment 24

[Jan Horak [:jhorak]]

Attachment: pin-prompt-when-sending2.patch

Yes, I get the prompt shows up even with nullptr as the context. The code proposed by you/Daiki also works. Attaching the updated patch, please feel free to add reviewer for this one.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: 1519093-v5.patch

Dana, would you mind to have a final look? Thanks in advance.

(fixed indent and my typo in comment, prepared for landing by Jörg)

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9067356 [details] [diff] [review]
1519093-v5.patch

Review of attachment 9067356 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 27

[Jorg K (CEST = GMT+2)]

This doesn't link on Windows: 0:23.11 lld-link: error: undefined symbol: CERT_GetCertNicknames

Comment 28

[Kai Engert [:KaiE:]]

Attachment: 1519093-export.patch

Jörg, thanks for checking, this additional patch should fix it.

Dana, thanks for the r+.

I suggest to uplift this to beta, it seems like a helpful fix for smartcard users.

[Approval Request Comment]
Regression caused by (bug #): 1319185
User impact if declined: Smartcard users have trouble signing S/MIME emails
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): very little

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 9068128 [details] [diff] [review]
1519093-export.patch

Thanks.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f965e7f4a1aa
prompt for smartcard PIN when S/MIME signing. r=keeler
https://hg.mozilla.org/comm-central/rev/df4dd9bfa02d
export CERT_GetCertNicknames. r=jorgk

Comment 31

[Richard[cz]]

Thank you all for dealing with this. Looking forward to TB69 to test it.

Comment 32

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/afa49d83a174bd45330797f798a10afdc4dd0510
https://hg.mozilla.org/releases/comm-beta/rev/736c33cdca9ec536b9bc3125374016535520d1a1

Comment 33

[Kai Engert [:KaiE:]]

Attachment: 1519093-uplift-esr60.patch

I wonder if we should uplift to 60.x. Untested patch.

Comment 34

[Jorg K (CEST = GMT+2)]

If you don't request it, we won't ;-) - We can build a version on try if you or someone else has a way to test it.

Comment 35

[Kai Engert [:KaiE:]]

It sounds like you're potentially open to the idea :) The last comments in bug 531073 had inspired me to it, as a related problem that was mentioned in bug 531073 comment 36 sounds similar to this bug.
I've started a local build, to check if it actually builds successfully. I can do a try next.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1579379c18bb
Follow-up: Reformat. rs=reformat DONTBUILD

Comment 37

[Richard[cz]]

Are you suggesting that a patch for current 60.x build can be prepared? If so, I'm in to test it on my setup with TB 60.7.0!

Comment 38

[Kai Engert [:KaiE:]]

Richard, the potential patch for TB 60.x is attached above, comment 33. I was unable to test it. My attempt to build it on my dev system failed with Rust errors. Maybe the Rust toolchain on my system is too new and the Rust syntax has changed? If you could test it, please do.

Comment 39

[Jorg K (CEST = GMT+2)]

Richard, I can do you a try build. Which platform do you need?

Comment 40

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/84cbd72e793ac2877a9fc4bf305e917dfb70f235 (reformat)

Comment 41

[Richard[cz]]

(In reply to Jorg K (GMT+2) from comment #39)

Richard, I can do you a try build. Which platform do you need?

Hi Jorg, my specs are Windows 10 x64 1809, TB 60.7.0 (32-bit)

Comment 42

[Jorg K (CEST = GMT+2)]

Building unofficial 60.7.1 32bit with the patch here now, will advise when ready:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c898b52bf87d93e12272550a70717d38088872f

Comment 43

[Jorg K (CEST = GMT+2)]

I forgot about this, here's your installer:
https://queue.taskcluster.net/v1/task/ZJj_CyIUQwWRN-aPWgwC6w/runs/0/artifacts/public/build/install/sea/target.installer.exe

This in an unofficial TB 60.7.1 which is 60.7.0 plus the small fix from this bug. It looks like a Daily, but it is an ESR version.

Comment 44

[Richard[cz]]

Thank you @Jorg K, I've just tried it and it works as it should! I'll test it more next week.

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 9069166 [details] [diff] [review]
1519093-uplift-esr60.patch

Thanks for testing, small patch I'll take it for TB 60.8 in July. Or earlier, should we do a security release 60.7.1 for other reasons.

Comment 46

[Jorg K (CEST = GMT+2)]

TB 60.7.1 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/a819a3a05f2109a7704d76267db248aaa972212f