1319185 - deal with certificate nickname-related API removals in bug 857627

Status: RESOLVED FIXED

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

Description

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

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).

Comment 1

[Jorg K (CEST = GMT+2)]

When do you plan to remove those APIs?

Comment 2

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

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.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1319185-C-plusplus-part.patch

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 4

[Magnus Melin [:mkmelin]]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Part 0: Temporary bustage fix [landed in comment #5]

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

Magnus, I did what you said (and assumed r+). All yours now ;-)

Comment 6

[Jorg K (CEST = GMT+2)]

Bug 857627 has just landed on M-C, so we can compare this run
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=c31824418da3515b1ecfc26d8c9abdfac79e1672
to the next.

Comment 7

[Jorg K (CEST = GMT+2)]

There don't appear to be tests, at least no new test failures seen.

Comment 8

[Magnus Melin [:mkmelin]]

(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.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1319185_smime_cert_nick.patch

I just added usage verification functions to nsICMSSecureMessage. Could be somewhere else too I suppose...

getCertByPrefID was unused, so I remove that too.

Comment 10

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

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).

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug1319185_smime_cert_nick.patch

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.

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[Richard Marti (:Paenglab)]

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

Comment 14

[Jorg K (CEST = GMT+2)]

Doesn't compile for me either, same errors, Magnus?

Comment 15

[Jorg K (CEST = GMT+2)]

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?

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: bug1319185_smime_cert_nick.patch - made to work on M$

This will at least compile ;-)

Comment 17

[Richard Marti (:Paenglab)]

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.

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[Magnus Melin [:mkmelin]]

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.

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Jorg K (CEST = GMT+2)]

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

Comment 22

[Kai Engert (:KaiE:)]

(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.