remove MOZ_NO_EV_CERTS and simplify EV root information loading

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox52 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

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.
It is also useful for platforms that don't have an EV indicator, e.g. FirefoxOS.
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.
Whiteboard: [psm-cleanup]
Assignee: nobody → dkeeler
Priority: -- → P1
Summary: consider removing MOZ_NO_EV_CERTS → remove MOZ_NO_EV_CERTS and simplify EV root information loading
Whiteboard: [psm-cleanup] → [psm-assigned]
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"`.
Attachment #8800341 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review84600
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review85004
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review85006
Attachment #8800341 - Flags: review?(mgoodwin) → review+
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/003ec40aa484
deterministically load EV information r=Cykesiopka,mgoodwin
Turns out, that test is actually buggy on android. See bug 1311077.
Flags: needinfo?(dkeeler)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/681034636358
deterministically load EV information r=Cykesiopka,mgoodwin
https://hg.mozilla.org/mozilla-central/rev/681034636358
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.