Closed
Bug 986156
Opened 10 years ago
Closed 10 years ago
mozilla::pkix does not accept anyPolicy in intermediate certificates when verifying certificate policy for EV
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: kwilson, Assigned: cviecco)
References
Details
Attachments
(2 files, 4 obsolete files)
54.49 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
6.04 KB,
patch
|
cviecco
:
review+
|
Details | Diff | Splinter Review |
I was getting EV treatment for the following sites this morning, then this afternoon my FF browser upgraded to FF 29, and now I'm not getting EV treatment for these sites. https://www.camerfirma.com https://addtrustexternalcaroot-ev.comodoca.com/ https://valid.evident.ca13.ssl.buypass.no/ https://valid.evident.ca23.ssl.buypass.no/ https://evdemo.cnnic.cn/ https://comodoecccertificationauthority-ev.comodoca.com/ https://comodocertificationauthority-ev.comodoca.com/ https://validev.entrust.net https://ssltest21.bbtest.net https://ssltest22.bbtest.net/ https://valid.gdig2.catest.godaddy.com/ https://evsslrca2-v.quovadisglobal.com https://repo2.secomtrust.net/ev.gif https://valid.sfi.catest.starfieldtech.com/ https://testevg2.swisssign.net https://root-class3.test.telesec.de https://ssltest8.bbtest.net https://valid.gdi.catest.godaddy.com/ https://evssl.turktrust.com.tr/ https://ssltest26.bbtest.net/ All of these test websites are from http://www.mozilla.org/about/governance/policies/security-group/certs/included/ There's a column indicating if EV is on for that root, and another column for the test website. I am still getting EV treatment for a few of the EV test websites, such as these two: https://secure.omniroot.com https://g2.startcom.org/
Reporter | ||
Comment 1•10 years ago
|
||
Just tried again, and now getting EV treatment for all of them. Something must have gotten cached, so I guess the problem is just when I hit these sites after freshly upgrading to FF 29.
Assignee | ||
Comment 2•10 years ago
|
||
My guess is that you were getting ocsp timeouts. Are you doing this from home? How are you testing? I tested this at the mv office and dont see any issues.
Reporter | ||
Comment 3•10 years ago
|
||
I just tried it again by using a different computer, upgrading to FF 29, and then hitting all of those sites. They all worked. So I guess I was just running into some sort of networking issue (from home) that caused the OCSP timeouts. Thanks.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 4•10 years ago
|
||
So, it has to do with mozpkix. Sorry for my confusion... I thought mozpkix was in FF 30, didn't realize that it's also in FF 29. I've just been browsing to the above websites using FirefoxAuroraDebug (FF 30.0a2) with both "security.use_insanity_verification" and "security.use_libpkix_verification" set to true. With the list above: - Only https://valid.evident.ca13.ssl.buypass.no/ gets EV treatment - https://evsslrca2-v.quovadisglobal.com/ gets the sec_error_bad_der error. - https://valid.evident.ca13.ssl.buypass.no/ gets the sec_error_ocsp_invalid_signing_cert error. - https://g2.startcom.org/ also does not get EV treatment
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Sites that were getting EV treatment no longer get EV treatment after upgrading to FF 29 → (mozilla::pkix) Sites not getting EV treatment
Version: 29 Branch → 30 Branch
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Kathleen Wilson from comment #4) > - https://valid.evident.ca13.ssl.buypass.no/ gets the > sec_error_ocsp_invalid_signing_cert error. Correction... - https://secure.omniroot.com/ gets the sec_error_ocsp_invalid_signing_cert error. Also note that https://evsslrca2-v.quovadisglobal.com/ only gets the sec_error_bad_der error sometimes. It doesn't get EV treatment, but sometimes it gets the padlock, then I do refresh and get the error.
(In reply to Kathleen Wilson from comment #5) > Also note that https://evsslrca2-v.quovadisglobal.com/ only gets the > sec_error_bad_der error sometimes. It doesn't get EV treatment, but > sometimes it gets the padlock, then I do refresh and get the error. With security.use_insanity_verification set to true and security.OCSP.require set to 1, this is reproducible for non-EV certs as well, try e.g. https://qvsslrca2-v.quovadisglobal.com/. Looks like mozilla::pkix is choking when it tries to parse the response extensions. The responses from ocsp.quovadisglobal.com include the id-pkix-ocsp-extended-revoke extension (RFC 6960 section 4.4.8), i.e.: [1] { SEQUENCE { SEQUENCE { OBJECT IDENTIFIER '1 3 6 1 5 5 7 48 1 9' OCTET STRING, encapsulates { NULL } } } } As far as I can tell, NestedOf/ExpectTagAndGetLength doesn't properly handle this case yet (bug 973751 is somewhat related).
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Kaspar Brand from comment #6) > security.OCSP.require set to 1 Should have read "set to true", sorry (in the UI, it's the "When an OCSP server connection fails, treat the certificate as invalid" thing).
(In reply to Kaspar Brand from comment #6) > (In reply to Kathleen Wilson from comment #5) > > Also note that https://evsslrca2-v.quovadisglobal.com/ only gets the > > sec_error_bad_der error sometimes. It doesn't get EV treatment, but > > sometimes it gets the padlock, then I do refresh and get the error. > > With security.use_insanity_verification set to true and > security.OCSP.require set to 1, this is reproducible for non-EV certs as > well, try e.g. https://qvsslrca2-v.quovadisglobal.com/. > > Looks like mozilla::pkix is choking when it tries to parse the response > extensions. The responses from ocsp.quovadisglobal.com include the > id-pkix-ocsp-extended-revoke extension (RFC 6960 section 4.4.8), i.e.: > > [1] { > SEQUENCE { > SEQUENCE { > OBJECT IDENTIFIER '1 3 6 1 5 5 7 48 1 9' > OCTET STRING, encapsulates { > NULL > } > } > } > } > > As far as I can tell, NestedOf/ExpectTagAndGetLength doesn't properly handle > this case yet (bug 973751 is somewhat related). I suspect there are multiple bugs here. I spun this particular issue off into bug 987295.
Assignee | ||
Updated•10 years ago
|
Summary: (mozilla::pkix) Sites not getting EV treatment → (mozilla::pkix) Does not accept the anyoid policy as a valid policy for intermediate certificates.
Assignee | ||
Updated•10 years ago
|
QA Contact: cviecco
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8395951 -
Flags: review?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cviecco
QA Contact: cviecco
Comment 10•10 years ago
|
||
Which CA/CAs is/are affected by this? Are you sure that we intend the allow anyPolicy for EV certificate verification? If we need to allow anyPolicy for the intermediates then we have to implement the inhibit-anyPolicy extension (bug 975877) too. I had thought that at one point in Gecko, we walked the cert chain and verified that every cert in the chain--except the root--explicitly included the given EV OID.
Status: REOPENED → ASSIGNED
Summary: (mozilla::pkix) Does not accept the anyoid policy as a valid policy for intermediate certificates. → mozilla::pkix does not accept anyPolicy in intermediate certificates when verifying certificate policy for EV
Comment 11•10 years ago
|
||
Comment on attachment 8395951 [details] [diff] [review] mozilla-pkix-allow-any-policy-oid-in-intermediates Review of attachment 8395951 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixcheck.cpp @@ +142,5 @@ > *policyInfos; ++policyInfos) { > if ((*policyInfos)->oid == requiredPolicy) { > return Success; > } > + // Intermediate certs are allowed to have the OID anypolicy oid s/the OID anypolicy oid/anyPolicy/ But, that is only true if none of the issuers in the chain have not asserted the inhibit-anyPolicy extension. I have a patch that implements checking the inhibit-anyPolicy extension, but I was expecting that we would not need it because I thought our intended EV verification rules were to require all intermediates to explicitly list the EV OID. Requiring that all EV policies be explicit throughout the chain makes EV a more useful security mechanism (to the extent it is useful at all).
Attachment #8395951 -
Flags: review-
Comment 12•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #10) > I had thought that at one point in Gecko, we walked the cert chain and > verified that every cert in the chain--except the root--explicitly included > the given EV OID. Nope. It seems like we had a redundant (AFAICT) check that the root was approved for EV, but we weren't checking every cert in the chain. However, I still think it is worth considering requiring EV policy to be explicit--i.e. resolving this bug RESOLVED INVALID and creating a Tech Evangelism bug to work with the affected CAs to address this.
Assignee | ||
Comment 13•10 years ago
|
||
Since inhibit-anyPolicy is supposed to be a critical extension( http://tools.ietf.org/html/rfc5280#section-4.2.1.14) and we abort in unknown critical extensions do you think handling inhibit any-policy is a blocker for this?
Updated•10 years ago
|
Blocks: mozilla::pkix-beta
No longer depends on: 987295
Reporter | ||
Comment 14•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #12) > However, I still think it is worth considering requiring EV policy to be > explicit We've always allowed the anyPolicy OID in the intermediate certs. EV Guidelines section 9.3.4 (section 8.2.2 in older versions): "Certificates issued to Subordinate CAs that are controlled by the Root CA MAY contain the special anyPolicy identifier (OID: 2.5.29.32.0)." > --i.e. resolving this bug RESOLVED INVALID Please don't do that. This bug impacts just about every CA with EV enabled in our program. In my opinion, this bug is a showstopper for mozilla::pkix. > and creating a Tech > Evangelism bug to work with the affected CAs to address this. That's fine to do as a side project, but to really do this will involve changing the CAB Forum's EV Guidelines and giving CAs time to transition to new intermediate certs.
Comment 15•10 years ago
|
||
(In reply to Kathleen Wilson from comment #14) > > However, I still think it is worth considering requiring EV policy to be > > explicit > > We've always allowed the anyPolicy OID in the intermediate certs. Understood. BTW, I mis-wrote when I said "explicit." Processing anyPolicy in the way that Camilo proposed is considered "explicit" matching according to RFC 5280. > EV Guidelines section 9.3.4 (section 8.2.2 in older versions): > "Certificates issued to Subordinate CAs that are controlled by the Root CA > MAY contain the special anyPolicy identifier (OID: 2.5.29.32.0)." Right. But, it doesn't say that a browser has to give EV treatment to such certificates. Remember that the EV guidelines are constraints on CAs, not constraints on browsers. However... > This bug impacts just about every CA with EV enabled > in our program. In my opinion, this bug is a showstopper for mozilla::pkix. I agree. It seems to prevent https://paypal.com and https://twitter.com from getting EV treatment. This is the only policy OID in Verisign's certificates, for example. Note that it isn't possible to implement anyPolicy support without inhibit-anyPolicy support and vice versa, so this bug will be about implementing both.
Updated•10 years ago
|
Comment 16•10 years ago
|
||
IMO we should not support anyPolicy on the leaf cert; this was discussed in bug 968817. Gerv
Assignee | ||
Comment 17•10 years ago
|
||
>
> Note that it isn't possible to implement anyPolicy support without
> inhibit-anyPolicy support and vice versa, so this bug will be about
> implementing both.
I dont see why is this. If we only process non-critical policy extensions (I realized that we do not process critical policy extensions correctly) and the library aborts critical inhibitanypolicy I dont see what could be the issue.
Reporter | ||
Comment 18•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #16) > IMO we should not support anyPolicy on the leaf cert; this was discussed in > bug 968817. Correct. EV treatment should not be given if the end-entity cert does not have the explicit EV Policy OID that we expect. We have always given (and should continue to give) EV treatment when the end-entity cert has the correct EV Policy OID and the intermediate cert has the anyPolicy OID, as per the EV Guidelines.
Assignee | ||
Comment 19•10 years ago
|
||
Comment 20•10 years ago
|
||
(In reply to Camilo Viecco (:cviecco) from comment #13) > Since inhibit-anyPolicy is supposed to be a critical extension( > http://tools.ietf.org/html/rfc5280#section-4.2.1.14) and we abort in unknown > critical extensions do you think handling inhibit any-policy is a blocker > for this? Sorry, I missed this comment yesterday. Yes, I think handling the "inhibit anyPolicy" extension is important for doing this. You are right that the spec says that the extension must be marked critical. However, it is almost certainly the case that in practice nobody can mark it critical because many applications don't deal with certificate policy stuff at all. Anyway, support for "inhibit anyPolicy" is not hard to implement. I will look for my patch and attach it here; it will need to be rebased to the current state of things.
Comment 21•10 years ago
|
||
In addition to my previous review comments, the documentation in pkix.h under "LIMITED SUPPORT FOR CERTIFICATE POLICIES" needs to be changed.
Comment on attachment 8395951 [details] [diff] [review] mozilla-pkix-allow-any-policy-oid-in-intermediates Review of attachment 8395951 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review due to the issue Brian raised.
Attachment #8395951 -
Flags: review?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Attachment #8396445 -
Flags: review?(dkeeler)
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #20) > (In reply to Camilo Viecco (:cviecco) from comment #13) > > Since inhibit-anyPolicy is supposed to be a critical extension( > > http://tools.ietf.org/html/rfc5280#section-4.2.1.14) and we abort in unknown > > critical extensions do you think handling inhibit any-policy is a blocker > > for this? > > Sorry, I missed this comment yesterday. Yes, I think handling the "inhibit > anyPolicy" extension is important for doing this. You are right that the > spec says that the extension must be marked critical. However, it is almost > certainly the case that in practice nobody can mark it critical because many > applications don't deal with certificate policy stuff at all. In this case why not do a follow up bug regarding imhibitAnyPolicy that does not block enabling mozilla::pkix? > > Anyway, support for "inhibit anyPolicy" is not hard to implement. I will > look for my patch and attach it here; it will need to be rebased to the > current state of things. I am more worried that currently a critical ceritificate policy with any arbirary non-matching policy would be accepted in mozilla:pkix and rejected by 'classic'. I would argue that to finish this bug (besides my patch) we should also reject certificates that have a critical certificate policy.
Comment on attachment 8396445 [details] [diff] [review] test-any-oid-plolicy-in-ev-intermediates Review of attachment 8396445 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: security/manager/ssl/tests/unit/test_ev_certs.js @@ +21,5 @@ > // Test for successful EV validation > 'int-ev-valid', > 'ev-valid', > + 'ev-valid-ap-int', > + 'int-ev-valid-ap-int', nit: maybe spell out "anypolicy" - we're not going to remember what "ap" means in 6 months. @@ +137,5 @@ > check_ee_for_ev("ev-valid", isDebugBuild); > ocspResponder.stop(run_next_test); > }); > > + nit: unnecessary vertical space @@ +217,5 @@ > : (isDebugBuild ? SEC_ERROR_REVOKED_CERTIFICATE > : SEC_ERROR_EXTENSION_NOT_FOUND)); > }); > > + nit: unnecessary vertical space @@ +244,5 @@ > : SEC_ERROR_EXTENSION_NOT_FOUND)); > failingOcspResponder.stop(run_next_test); > }); > }); > + nit: unnecessary vertical space @@ +272,5 @@ > let identityInfo = cert.QueryInterface(Ci.nsIIdentityInfo); > do_check_eq(identityInfo.isExtendedValidation, false); > ocspResponder.stop(run_next_test); > } > + nit: unnecessary vertical space ::: security/manager/ssl/tests/unit/test_ev_certs/generate.py @@ +45,5 @@ > + "policyIdentifier = " + > + "2.5.29.32.0\n\n" + > + "CPS.1 = \"http://mytestdomain.local/cps\"") > + > + nit: unnecessary vertical space @@ +137,5 @@ > + "int-" + prefix) > + import_cert_and_pkcs12(int_cert, pk12file, "int-" + prefix, ",,") > + import_untrusted_cert(ee_cert, prefix) > + > + nit: unnecessary vertical space
Attachment #8396445 -
Flags: review?(dkeeler) → review+
Comment 25•10 years ago
|
||
(In reply to Camilo Viecco (:cviecco) from comment #23) > (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) > from comment #20) > > (In reply to Camilo Viecco (:cviecco) from comment #13) > > > Since inhibit-anyPolicy is supposed to be a critical extension( > > > http://tools.ietf.org/html/rfc5280#section-4.2.1.14) and we abort in unknown > > > critical extensions do you think handling inhibit any-policy is a blocker > > > for this? > > > > Sorry, I missed this comment yesterday. Yes, I think handling the "inhibit > > anyPolicy" extension is important for doing this. You are right that the > > spec says that the extension must be marked critical. However, it is almost > > certainly the case that in practice nobody can mark it critical because many > > applications don't deal with certificate policy stuff at all. > > In this case why not do a follow up bug regarding imhibitAnyPolicy that does > not block enabling mozilla::pkix? If you want, we can split out the implementation of inhibit-anyPolicy into its own bug, but that bug should block mozilla::pkix because it is incorrect to implement anyPolicy support without implementing inhibit-anyPolicy support. Also, it will not be hard to add inhibit-anyPolicy support; I've written a patch for it already. Please file the new bug if you agree. > I am more worried that currently a critical ceritificate policy with any > arbirary non-matching policy would be accepted in mozilla:pkix and rejected > by 'classic'. I would argue that to finish this bug (besides my patch) we > should also reject certificates that have a critical certificate policy. This was already RESOLVED INVALID in another bug.
Assignee | ||
Comment 26•10 years ago
|
||
> If you want, we can split out the implementation of inhibit-anyPolicy into > its own bug, but that bug should block mozilla::pkix because it is incorrect > to implement anyPolicy support without implementing inhibit-anyPolicy > support. Also, it will not be hard to add inhibit-anyPolicy support; I've > written a patch for it already. Please file the new bug if you agree. New bug: https://bugzilla.mozilla.org/show_bug.cgi?id=989051
Assignee | ||
Comment 27•10 years ago
|
||
Secure workaround until Bug 989051 lands
Assignee | ||
Updated•10 years ago
|
Attachment #8398106 -
Flags: review?(dkeeler)
Attachment #8398106 -
Flags: feedback?(brian)
Comment on attachment 8398106 [details] [diff] [review] workaround-inhibit-anypolicy Review of attachment 8398106 [details] [diff] [review]: ----------------------------------------------------------------- Great. ::: security/pkix/lib/pkixbuild.cpp @@ +67,5 @@ > case 30: out = &encodedNameConstraints; break; > case 32: out = &encodedCertificatePolicies; break; > case 35: out = &dummyEncodedAuthorityKeyIdentifier; break; // bug 965136 > case 37: out = &encodedExtendedKeyUsage; break; > + case 54: out = &encodedInhibitAnyPolicy; break; // Bug 989051 tiny nit: lowercase "bug" to be consistent ::: security/pkix/lib/pkixcheck.cpp @@ +117,5 @@ > PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > return FatalError; > } > > + // Bug 989051. Until we handle inhibitAnyPolicy we will fail close when nit: I think the expression is more commonly "fail closed"
Attachment #8398106 -
Flags: review?(dkeeler) → review+
Comment 29•10 years ago
|
||
Comment on attachment 8398106 [details] [diff] [review] workaround-inhibit-anypolicy Review of attachment 8398106 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixcheck.cpp @@ +117,5 @@ > PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > return FatalError; > } > > + // Bug 989051. Until we handle inhibitAnyPolicy we will fail close when Please s/Bug 989051./TODO(bug 989051):/
Attachment #8398106 -
Flags: feedback?(brian) → feedback+
Updated•10 years ago
|
Attachment #8398106 -
Flags: review+
Assignee | ||
Comment 30•10 years ago
|
||
Same logic, but now with inhibitAnyPilicy workaround this should be OK.
Attachment #8395951 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8398145 -
Flags: review?(dkeeler)
Attachment #8398145 -
Flags: feedback?(brian)
Comment 31•10 years ago
|
||
Comment on attachment 8398145 [details] [diff] [review] mozilla-pkix-allow-any-policy-oid-in-intermediates (v1.1) Review of attachment 8398145 [details] [diff] [review]: ----------------------------------------------------------------- You need to update the comments in pkix.h as I mentioned before.
Attachment #8398145 -
Flags: feedback?(brian) → feedback-
Updated•10 years ago
|
Attachment #8398145 -
Flags: feedback- → review-
Assignee | ||
Comment 32•10 years ago
|
||
what about this comment?
Attachment #8398145 -
Attachment is obsolete: true
Attachment #8398145 -
Flags: review?(dkeeler)
Attachment #8398168 -
Flags: review?(brian)
Assignee | ||
Comment 33•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6f3d8f8b3b4b
Comment 34•10 years ago
|
||
Comment on attachment 8398168 [details] [diff] [review] mozilla-pkix-allow-any-policy-oid-in-intermediates (v 1.2) Review of attachment 8398168 [details] [diff] [review]: ----------------------------------------------------------------- Please fold the workaround-inhibit-anypolicy patch and this patch together before checking them in. Please address the comments below before checking in the patches. ::: security/pkix/include/pkix/pkix.h @@ +33,5 @@ > // In RFC 5280 terms: > // > // * user-initial-policy-set = { requiredPolicy }. > // * initial-explicit-policy = true > // * initial-any-policy-inhibit = true * initial-any-policy-inhibit = false now, right? @@ +37,5 @@ > // * initial-any-policy-inhibit = true > // > +// We allow intermediate cerificates to use the anyPolicy policy but since > +// we do not process the inhibit anyPolicy extesion we will fail if this > +// extension is present. Please add a TODO(bug 989051) and s/this extension/the inhibit anyPolicy extension/
Attachment #8398168 -
Flags: review?(brian) → review+
Assignee | ||
Comment 35•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6f3d8f8b3b4b
Assignee | ||
Comment 36•10 years ago
|
||
Folded in workaround for inhibit anypolicy. Keeping r+ from bsmith, and dkeeler
Attachment #8398106 -
Attachment is obsolete: true
Attachment #8398168 -
Attachment is obsolete: true
Attachment #8399450 -
Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f18b20580a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb2a113f16b
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7bb2a113f16b https://hg.mozilla.org/mozilla-central/rev/5f18b20580a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•