921886 - Add certificate policy support to insanity::pkix

Status: RESOLVED FIXED

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

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The certificate policy support only needs to be good enough to verify EV certificates. That means, AFAICT, that we can always require explicit policy, always prohibit policy mapping, and always ignore anyPolicy. This is great because then we won't have to explicitly implement the policy mapping extension, policy constraints extension, or inhibit anyPolicy extension. It also means we can greatly simplify testing.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: certificate-policies.patch

This will be tested (only, for now) via the test_ev_certs.js xpcshell tests, which will be extended in bug 921885 to cover more of this.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8380879 [details] [diff] [review]
certificate-policies.patch

Review of attachment 8380879 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/insanity/lib/pkixcheck.cpp
@@ +420,5 @@
>    }
>  
> +  // 4.2.1.1. Authority Key Identifier is ignored (see bug 965136).
> +
> +  // 4.2.1.2. Subject Key Identifier is ignored (see bug 965136).

Note that some of these comment changes were unrelated, but I don't think it is worth splitting them into another patch, considering that ~50% of them are relevant to this patch anyway.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8380879 [details] [diff] [review]
certificate-policies.patch

Review of attachment 8380879 [details] [diff] [review]:
-----------------------------------------------------------------

r- just for clarifications.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +76,4 @@
>                                     const CERTCertificate* candidateCert,
>                                     /*out*/ TrustLevel* trustLevel)
>  {
> +  PR_ASSERT(candidateCert);

Just to be consistent: PR_XX instead of PORT_XX from now on?

::: security/insanity/lib/pkixcheck.cpp
@@ +122,5 @@
> +  // which policies is made by the TrustDomain's GetCertTrust method. Note
> +  // that this is needed for many EV root certificates.
> +  //
> +  // XXX: Is this correct? I don't think cert.GetNSSCert()->isRoot is
> +  // calculated correctly. Also, not sure this is 100% correct logic.

Why not stop with isTrustAnchor && endEntityOrCA == MustBeCA?

@@ +127,5 @@
> +  if (isTrustAnchor && endEntityOrCA == MustBeCA && cert.GetNSSCert()->isRoot) {
> +    return Success;
> +  }
> +
> +  if (!cert.encodedCertificatePolicies) {

if (endEntityOrCA == MustBeEndEntity) { return FatalError;} ?

CA is ok to return recoverable.

@@ +135,5 @@
> +
> +  ScopedPtr<CERTCertificatePolicies, CERT_DestroyCertificatePoliciesExtension>
> +    policies(CERT_DecodeCertificatePoliciesExtension(
> +                cert.encodedCertificatePolicies));
> +  if (!policies) {

again, on EE I think this should be fatal.

@@ +145,5 @@
> +    if ((*policyInfos)->oid == requiredPolicy) {
> +      return Success;
> +    }
> +  }
> +

fatal on EE?

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 8380879 [details] [diff] [review]
certificate-policies.patch

Review of attachment 8380879 [details] [diff] [review]:
-----------------------------------------------------------------

r- just for clarifications.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +76,4 @@
>                                     const CERTCertificate* candidateCert,
>                                     /*out*/ TrustLevel* trustLevel)
>  {
> +  PR_ASSERT(candidateCert);

Just to be consistent: PR_XX instead of PORT_XX from now on?

::: security/insanity/lib/pkixcheck.cpp
@@ +122,5 @@
> +  // which policies is made by the TrustDomain's GetCertTrust method. Note
> +  // that this is needed for many EV root certificates.
> +  //
> +  // XXX: Is this correct? I don't think cert.GetNSSCert()->isRoot is
> +  // calculated correctly. Also, not sure this is 100% correct logic.

Why not stop with isTrustAnchor && endEntityOrCA == MustBeCA?

@@ +127,5 @@
> +  if (isTrustAnchor && endEntityOrCA == MustBeCA && cert.GetNSSCert()->isRoot) {
> +    return Success;
> +  }
> +
> +  if (!cert.encodedCertificatePolicies) {

if (endEntityOrCA == MustBeEndEntity) { return FatalError;} ?

CA is ok to return recoverable.

@@ +135,5 @@
> +
> +  ScopedPtr<CERTCertificatePolicies, CERT_DestroyCertificatePoliciesExtension>
> +    policies(CERT_DecodeCertificatePoliciesExtension(
> +                cert.encodedCertificatePolicies));
> +  if (!policies) {

again, on EE I think this should be fatal.

@@ +145,5 @@
> +    if ((*policyInfos)->oid == requiredPolicy) {
> +      return Success;
> +    }
> +  }
> +

fatal on EE?

Comment 5

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

Comment on attachment 8380879 [details] [diff] [review]
certificate-policies.patch

Review of attachment 8380879 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: security/insanity/lib/pkixcheck.cpp
@@ +122,5 @@
> +  // which policies is made by the TrustDomain's GetCertTrust method. Note
> +  // that this is needed for many EV root certificates.
> +  //
> +  // XXX: Is this correct? I don't think cert.GetNSSCert()->isRoot is
> +  // calculated correctly. Also, not sure this is 100% correct logic.

Agreed - why not trust what the TrustDomain says? (i.e. if it says it's a trust anchor and we're looking for a CA, then it's a root, right?)

@@ +127,5 @@
> +  if (isTrustAnchor && endEntityOrCA == MustBeCA && cert.GetNSSCert()->isRoot) {
> +    return Success;
> +  }
> +
> +  if (!cert.encodedCertificatePolicies) {

The recoverable/fatal error distinction only makes sense in terms of trying other possibilities, right? And in the EE case, there are no other possibilities, so processing won't continue if either a recoverable or fatal error is encountered here. So, I think it's fine that RecoverableError is returned here and elsewhere in this function for both EEs and CAs.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8380879 [details] [diff] [review]
certificate-policies.patch

Review of attachment 8380879 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +76,4 @@
>                                     const CERTCertificate* candidateCert,
>                                     /*out*/ TrustLevel* trustLevel)
>  {
> +  PR_ASSERT(candidateCert);

Yes, please use PR_*, except in NSS. In NSS use PORT_*.

::: security/insanity/lib/pkixcheck.cpp
@@ +122,5 @@
> +  // which policies is made by the TrustDomain's GetCertTrust method. Note
> +  // that this is needed for many EV root certificates.
> +  //
> +  // XXX: Is this correct? I don't think cert.GetNSSCert()->isRoot is
> +  // calculated correctly. Also, not sure this is 100% correct logic.

Unless I hear otherwise from y'all, I will remove the IsRoot check.

An intermediate could be explicitly trusted by the user in the certificate manager and still not be a root. In that case, do we want to treat the end-entity certificate as EV?

@@ +127,5 @@
> +  if (isTrustAnchor && endEntityOrCA == MustBeCA && cert.GetNSSCert()->isRoot) {
> +    return Success;
> +  }
> +
> +  if (!cert.encodedCertificatePolicies) {

David is correct here. In the EE case it is kind of a distinction without a difference anyway. However, FatalError is really for errors where we must stop path building dead. In fact, now that all this code is written, I'm not sure it was even a good idea to have the Fatal/Recoverable distinction; we could probably have just written an IsFatalError(PRErrorCode) function that calculates the recoverability (fatality?) from the error code. Maybe we can change to that in the future.

Comment 7

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> > +  // XXX: Is this correct? I don't think cert.GetNSSCert()->isRoot is
> > +  // calculated correctly. Also, not sure this is 100% correct logic.
> 
> Unless I hear otherwise from y'all, I will remove the IsRoot check.
> 
> An intermediate could be explicitly trusted by the user in the certificate
> manager and still not be a root. In that case, do we want to treat the
> end-entity certificate as EV?

Actually, maybe we should also see if cert is self-signed?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #7)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> > An intermediate could be explicitly trusted by the user in the certificate
> > manager and still not be a root. In that case, do we want to treat the
> > end-entity certificate as EV?
> 
> Actually, maybe we should also see if cert is self-signed?

David and Camilo, please see if the comments in pkixtypes.h in my updated patch address this issue to your satisfaction.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: certificate-policies.patch [v2]

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8381959 [details] [diff] [review]
certificate-policies.patch [v2]

Review of attachment 8381959 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM now (the comment makes a lot of difference)

Comment 11

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

Comment on attachment 8381959 [details] [diff] [review]
certificate-policies.patch [v2]

Review of attachment 8381959 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds like a plan.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e50c326ad721

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8381959 [details] [diff] [review]
certificate-policies.patch [v2]

[Approval Request Comment]
See bug 915931 comment 43.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e50c326ad721

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d447f155ab7