Closed Bug 1243923 Opened 6 years ago Closed 5 years ago

add support for CAB forum EV certificatePolicies OID (2.23.140.1.1)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox47 --- affected
firefox52 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

(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).
Whiteboard: [psm-backlog]
Priority: -- → P2
Assignee: nobody → dkeeler
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
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.
Attachment #8794407 - Flags: review?(cykesiopka.bmo) → review+
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 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`.
Attachment #8794407 - Flags: review?(jjones) → review+
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?
ni so JC is aware of the question in comment 6.
Flags: needinfo?(jjones)
(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.
Flags: needinfo?(jjones)
Cool - sounds good.
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a94cba4c1a1
add support for the CA/Browser Forum EV OID r=Cykesiopka,jcj
https://hg.mozilla.org/mozilla-central/rev/5a94cba4c1a1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.