1284946 - remove nsIX509Cert.getUsagesArray, requestUsagesArrayAsync, and getUsagesString

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

nsIX509Cert.getUsagesArray, requestUsagesArrayAsync, and getUsagesString are all architecturally problematic (they can cause certificate verifications on the main thread, they use needlessly complicated threading infrastructure for non-main-thread verifications, etc.) and are not very precise (for one, even if a certificate verifies successfully as a web server certificate, this API tells us nothing about if it would verify successfully for a particular hostname, which may be relevant for a web browser).

The (hopefully) soon-to-land nsIX509CertDB.asyncVerifyCertAtTime is more powerful, more architecturally manageable, and enforces that verifications happen off the main thread. This is what should be used instead.

Comment 1

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

Attachment: bug 1284946 - fix dialog parameter passing in browser_certViewer.js

The changes in bug 1217602 missed that browser_certViewer.js should have been
updated to use a nsIDialogParamBlock instead of a (mock) nsIPKIParamBlock.
"Luckily" the test harness completely ignored the errors resulting from this
oversight.

Review commit: https://reviewboard.mozilla.org/r/62868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62868/

Comment 2

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

Attachment: bug 1284946 - remove usages-related APIs from nsIX509Cert

nsIX509Cert provided the APIs getUsagesArray, requestUsagesArrayAsync, and
getUsagesString. These APIs were problematic in that the synchronous ones would
cause certificate verification to block the main thread and the asynchronous one
was needlessly indirect in its definition (it made use of two additional
special-case xpidl types) and needlessly complex in its implementation (it
required nsNSSComponent to manually manage a background thread without the aid
of recent improvements in that area (e.g. CryptoTask)). Furthermore, these APIs
would return string descriptions of the usages the certificate in question had
been verified for rather than using more concrete identifiers or values. This
paradigm is usable but imprecise. The new nsIX509CertDB API
asyncVerifyCertAtTime is much more expressive, enforces off-main-thread
computation, and makes use of CryptoTask for a simple implementation. Using this
API, previous uses of the old nsIX509Cert APIs can be replaced. As an additional
benefit, this removes a ton of obsolete C++ code.

Review commit: https://reviewboard.mozilla.org/r/62968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62968/

Comment 3

[:Cykesiopka]

Comment on attachment 8769332 [details]
bug 1284946 - fix dialog parameter passing in browser_certViewer.js

https://reviewboard.mozilla.org/r/62868/#review60294

Nice catch.
There is one more error that occurs even with this patch applied, but it's due to how viewCertDetails.js is written, and goes away with part 2, so all is good.

Comment 4

[:Cykesiopka]

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

https://reviewboard.mozilla.org/r/62968/#review60538

\o/ Nice!

::: security/manager/pki/resources/content/viewCertDetails.js:63
(Diff revision 1)
> - * @param {Node} verifyInfoBox
> - *        Parent node to append to.
>   */
> -function AddUsage(usage, verifyInfoBox)
> +function AddUsage(usage)
>  {
> +  let verifyInfoBox = document.getElementById('verify_info_box');

Nit: Double quotes.

::: security/manager/pki/resources/content/viewCertDetails.js:122
(Diff revision 1)
> +  certificateUsageEmailSigner,
> +  certificateUsageEmailRecipient,
> +  certificateUsageObjectSigner,
> +};
> +
> +// Map of certificate usage name to localization identifier.

Looks like the VerifyCAVerifier and VerifyStatusResponder l10n entries can be removed now - I assume certificateUsageStatusResponder isn't present here because it's of low value?.
(And I think it might be possible to remove certificateUsageStatusResponder handling in CertVerifier as well.)

