1454504 - switch CertUtils.checkCert away from calling nsIX509Cert.issuer repeatedly

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement, P1)

Description

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

nsIX509Cert.issuer performs synchronous certificate verification and isn't even guaranteed to return a verified result. Fortunately we should be able to use nsISSLStatus.succeededCertChain instead, which contains the already-verified certificate chain of the connection we're interested in. This should be a performance as well as architectural improvement.

Comment 1

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

Attachment: bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert

nsIX509Cert.issuer performs synchronous certificate verification and isn't even
guaranteed to return a verified result. Luckily we can replace this with
nsISSLStatus.succeededCertChain, which contains the already-verified certificate
chain of the connection we're interested in.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8968350 [details]
bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert

https://reviewboard.mozilla.org/r/237028/#review242852

::: toolkit/modules/CertUtils.jsm:146
(Diff revision 1)
>      }
>      return;
>    }
>  
> -  var cert =
> -      aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider).
> +  let sslStatus = aChannel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider)
> +                          .SSLStatus.QueryInterface(Ci.nsISSLStatus);

Nit: QI(nsISSLStatus) is completely unnecessary.

::: toolkit/modules/CertUtils.jsm:155
(Diff revision 1)
>  
> -  if (aAllowNonBuiltInCerts === true)
> +  if (aAllowNonBuiltInCerts === true) {
>      return;
> +  }
>  
> -  var issuerCert = cert;
> +  let certEnumerator = sslStatus.succeededCertChain.getEnumerator();

Random fact: nsISSLStatus.idl is super well-documented.

::: toolkit/modules/CertUtils.jsm:156
(Diff revision 1)
> -  while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert))
> -    issuerCert = issuerCert.issuer;
> +  let issuerCert = null;
> +  while (certEnumerator.hasMoreElements()) {

Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))`

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8968350 [details]
bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert

https://reviewboard.mozilla.org/r/237028/#review243144

r+ with Kris's comments addressed.

Comment 4

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

Comment on attachment 8968350 [details]
bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert

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

Comment 5

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

Comment on attachment 8968350 [details]
bug 1454504 - use a more performant API to find a root certificate in CertUtils.checkCert

https://reviewboard.mozilla.org/r/237028/#review242852

Thanks for the reviews!

> Nit: QI(nsISSLStatus) is completely unnecessary.

Heh - whoops. Fixed.

> Random fact: nsISSLStatus.idl is super well-documented.

Indeed :(

> Nit: `for (issuerCert of XPCOMUtils.IterSimpleEnumerator(certEnumerator, Ci.nsIX509Cert))`

Cool - fixed.

Comment 6

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

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=389e57f8a8090aa6a7eaadedf09343dfc67546f5

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a231670d9c66
use a more performant API to find a root certificate in CertUtils.checkCert r=kmag,mossop

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a231670d9c66

Comment 9

[Victor Carciu]

Please let us know if any testing is needed here or mark the bug as "qe-verify-"

Comment 10

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

If there are existing smoke tests that ensure that updating add-ons and the browser still works, it would be good to confirm that this change doesn't break that. Thanks!

Comment 11

[Victor Carciu]

We made a smoke testing around updates and everything worked as expected. Marking bug as verified.