1227638 - remove MOZ_NO_EV_CERTS and simplify EV root information loading

Status: RESOLVED FIXED

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

Description

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

If I understand correctly, MOZ_NO_EV_CERTS is leftover from when we used NSS' libpkix for EV certificate verification (that is, we disabled EV certificate verification entirely on some platforms because libpkix either didn't work or wasn't performant or some other concern). Now that we use mozilla::pkix for all certificate verifications, this build flag/ifdef seems unnecessary.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It is also useful for platforms that don't have an EV indicator, e.g. FirefoxOS.

Comment 2

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

Good point. Maybe it could be a runtime check instead. That is, we don't want to do unnecessary work if the front-end isn't even going to display it, but I'm not entirely sure that #ifdef blocks are the right way to implement that in this case.

Comment 3

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

Attachment: bug 1227638 - deterministically load EV information

Previously PSM would load EV information on-demand (i.e. just before verifying a
certificate). This simplifies this operation, removes a dubious optimization
(loading the EV information on another thread while opening a network
connection), and relocates the loading operation to when we are likely to have
good disk locality (i.e. when we've just loaded the built-in roots module).

This also removes the now-unused MOZ_NO_EV_CERTS build flag.

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

Comment 4

[:Cykesiopka]

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review84002

Much nicer!

::: security/certverifier/ExtendedValidation.cpp:1271
(Diff revision 1)
>    }
>  
>    for (size_t iEV = 0; iEV < mozilla::ArrayLength(myTrustedEVInfos); ++iEV) {
>      nsMyTrustedEVInfo& entry = myTrustedEVInfos[iEV];
>  
> +    SECStatus rv;

Nit: This function returns `nsresult`, so this should probably be `srv` instead.

::: security/certverifier/ExtendedValidation.cpp:1280
(Diff revision 1)
> +    // actually present in our loaded root certificates module. It is
> +    // unnecessary to check this in non-debug builds since we will safely fall
> +    // back to DV if the EV information is incorrect.
>      mozilla::ScopedAutoSECItem derIssuer;
> -    SECStatus rv = ATOB_ConvertAsciiToItem(&derIssuer, entry.issuer_base64);
> -    PR_ASSERT(rv == SECSuccess);
> +    rv = ATOB_ConvertAsciiToItem(&derIssuer, entry.issuer_base64);
> +    MOZ_ASSERT(rv == SECSuccess, "Could not base64-decode built-in EV issuer");

The `MOZ_ASSERT` here and the ones below will need an `#include "mozilla/Assertions.h"`.

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review84600

Comment 6

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review85004

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review85006

Comment 8

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

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review84002

Thanks for the reviews!

> Nit: This function returns `nsresult`, so this should probably be `srv` instead.

Good call.

> The `MOZ_ASSERT` here and the ones below will need an `#include "mozilla/Assertions.h"`.

Ok - included.

Comment 9

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

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/003ec40aa484
deterministically load EV information r=Cykesiopka,mgoodwin

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for android Cpp test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5179832&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/9fa614d8310d

Comment 12

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

Turns out, that test is actually buggy on android. See bug 1311077.

Comment 13

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

Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

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

Comment 14

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

Ok, that's fixed and try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9122824c2e5

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/681034636358
deterministically load EV information r=Cykesiopka,mgoodwin

Comment 16

[Carsten Book [:Tomcat]]

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