::: security/manager/pki/resources/content/viewCertDetails.js:153
(Diff revision 1)
> + * @param {nsIX509Cert} cert
> + *        The certificate to determine valid usages for.
> + */
> +function asyncDetermineUsages(cert) {
> +  let promises = [];
> +  let now = (new Date()).getTime() / 1000;

Nit: This can just be |Date.now() / 1000|.

::: security/manager/pki/resources/content/viewCertDetails.js:161
(Diff revision 1)
> +  Object.keys(certificateUsages).forEach(usageString => {
> +    promises.push(new Promise((resolve, reject) => {
> +      let usage = certificateUsages[usageString];
> +      certdb.asyncVerifyCertAtTime(cert, usage, 0, null, now,
> +        (aPRErrorCode, aVerifiedChain, aHasEVPolicy) => {
> +          resolve({ usageString: usageString, errorCode: aPRErrorCode });

This can just be |usageString, errorCode: aPRErrorCode| if preferred.

::: security/manager/pki/resources/content/viewCertDetails.js:173
(Diff revision 1)
> +/**
> + * Updates the usage display area given the results from asyncDetermineUsages.
> + * @param {Array} results
> + *        An array of objects with the properties "usageString" and "errorCode".
> + *        usageString is a string that is a key in the certificateUsages map.
> + *        errorCode is either 0 (indicating success) or an NSPR error code.

Nit: I guess this should say PRErrorCodeSuccess instead of 0 if we're going to introduce PRErrorCodeSuccess at all.

::: security/manager/pki/resources/content/viewCertDetails.js:178
(Diff revision 1)
> +  let someSuccess = results.some(result => {
> +    return result.errorCode == PRErrorCodeSuccess;
> +  });

ESLint complains about this block.

::: security/manager/pki/resources/content/viewCertDetails.js:213
(Diff revision 1)
> +      let errorPresent = results.some(result => {
> +        return result.errorCode == errorRanking.error;
> +      });

ESLint complains about this block.

::: security/manager/pki/resources/content/viewCertDetails.js:227
(Diff revision 1)
> +      verifystr = bundle.getString("certNotVerified_Unknown");
> +    }
> +    verified.textContent = verifystr;
> +  }
> +  // Notify tests that we are done determining the certificate's valid usages.
> +  Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService)

Nit: I think this can be just |Services.obs|.

::: security/manager/ssl/nsCertTree.cpp
(Diff revision 1)
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyRevoked", _retval);
> -        break;
> -      case nsIX509Cert::CERT_EXPIRED:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyExpired", _retval);
> -        break;
> -      case nsIX509Cert::CERT_NOT_TRUSTED:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyNotTrusted", _retval);
> -        break;
> -      case nsIX509Cert::ISSUER_NOT_TRUSTED:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyIssuerNotTrusted", _retval);
> -        break;
> -      case nsIX509Cert::ISSUER_UNKNOWN:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyIssuerUnknown", _retval);
> -        break;
> -      case nsIX509Cert::INVALID_CA:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyInvalidCA", _retval);
> -        break;
> -      case nsIX509Cert::SIGNATURE_ALGORITHM_DISABLED:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyDisabledAlgorithm", _retval);
> -        break;
> -      case nsIX509Cert::NOT_VERIFIED_UNKNOWN:
> -      case nsIX509Cert::USAGE_NOT_ALLOWED:
> -      default:
> -        rv = mNSSComponent->GetPIPNSSBundleString("VerifyUnknown", _retval);

Looks like all the l10n entries listed here can be removed as well.

::: security/manager/ssl/nsNSSCertificate.cpp
(Diff revision 1)
> -    return NS_ERROR_NOT_AVAILABLE;
> -
> -  nsresult rv;
> -  const int max_usages = 13;
> -  char16_t* tmpUsages[max_usages];
> -  const char* suffix = "_p";

I think the removal of this code means we can get rid of l10n entries like VerifySSLClient_p as well.

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:150
(Diff revision 1)
> +  return new Promise((resolve, reject) => {
> +    OS.File.read(getTestFilePath(filename)).then(data => {
> +      let decoder = new TextDecoder();
> +      let pem = decoder.decode(data);
> -  let certdb = Cc["@mozilla.org/security/x509certdb;1"]
> +      let certdb = Cc["@mozilla.org/security/x509certdb;1"]
> -                 .getService(Ci.nsIX509CertDB);
> +                     .getService(Ci.nsIX509CertDB);
> -  let certList = certdb.getCerts();
> -  let enumerator = certList.getEnumerator();
> -  ok(enumerator.hasMoreElements(), "we have at least one certificate");
> -  let cert = enumerator.getNext().QueryInterface(Ci.nsIX509Cert);
> -  ok(cert, "found a certificate to look at");
> -  info("looking at certificate with nickname " + cert.nickname);
> +      let base64 = pemToBase64(pem);
> +      certdb.addCertFromBase64(base64, trustString, "unused");
> +      let cert = certdb.constructX509FromBase64(base64);
> +      certificates.push(cert); // so we remember to delete this at the end
> +      resolve(cert);
> +    }, reject);
> +  });

I think we can avoid explicitly wrapping in a new promise here:
> return OS.File.read(getTestFilePath(filename)).then(data => {
>   [...]
>   return cert;
> }, () => {
>   throw new Error("Couldn't read certificate");
> });
... or something like that. Doesn't really matter either way though.

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:202
(Diff revision 1)
> + * viewer has opened is displaying that certificate, and has finished
> + * determining its valid usages.
> + *
> + * @param {nsIX509Cert} certificate
> + *        The certificate to view and determine usages for.
> + * @param A promise that will resolve with a handle on the opened certificate

@return {Promise} ...

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:214
(Diff revision 1)
> -  let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
> +    let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
> -                 .createInstance(Ci.nsIDialogParamBlock);
> +                   .createInstance(Ci.nsIDialogParamBlock);
> -  params.objects = array;
> +    params.objects = array;
> -  gBugWindow = window.openDialog("chrome://pippki/content/certViewer.xul",
> -                                 "", "", params);
> -  gBugWindow.addEventListener("load", onLoad);
> +    let win = window.openDialog("chrome://pippki/content/certViewer.xul", "",
> +                                "", params);
> +    whenObserved("ViewCertDetails:CertUsagesDone").then(resolve);

It looks like this line and associated code can be replaced with something like:
> TestUtils.topicObserved("ViewCertDetails:CertUsagesDone").then(subAndData => {
>   let subject = subAndData[0];
>   resolve(subject);
> });

This will involve adding a new global to testing/mochitest/browser.eslintrc, but I'm fairly confident this is correct.
See https://hg.mozilla.org/mozilla-central/file/eafefe59f701/testing/mochitest/browser-test.js#l162

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:229
(Diff revision 1)
> + * @return An array of strings describing the usages the certificate is valid
> + *         for.
> + */
> +function getUsages(win) {
> +  let determinedUsages = [];
> +  let verifyInfoBox = win.document.getElementById("verify_info_box");

Hmm, the first thing I think of here is CPOWs and e10s, but I guess since then cert viewer is displayed in the parent it doesn't matter?

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:303
(Diff revision 1)
> + *
> + * @param {window} win
> + *        The certificate viewer window.
> + * @return A promise that will resolve with no value when the window has closed.
> + */
> +function closeWindow(win) {

It looks like BrowserTestUtils.closeWindow() already exists for doing this.

Comment 5

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

https://reviewboard.mozilla.org/r/62968/#review60538

Thanks!

> Nit: Double quotes.

Fixed.

> Looks like the VerifyCAVerifier and VerifyStatusResponder l10n entries can be removed now - I assume certificateUsageStatusResponder isn't present here because it's of low value?.
> (And I think it might be possible to remove certificateUsageStatusResponder handling in CertVerifier as well.)

Good call. I removed certificateUsageStatusResponder because manually verifying for that usage doesn't exactly match how mozilla::pkix actually verifies OCSP responder certificates when verifying an OCSP response, so there's a potential for misleading results. Also, Firefox isn't meant to be a general-purpose CA tool, so it doesn't make much sense to test that usage. If a user wanted to know whether or not OCSP responses signed by a particular responder will verify correctly in Firefox, the best way to do this would be to set up a server that staples a response and see if Firefox can connect successfully. (I'm just sort-of laying out my reasoning here for posterity and anyone else following along.)

Since this bug already changes a fair bit of code, I'm going to leave the CertVerifier changes to bug 1257362.

> Nit: This can just be |Date.now() / 1000|.

Oh yeah. Fixed.

> This can just be |usageString, errorCode: aPRErrorCode| if preferred.

I kept this as-is for symmetry.

> Nit: I guess this should say PRErrorCodeSuccess instead of 0 if we're going to introduce PRErrorCodeSuccess at all.

Sounds good.

> ESLint complains about this block.

Ah - fixed.

> ESLint complains about this block.

Fixed.

> Nit: I think this can be just |Services.obs|.

Fixed.

> Looks like all the l10n entries listed here can be removed as well.

Good call. I found some others as well.

> I think the removal of this code means we can get rid of l10n entries like VerifySSLClient_p as well.

Sounds good.

> I think we can avoid explicitly wrapping in a new promise here:
> > return OS.File.read(getTestFilePath(filename)).then(data => {
> >   [...]
> >   return cert;
> > }, () => {
> >   throw new Error("Couldn't read certificate");
> > });
> ... or something like that. Doesn't really matter either way though.

Ok - sounds good.

> @return {Promise} ...

Fixed.

> It looks like this line and associated code can be replaced with something like:
> > TestUtils.topicObserved("ViewCertDetails:CertUsagesDone").then(subAndData => {
> >   let subject = subAndData[0];
> >   resolve(subject);
> > });
> 
> This will involve adding a new global to testing/mochitest/browser.eslintrc, but I'm fairly confident this is correct.
> See https://hg.mozilla.org/mozilla-central/file/eafefe59f701/testing/mochitest/browser-test.js#l162

Ah, cool. I probably should have assumed something like this existed.

> Hmm, the first thing I think of here is CPOWs and e10s, but I guess since then cert viewer is displayed in the parent it doesn't matter?

Right - this should all be in the parent process, so I don't think there's a concern here.

> It looks like BrowserTestUtils.closeWindow() already exists for doing this.

I definitely should have assumed something like this existed.

Comment 6

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

Comment on attachment 8769332 [details]
bug 1284946 - fix dialog parameter passing in browser_certViewer.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62868/diff/1-2/

Comment 7

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

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62968/diff/1-2/

Comment 8

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

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

Felipe, if you could review the change to testing/mochitest/browser.eslintrc, that would be great - thanks!

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

https://reviewboard.mozilla.org/r/62968/#review60818

r+ for .eslintrc

Comment 10

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

https://reviewboard.mozilla.org/r/62968/#review61134

I appreciate the benefit of seeing this code after Cykesiopka has already been through it.

I dug, but really it LGTM. There's one nit on a comment.

::: security/manager/pki/resources/content/viewCertDetails.js:227
(Diff revision 2)
> +    if (!verifystr) {
> +      verifystr = bundle.getString("certNotVerified_Unknown");
> +    }
> +    verified.textContent = verifystr;
> +  }
> +  // Notify tests that we are done determining the certificate's valid usages.

Nit: This is really notifying any listener, right? As this is used outside of tests.

Comment 11

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

https://reviewboard.mozilla.org/r/62968/#review61134

Thanks!

> Nit: This is really notifying any listener, right? As this is used outside of tests.

Good call - I clarified that while it will notify any code listening for it, it's really intended for tests.

Comment 12

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

Comment on attachment 8769332 [details]
bug 1284946 - fix dialog parameter passing in browser_certViewer.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62868/diff/2-3/

Comment 13

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

Comment on attachment 8769333 [details]
bug 1284946 - remove usages-related APIs from nsIX509Cert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62968/diff/2-3/

Comment 14

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

Thanks for the reviews!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/595bfede9d26
fix dialog parameter passing in browser_certViewer.js r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/3d09e3895406
remove usages-related APIs from nsIX509Cert r=Cykesiopka,Felipe,jcj

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/595bfede9d26
https://hg.mozilla.org/mozilla-central/rev/3d09e3895406

Comment 17

[Loic]

As some add-ons are affected by this removal of APIs (like add-on signTextJS in bug 1309690), could we add a note in the dev notes for FF50[0], please. 

[0] https://developer.mozilla.org/en-US/Firefox/Releases/50

Comment 18

[Andreas Wagner [:TheOne] [use NI]]

This was convered months ago in https://blog.mozilla.org/addons/2016/09/01/compatibility-for-firefox-50/