1006958 - Use mozilla::pkix::der instead of NSS to decode certificate policies

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

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

Attachment: decode-Policy.patch

This patch:

* Removes the dependency on NSS's CERT_DecodeCertificatePoliciesExtension/CERT_DestroyCertificatePoliciesExtension/CERTCertificatePolicies functions for decoding certificate policies.

* Replaces the use of SECOidTag with a more-easily-extensible type-safe structure. This will enable us to more easily add support for testing not-yet-included EV policy OIDs via an environment variable. It also makes the API more type-safe.

* Makes processing certificate policies slightly more efficient by eliminating any memory allocation and avoiding the parsing/processing of the policy qualifier stuff.

Once we remove the old NSS-based certificate verification from PSM, we can simplify the EV policy stuff in ExtendedValidation.cpp by replacing the String -> SECOidData -> SECOidTag -> SECOidData -> CertPolicyId conversions with static/const CertPolicyId definitions, similar to how we simplified the processing of the certificate hashes in ExtendedValidation.cpp.

Comment 1

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

Comment on attachment 8418496 [details] [diff] [review]
decode-Policy.patch

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

This looks good. Again, a little bit more documentation would be appreciated.

::: security/certverifier/ExtendedValidation.cpp
@@ +1039,5 @@
>        bool found = false;
>        while (*policyInfos) {
>          const CERTPolicyInfo* policyInfo = *policyInfos++;
>  
>          SECOidTag oid_tag = policyInfo->oid;

Looks like this function has both oidTag and oid_tag. Can we rename oid_tag to something like candidatePolicyOidTag? Or rename oidTag to extensionOidTag and have oid_tag be oidTag? (This can be a follow-up bug.)

@@ +1057,1 @@
>            // in our list of OIDs accepted for EV

I think this comment got separated from where it should go.

::: security/pkix/include/pkix/pkixtypes.h
@@ +88,5 @@
>    // Determine the level of trust in the given certificate for the given role.
>    // This will be called for every certificate encountered during path
>    // building.
>    //
>    // When policy == SEC_OID_X509_ANY_POLICY, then no policy-related checking

"policy.IsAnyPolicy()"

::: security/pkix/lib/pkixcheck.cpp
@@ +118,5 @@
>  // user is not concerned about certificate policy."
> +//
> +
> +/*static*/ const CertPolicyId CertPolicyId::anyPolicy = {
> +  4, { (40*2)+5, 29, 32, 0 } };

nit: final }; on next line
Also, let's document where these numbers come from.

@@ +141,5 @@
> +                     requiredPolicy.bytes)) {
> +    found = true;
> +  } else if (endEntityOrCA == EndEntityOrCA::MustBeCA &&
> +             input.MatchTLV(der::OIDTag, CertPolicyId::anyPolicy.numBytes,
> +                              CertPolicyId::anyPolicy.bytes)) {

nit: indentation

@@ +171,5 @@
> +      requiredPolicy.numBytes > sizeof requiredPolicy.bytes) {
> +    return Fail(FatalError, SEC_ERROR_INVALID_ARGS);
> +  }
> +
> +  if (requiredPolicy.IsAnyPolicy()) {

There's an aspect of anyPolicy that I may be confused about. If requiredPolicy is anyPolicy, we're not checking for any particular policy in the current verification, right? So the inhibitAnyPolicy extension wouldn't have any effect, since we're not looking for a policy anyway. However, if requiredPolicy is something specific, a certificate with anyPolicy will match it. If we encounter an inhibitAnyPolicy extension, though, we want to fail verification until we fix bug 989051, right?
In any case, it would be good to document how this is supposed to work.
Ok - looks like TrustDomain::GetCertTrust documents the first part of this, but let's be clear that that reasoning applies here too.

Comment 2

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

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #1)
> There's an aspect of anyPolicy that I may be confused about. If
> requiredPolicy is anyPolicy, we're not checking for any particular policy in
> the current verification, right? So the inhibitAnyPolicy extension wouldn't
> have any effect, since we're not looking for a policy anyway. However, if
> requiredPolicy is something specific, a certificate with anyPolicy will
> match it. If we encounter an inhibitAnyPolicy extension, though, we want to
> fail verification until we fix bug 989051, right?
> In any case, it would be good to document how this is supposed to work.
> Ok - looks like TrustDomain::GetCertTrust documents the first part of this,
> but let's be clear that that reasoning applies here too.

Your understanding is correct.

Comment 3

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

Attachment: fix-warning-in-ExtendedValidation-cpp.patch

Comment 4

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

Comment on attachment 8423250 [details] [diff] [review]
fix-warning-in-ExtendedValidation-cpp.patch

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

LGTM.

::: security/certverifier/ExtendedValidation.cpp
@@ +1047,5 @@
>            PR_ASSERT(oidData->oid.data);
>            PR_ASSERT(oidData->oid.len > 0);
>            PR_ASSERT(oidData->oid.len <= mozilla::pkix::CertPolicyId::MAX_BYTES);
>            if (oidData && oidData->oid.data && oidData->oid.len > 0 &&
>                oidData->oid.len <= mozilla::pkix::CertPolicyId::MAX_BYTES) {

I'm assuming for this compare MAX_BYTES gets promoted to whatever type oid.len is?

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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