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

RESOLVED FIXED in mozilla32

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Attachment #8418496 - Flags: review?(dkeeler)
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.
Attachment #8418496 - Flags: review?(dkeeler) → review+
(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 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?
Attachment #8423250 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/a4ae7060f43a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
No longer depends on: 1032947
You need to log in before you can comment on or make changes to this bug.