1296214 - Stop storing handle to CERTCertificate in ExtendedValidation.cpp

Status: RESOLVED FIXED

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

Description

[:Cykesiopka]

(David Keeler [:keeler] (use needinfo?) from Bug 1289455 comment #2)
> I was thinking about making it so the array
> of nsMyTrustedEVInfo doesn't even need to keep a handle on each root
> certificate (comparing serial number, issuer, and sha-256 hash should be
> sufficient). This might save us a little bit of memory.

Poking around, this seems possible, but requires CertIsAuthoritativeForEVPolicy() to not use CERT_CompareCerts() here: https://hg.mozilla.org/mozilla-central/annotate/fe895421dfbe1f1f8f1fc6a39bb20774423a6d74/security/certverifier/ExtendedValidation.cpp#l1299, and instead do something like construct a SHA-256 hash of |cert| and compare it with |entry.ev_root_sha256_fingerprint|.

Whether this is still a net gain, I'm not sure.

Comment 1

[:Cykesiopka]

Attachment: bug1296214_stop-storing-certcertificate-extendedvalidation.cpp_WIPv1.patch

Here's a WIP patch containing the work I did exploring this bug.
It passes xpcshell tests, but I haven't verified that it's semantically correct otherwise.

Comment 2

[:Cykesiopka]

Attachment: bug1296214_stop-storing-certcertificate-extendedvalidation.cpp_WIPv2.patch

Unbitrotten.

Again, passes xpcshell tests, but I haven't put much further thought into confirming this is correct.

Comment 3

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

Comment on attachment 8797662 [details] [diff] [review]
bug1296214_stop-storing-certcertificate-extendedvalidation.cpp_WIPv2.patch

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

::: security/certverifier/ExtendedValidation.cpp
@@ +1229,5 @@
>    const SECOidData* cabforumOIDData = SECOID_FindOIDByTag(sCABForumEVOIDTag);
>    for (const nsMyTrustedEVInfo& entry : myTrustedEVInfos) {
> +    // XXX: Without a check that ensures |cert| corresponds to an entry
> +    // in myTrustedEVInfos (in addition to matching OIDs), tests fail. Maybe
> +    // this check can be removed somehow, maybe not.

I think this check is important. Suppose there's a root "FooCorp Root 1" that's in Mozilla's CA program, and that we trust it for EV for a certain OID. Then, suppose FooCorp wants to move to a new root and wants it to eventually be trusted for that same OID (but not right away). Without some sort of check that the given root is identical to one that's in the EV list, there's no way to trust the new root without also enabling it for EV (again, given that the OIDs are the same).

Comment 4

[:Cykesiopka]

Attachment: Bug 1296214 - Stop storing handle to CERTCertificate in ExtendedValidation.cpp.

This may save us some memory and reduce the number of static constructors.

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

Comment 5

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

Comment on attachment 8798022 [details]
Bug 1296214 - Stop storing handle to CERTCertificate in ExtendedValidation.cpp.

https://reviewboard.mozilla.org/r/83624/#review82308

Great - r=me. Just the one nit about being consistent with namespaces.

::: security/certverifier/ExtendedValidation.cpp:1234
(Diff revision 1)
>    const SECOidData* cabforumOIDData = SECOID_FindOIDByTag(sCABForumEVOIDTag);
>    for (const nsMyTrustedEVInfo& entry : myTrustedEVInfos) {
> -    if (entry.cert && CERT_CompareCerts(cert.get(), entry.cert.get())) {
> +    // This check ensures that only the specific roots we approve for EV get
> +    // that status, and not certs (roots or otherwise) that happen to have an
> +    // OID that's already been approved for EV.
> +    if (!PodEqual(fingerprint, entry.ev_root_sha256_fingerprint)) {

Oh, huh - looks like the mozilla:: prefix wasn't necessary for the PodEqual calls I added. Let's just be consistent one way or another here.

Comment 6

[:Cykesiopka]

Comment on attachment 8798022 [details]
Bug 1296214 - Stop storing handle to CERTCertificate in ExtendedValidation.cpp.

https://reviewboard.mozilla.org/r/83624/#review82308

Thanks!

> Oh, huh - looks like the mozilla:: prefix wasn't necessary for the PodEqual calls I added. Let's just be consistent one way or another here.

Yeah, turns out half of the code in this file is under the mozilla::psm namespace, while the other half isn't.
I filed Bug 1308143 for cleaning up stuff like this.

Comment 7

[:Cykesiopka]

Comment on attachment 8798022 [details]
Bug 1296214 - Stop storing handle to CERTCertificate in ExtendedValidation.cpp.

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

Comment 8

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9a2de9692985d8aab07b20707e584ed7ad48ed
(The orange looks like an intermittent.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f033b5b7fd6
Stop storing handle to CERTCertificate in ExtendedValidation.cpp. r=keeler

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f033b5b7fd6