deal with certificate nickname-related API removals in bug 857627

RESOLVED FIXED in Thunderbird 53.0

Status

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: keeler, Assigned: mkmelin)

Tracking

(Blocks 1 bug)

unspecified
Thunderbird 53.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird52 unaffected, thunderbird53 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Bug 857627 will be removing some APIs that comm-central uses. They are:

nsIX509CertDB.findCertByNickname
nsIX509CertDB.findEmailEncryptionCert
nsIX509CertDB.findEmailSigningCert
nsIX509Cert.nickname

Here are the uses of these APIs I could find in comm-central:

https://dxr.mozilla.org/comm-central/rev/b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/content/am-smime.js#266
https://dxr.mozilla.org/comm-central/rev/b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/content/am-smime.js#269
(That whole function looks a bit suspect - it appears to be trying to get two different certificates that nonetheless have the same nickname, which I didn't think was possible. It may be that that code can be deleted entirely.)

https://dxr.mozilla.org/comm-central/rev/b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#917
https://dxr.mozilla.org/comm-central/rev/b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#938
(These appear to be a fallback scheme to be compatible with older versions. At this point, I think it should be fine to move forward with using the more appropriate API, nsIX509CertDB.findCertByDBKey, which that code already does. I think this can just be removed.)

For nsIX509Cert.nickname, there are many uses in this file: https://dxr.mozilla.org/comm-central/source/mailnews/extensions/smime/content/am-smime.js
These can probably all be replaced by the new attribute "displayName", which is a human-readable name for the certificate (note that this isn't a unique identifier - for places that need that, use dbKey).

Along similar lines, it might be a good idea to rework code in comm-central that uses the NSS nickname API (e.g. https://dxr.mozilla.org/comm-central/rev/b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/nsCertPicker.cpp#37 ), since PSM is moving away from exposing them (see again bug 857627).
When do you plan to remove those APIs?
Flags: needinfo?(dkeeler)
Time-wise, as soon as the patches in bug 857627 can be reviewed and landed. Version-wise, it looks like it will be Firefox 53.
Flags: needinfo?(dkeeler)
Posted patch 1319185-C-plusplus-part.patch (obsolete) — Splinter Review
Minimal patch to fix C++ compile bustage once bug 857627 lands.

am-smime.js needs more work: Lines 269 and 269:
Remove certdb.findEmailEncryptionCert() and certdb.findEmailSigningCert() and check usage of 'nickname' and use 'displayName' instead where appropriate.
Comment on attachment 8816239 [details] [diff] [review]
1319185-C-plusplus-part.patch

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

Maybe better to just comment out the code, and also add a comment that it's temporarily removed until bug 1319185 is properly fixed.
https://hg.mozilla.org/comm-central/rev/c31824418da3515b1ecfc26d8c9abdfac79e1672

Magnus, I did what you said (and assumed r+). All yours now ;-)
Attachment #8816239 - Attachment is obsolete: true
Attachment #8816399 - Flags: review+
There don't appear to be tests, at least no new test failures seen.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #0)
> Here are the uses of these APIs I could find in comm-central:
> 
> https://dxr.mozilla.org/comm-central/rev/
> b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/content/
> am-smime.js#266
> https://dxr.mozilla.org/comm-central/rev/
> b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/content/
> am-smime.js#269
> (That whole function looks a bit suspect - it appears to be trying to get
> two different certificates that nonetheless have the same nickname, which I
> didn't think was possible. It may be that that code can be deleted entirely.)

Apparently very long ago the prefs stored cert.nickname, but now they store the cert.dbKey.

So, we have two certificate prefs: which to use for encryption and which to use for signing. When you set up/update the one for encryption, there is UI logic to ask if you want to use the same for signing (and vice versa). I believe the search for the "other cert" is were just used to verify that the other operation could be performed with the certificate you just selected. There were not really another certificate with the same nickname, it's the same certificate, but in the process of calling findEmailEncryptionCert verified to allow use=encryption. 

> https://dxr.mozilla.org/comm-central/rev/
> b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/
> nsMsgComposeSecure.cpp#917
> https://dxr.mozilla.org/comm-central/rev/
> b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/
> nsMsgComposeSecure.cpp#938
> (These appear to be a fallback scheme to be compatible with older versions.
> At this point, I think it should be fine to move forward with using the more
> appropriate API, nsIX509CertDB.findCertByDBKey, which that code already
> does. I think this can just be removed.)

Yes. 

> For nsIX509Cert.nickname, there are many uses in this file:
> https://dxr.mozilla.org/comm-central/source/mailnews/extensions/smime/
> content/am-smime.js
> These can probably all be replaced by the new attribute "displayName", which
> is a human-readable name for the certificate (note that this isn't a unique
> identifier - for places that need that, use dbKey).

I looked through those and changed them to displayName. At least nothing looked obviously wrong...

> Along similar lines, it might be a good idea to rework code in comm-central
> that uses the NSS nickname API (e.g.
> https://dxr.mozilla.org/comm-central/rev/
> b11789455e43844c9023262ddc34b992db99b082/mailnews/extensions/smime/src/
> nsCertPicker.cpp#37 ), since PSM is moving away from exposing them (see
> again bug 857627).

I leave this for another bug.
I just added usage verification functions to nsICMSSecureMessage. Could be somewhere else too I suppose...

getCertByPrefID was unused, so I remove that too.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8818345 - Flags: feedback?(dkeeler)
Comment on attachment 8818345 [details] [diff] [review]
bug1319185_smime_cert_nick.patch

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

This seems reasonable.

::: mailnews/mime/src/nsCMSSecureMessage.cpp
@@ +65,5 @@
>    nsCOMPtr<nsISupports> nssInitialized = do_GetService("@mozilla.org/psm;1", &rv);
>    return rv;
>  }
>  
> +NS_IMETHODIMP nsCMSSecureMessage::CheckUsageOk(

nit: since this isn't implementing an idl function, I would use 'nsresult' instead

@@ +78,5 @@
>  
> +  RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
> +  NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
> +
> +  nsAutoCString dbkey;

dbkey seems unused here

@@ +87,5 @@
> +  if (certVerifier->VerifyCert(aCert->GetCert(),
> +                               aUsage,
> +                               mozilla::pkix::Now(),
> +                               nullptr, nullptr,
> +                               builtChain) == mozilla::pkix::Success) {

Note that unless you specify FLAG_LOCAL_ONLY, this could cause OCSP requests to block whatever thread is calling this (presumably the main thread).
Attachment #8818345 - Flags: feedback?(dkeeler) → feedback+
Maybe you want to rubberstamp this?

On the identity security tab you should get asked if you want to use the same cert for encryption when you choose signing cert. 

I'm not sure if there really are s/mime certs issued that don't allow both signing and encryption, so I haven't tested live the case that only one of them is allowed.
Attachment #8818345 - Attachment is obsolete: true
Attachment #8819526 - Flags: review?(jorgk)
Comment on attachment 8819526 [details] [diff] [review]
bug1319185_smime_cert_nick.patch

Yes, I'd like to rs this ;-) Can you put rs=jorgk when landing?
Since we're currently busted, that will have to wait before landing.
In the meantime, Richard, could you please give this a whirl.
(AFAIK, Richard uses S/MIME.)

Thanks Magnus for tackling this, this fixes the last issue on my "top five" hotlist.
Attachment #8819526 - Flags: review?(jorgk)
Attachment #8819526 - Flags: review+
Attachment #8819526 - Flags: feedback?(richard.marti)
Comment on attachment 8819526 [details] [diff] [review]
bug1319185_smime_cert_nick.patch

Hmm, am I doing something wrong? During build I get:

0:42.64 z:/Mozilla/comm-central/mailnews/mime/src/nsCMSSecureMessage.cpp(145): error C2039: 'SendMessageA': is not a member of 'nsCMSSecure Message'
 0:42.64 z:\mozilla\comm-central\mailnews\mime\src\nsCMSSecureMessage.h(23): note: see declaration of 'nsCMSSecureMessage'
 0:42.65 z:/Mozilla/comm-central/mailnews/mime/src/nsCMSSecureMessage.cpp(171): error C3861: 'decode': identifier not found
 0:42.65 z:/Mozilla/comm-central/mailnews/mime/src/nsCMSSecureMessage.cpp(254): error C3861: 'encode': identifier not found
Doesn't compile for me either, same errors, Magnus?
Flags: needinfo?(mkmelin+mozilla)
I'm sure Magnus compiled this, so by the looks of it, this collides with the Windows function SendMessage which has an A (ANSI) and a W (wide, Unicode) version:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms644950(v=vs.85).aspx

Not sure how to fix it though. Also, ...
private:
  virtual ~nsCMSSecureMessage();
  NS_METHOD encode(const unsigned char *data, int32_t dataLen, char **_retval);
  NS_METHOD decode(const char *data, unsigned char **result, int32_t * _retval);
shouldn't encode and decode be nsresult as their implementation?
This will at least compile ;-)
Attachment #8819526 - Attachment is obsolete: true
Attachment #8819526 - Flags: feedback?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Attachment #8819536 - Flags: review+
Attachment #8819536 - Flags: feedback?(richard.marti)
Comment on attachment 8819536 [details] [diff] [review]
bug1319185_smime_cert_nick.patch - made to work on M$

Yes, without patch it doesn't ask and with it does.
Attachment #8819536 - Flags: feedback?(richard.marti) → feedback+
Magnus, if you're happy with my changes, you or I can land this then, r=jorgk since I did a bit of reviewing here now.

https://bugzilla.mozilla.org/attachment.cgi?oldid=8819526&action=interdiff&newid=8819536&headers=1
Happy, but that's certainly unexpected! Never name your method SendMessage! I also don't really understand why it worked earlier as nsresult. I just changed it to NS_IMETHODIMP to reflect reality, it wasn't strictly necessary.
I don't think it has anything to do with the type of the function, I reverted that and that didn't fix it. I think it has to do with an include you do now.

Clearly, if the C++ preprocessor appends an A to the function name, you're busted.
https://hg.mozilla.org/comm-central/rev/e47002b53b5f9294a331d8b84e291ed6c362fb01
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0

(In reply to comment #0)

(That whole function looks a bit suspect - it appears to be trying to get
two different certificates that nonetheless have the same nickname, which I
didn't think was possible. It may be that that code can be deleted entirely.)

FYI, this it related to a functionality called "dual-key certificates", mentioned here:
https://www-archive.mozilla.org/projects/security/pki/psm/smime_guide.html

With "dual-key certificates", the user creates two key pairs, and obtains two certificates. Both certificates use the same subject, but have different serial numbers, usage, and keys.

If you use a different key for encryption, then a corporate environment could escrow a person's encryption key. However, by using a different key for signing, the escrowed key cannot be used to sign in the person's name.

The reason why the S/MIME preferences allow to configure separate certificates for signing and encryption, is to support dual-key certificates.

By storing the nickname, it was possible to search for the certificate, obtain more than one result, and then select the appropriate one based on the certificate's embedded usage attribute.

The changes made here disabled storage of the nickname.
I don't remember if any part of the S/MIME code relied on that.

We'd have to test if the changes broke support for dual-key certificates in any way.

Blocks: 1549709
You need to log in before you can comment on or make changes to this bug.