remove MOZ_NO_EV_CERTS and simplify EV root information loading

RESOLVED FIXED in Firefox 52

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla52
Points:
---

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]
Comment hidden (mozreview-request)
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 4

2 years ago
mozreview-review
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 5

2 years ago
mozreview-review
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

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

Comment 6

2 years ago
mozreview-review
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

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

Comment 7

2 years ago
mozreview-review
Comment on attachment 8800341 [details]
bug 1227638 - deterministically load EV information

https://reviewboard.mozilla.org/r/85262/#review85006
Attachment #8800341 - Flags: review?(mgoodwin) → review+
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

2 years ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/003ec40aa484
deterministically load EV information r=Cykesiopka,mgoodwin
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
Flags: needinfo?(dkeeler)
Depends on: 1311077
Turns out, that test is actually buggy on android. See bug 1311077.
Flags: needinfo?(dkeeler)
Comment hidden (mozreview-request)

Comment 15

2 years ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/681034636358
deterministically load EV information r=Cykesiopka,mgoodwin

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/681034636358
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.