1243923 - add support for CAB forum EV certificatePolicies OID (2.23.140.1.1)

Status: RESOLVED FIXED

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

Description

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

(Rick Andrews from bug 1242086 comment 0:)
> 2) Firefox should add the CA/Browser Forum EV certificatePolicies OID
> (2.23.140.1.1) to the list of EV certificatePolicies OIDs expected in valid
> EV certificates. This should be done for all roots capable of issuing EV
> certificates. For the next few years, Firefox should expect either OID (the
> EV certificatePolicies OID currently configured for each root, plus the
> CA/Browser Forum EV certificatePolicies OID).

Comment 1

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

Attachment: bug 1243923 - add support for the CA/Browser Forum EV OID

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

Comment 2

[:Cykesiopka]

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

https://reviewboard.mozilla.org/r/80878/#review80224

Looks good, assuming the .certspec issue is fixed.

::: security/certverifier/ExtendedValidation.cpp:1307
(Diff revision 1)
>  
>    for (const nsMyTrustedEVInfo& entry : myTrustedEVInfos) {
>      if (entry.cert && CERT_CompareCerts(cert.get(), entry.cert.get())) {
> +      const SECOidData* cabforumOIDData = SECOID_FindOIDByTag(sCABForumEVOIDTag);
> +      if (cabforumOIDData && cabforumOIDData->oid.len == policy.numBytes &&
> +          !memcmp(cabforumOIDData->oid.data, policy.bytes, policy.numBytes)) {

Consider using mozilla::PodEqual() instead (mainly because it seems a bit safer).

::: security/certverifier/ExtendedValidation.cpp:1326
(Diff revision 1)
>  static PRStatus
>  IdentityInfoInit()
>  {
> +  mozilla::ScopedAutoSECItem cabforumOIDItem;
> +  if (SEC_StringToOID(nullptr, &cabforumOIDItem, "2.23.140.1.1", 0)
> +      != SECSuccess) {

Nit: Indent this line two spaces further.

::: security/manager/ssl/tests/unit/test_ev_certs/test-and-cabforum-oid-ee-path-ee.pem.certspec:4
(Diff revision 1)
> +issuer:test-and-cabforum-oid-ee-path-int
> +subject:test-and-cabforum-oid-ee-path-ee
> +extension:authorityInformationAccess:http://www.example.com:8888/test-and-cabforum-oid-ee-path-ee/
> +extension:certificatePolicies:2.23.140.1.1,1.3.6.1.4.1.13769.666.666.666.1.500.9.1

This should be the other way around, IIUC your naming convention.

Comment 3

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

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

https://reviewboard.mozilla.org/r/80878/#review80224

Thanks!

> Consider using mozilla::PodEqual() instead (mainly because it seems a bit safer).

Good call.

> Nit: Indent this line two spaces further.

Fixed.

> This should be the other way around, IIUC your naming convention.

Good catch.

Comment 4

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

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

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

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

https://reviewboard.mozilla.org/r/80878/#review80978

Looks good; take a gander at my two nits. Thanks!

::: security/certverifier/ExtendedValidation.cpp:1306
(Diff revision 2)
>      return false;
>    }
>  
>    for (const nsMyTrustedEVInfo& entry : myTrustedEVInfos) {
>      if (entry.cert && CERT_CompareCerts(cert.get(), entry.cert.get())) {
> +      const SECOidData* cabforumOIDData = SECOID_FindOIDByTag(sCABForumEVOIDTag);

Nit: ISTM this lookup could go outside the for loop. Not commonly called, and I'm sure it's fast, so nbd.

::: security/certverifier/ExtendedValidation.cpp:1327
(Diff revision 2)
> +  if (SEC_StringToOID(nullptr, &cabforumOIDItem, "2.23.140.1.1", 0)
> +        != SECSuccess) {
> +    return PR_FAILURE;
> +  }
> +  sCABForumEVOIDTag = RegisterOID(cabforumOIDItem, "CA/Browser Forum EV OID");

Theses strings should be un-magicked, right? There's no other inline string usage outside of structs in this file that I see, other than the `PR_NOT_REACHED`.

Comment 6

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

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

https://reviewboard.mozilla.org/r/80878/#review80978

Thanks!

> Nit: ISTM this lookup could go outside the for loop. Not commonly called, and I'm sure it's fast, so nbd.

Right - it will only be called more than once if the root we have is in the EV info list more than once (which is only true for one root at the moment, according to a quick check I did). It does look algorithmically inefficient, though, so I went ahead and moved it.

> Theses strings should be un-magicked, right? There's no other inline string usage outside of structs in this file that I see, other than the `PR_NOT_REACHED`.

Un-magicked in what sense? Should I declare them as some const char*s somewhere?

Comment 7

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

ni so JC is aware of the question in comment 6.

Comment 8

[J.C. Jones [:jcj] (he/they)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> > Theses strings should be un-magicked, right? There's no other inline string usage outside of structs in this file that I see, other than the `PR_NOT_REACHED`.
> 
> Un-magicked in what sense? Should I declare them as some const char*s
> somewhere?

Yeah, that's what I meant. That seems to be a style in some sections of Gecko; if not here, nbd.

Comment 9

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

Cool - sounds good.

Comment 10

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

Comment on attachment 8794407 [details]
bug 1243923 - add support for the CA/Browser Forum EV OID

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

Comment 11

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

Here's try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18cccd09a6ec
eslint failed, but it looked like an infrastructure failure. I ran ./mach eslint to double-check, and everything passed on my machine, so I'm going to go ahead and push this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a94cba4c1a1
add support for the CA/Browser Forum EV OID r=Cykesiopka,jcj

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5a94cba4c1a1