552775 - CERT_PKIXVerifyCert fails with SEC_ERROR_POLICY_VALIDATION_FAILED if an intermediate CA cert has requireExplicitPolicy

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: DER certificate of https://www.us.army.mil/

To reproduce this bug, call CERT_PKIXVerifyCert as follows:
1. Do NOT specify cert_pi_policyOID in the paramsIn input argument.
2. Have it verify a certificate whose intermediate CA certificate
has requireExplicitPolicy in its policyConstraints extension.
The certificate chain of https://www.us.army.mil/ has this property.
I attached its certificate chain to this bug report.

CERT_PKIXVerifyCert fails with SEC_ERROR_POLICY_VALIDATION_FAILED.
Using LXR, I found three possible places where we set this error:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c&rev=1.1&mark=2501#2499

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c&rev=1.1&mark=2622#2612

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c&rev=1.1&mark=2649#2640

I can work around this by passing a cert_pi_policyOID array of
just the certificate policy of the end entity cert to
CERT_PKIXVerifyCert.

It seems that if cert_pi_policyOID is not specified, CERT_PKIXVerifyCert
should not perform policy validation (pkix_PolicyChecker_Check).

Comment 1

[Wan-Teh Chang]

Attachment: DER certificate of DOD CA 17 (the intermediate CA)

Comment 2

[Wan-Teh Chang]

Attachment: DER certificate of DoD Root CA 2

To reproduce this bug, this root CA needs to be trusted.

Comment 3

[Wan-Teh Chang]

This bug was originally reported in Chromium issue 31497:
http://code.google.com/p/chromium/issues/detail?id=31497

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I believe it is possible to check that a cert chain has at least
one valid explicit policy, when required, even when the caller does not 
require any specific policy.  I haven't looked at this chain yet. 
Is there some policy OID for which it has an explicit policy chain?

Comment 5

[Wan-Teh Chang]

Nelson, thank you for your comment.  Your suggestion is exactly
the workaround I'm planning to use in Chromium:
http://codereview.chromium.org/545103/diff/15001/16001#newcode447

The first workaround I tried was to pass a cert_pi_policyOID array
of just SEC_OID_X509_ANY_POLICY.  That didn't work.

A problem with my workaround is that cert_pi_policyOID is an
array of SECOidTag's, so I have to import the certificate's
policy OID into NSS as a dynamic OID (see the SECOID_AddEntry
call in my Chromium patch).  So the NSS dynamic OID table
could grow if Chromium keeps encountering such cert chains
in a browsing session.

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch: change the default of cert_pi_policyOID

Good news!  While debugging this bug today, I discovered the
initialIsAnyPolicy variable in pkix_policychecker.c, so I redid
my test.  Contrary to what I reported in the previous comment,
passing a cert_pi_policyOID array of just SEC_OID_X509_ANY_POLICY
works!

RFC 5280 says:

      (c)  user-initial-policy-set:  A set of certificate policy
           identifiers naming the policies that are acceptable to the
           certificate user.  The user-initial-policy-set contains the
           special value any-policy if the user is not concerned about
           certificate policy.

So I think our default value for cert_pi_policyOID is wrong:

   cert_pi_policyOID       = 4, /* validate certificate for policy OID.
				 * Specified in value.array.oids. Cert must
				 * be good for at least one OID in order
				 * to validate. Default is no policyOID */
                                                ^^^^^^^^^^^^^^^^^^^^^^^

I think the default should be anyPolicy.  This comment in libpkix
supports that:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/include/pkix_params.h&rev=1.8&mark=488-492,512#479

I don't think any NSS-based apps depend on the current default, so
we should be able to change the default.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, The solution should not depend on the use of "anyPolicy".
The reason is this: just as any CA cert can requireExplicitPolicy,
so also any CA cert can inhibitAnyPolicy.  The cert chain should
still validate with both of those specified, even in the absence of 
an application-supplied policy OID.

Comment 8

[Wan-Teh Chang]

Nelson, thanks again for your comment.  I spent several more hours
last night to study the relevant libpkix code and RFC 3280/5280.

The first thing I learned is that RFC 3280/5280 distinguishes
between
    anyPolicy: a special certificate policy, with a value of
    { 2 5 29 32 0 }, used in CA certificates
and
    any-policy: a special value of the user-initial-policy-set
    if the user is not concerned about certificate policy

This distinction isn't obvious in RFC 3280, which sometimes uses
"any-policy" to mean "anyPolicy", but is made strict in RFC 5280.
You can see this change by searching for "any-policy" in RFC 3280
and RFC 5280.

So, libpkix's use of the anyPolicy OID to mean the any-policy
special value of user-initial-policy-set is strictly speaking
incorrect:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c&rev=1.1&mark=477-482#477

This incorrect use of the anyPolicy OID in user-initial-policy-set
won't lead to bugs if we're careful, but it would be a good idea
to avoid this confusion.

So, inhibitAnyPolicy applies to anyPolicy (in CA certificates),
rather than any-policy (in user-initial-policy-set).

Second, I found that my workaround of passing a cert_pi_policyOID
array of just SEC_OID_X509_ANY_POLICY doesn't work in some
important cases, because we also set initial-explicit-policy if
any cert_pi_policyOID is specified:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfypkix.c&rev=1.43&mark=1567,1569-1571#1567

Because initial-explicit-policy is set, a certificate that doesn't
have a certificatePolicies extension fails validation.

I can fix these problems, but I'd like to know: do we still want to
allow the use of SEC_OID_X509_ANY_POLICY to mean the special value
any-policy for the user-initial-policy-set?  I can make it work
either way.  I am inclined towards disallowing that to avoid confusion.

Comment 9

[Wan-Teh Chang]

Note for myself: if initial-explicit-policy is set and a certificate
doesn't have a certificatePolicies extension, libpkix returns an error
here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c&rev=1.1&mark=2499-2502#2499

The value of the variable 'state' is (for the cert of mail.google.com):
(gdb) print *state
$1 = {certPoliciesExtension = 0x7fffecfbfb28, 
  policyMappingsExtension = 0x7fffecfbfae8, 
  policyConstraintsExtension = 0x7fffecfbfaa8, 
  inhibitAnyPolicyExtension = 0x7fffecfbfd28, anyPolicyOID = 0x7fffecfbfce8, 
  initialIsAnyPolicy = 1, validPolicyTree = 0x0, 
  userInitialPolicySet = 0x7fffed0e7a28, 
  mappedUserInitialPolicySet = 0x7fffed0e7a28, policyQualifiersRejected = 0, 
  initialPolicyMappingInhibit = 0, initialExplicitPolicy = 1, 
  initialAnyPolicyInhibit = 0, explicitPolicy = 0, inhibitAnyPolicy = 3, 
  policyMapping = 3, numCerts = 2, certsProcessed = 0, 
  anyPolicyNodeAtBottom = 0x0, newAnyPolicyNode = 0x0, 
  certPoliciesCritical = 0, mappedPolicyOIDs = 0x0}

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v2

This patch continues to allow the use of the anyPolicy OID to
mean the special value any-policy for the user-initial-policy-set.
I recommend that we disallow it.  It won't break backward
compatibility because having the anyPolicy OID in the
cert_pi_policyOID array breaks certificates that have no
certificatePolicies extensions such as https://mail.google.com
and https://www.apache.org.

Other changes:

1. Map PKIX_FUNCTIONMUSTNOTBEUSED to SEC_ERROR_LIBPKIX_INTERNAL
instead of SEC_ERROR_INVALID_ARGS.

2. Add PKIX_PRECONDITIONFAILED, because I didn't find any
PKIX assertion macro.

3. Add a precondition check to pkix_PolicyChecker_CalculateIntersection.

4. Fix an input argument check in pkix_PolicyChecker_PolicyMapProcessing.
It should check the argument that the function will be using.

Comment 11

[Wan-Teh Chang]

Attachment: Prevent anyPolicy from setting initial-explicit-policy

A change in my proposed patch v2 breaks three tests in chains.sh.
The expected results of those three tests need to be updated.
Because this may be controversial, I decided to split that change
from the main patch.

Comment 12

[Wan-Teh Chang]

Attachment: Proposed patch v3 (checked in)

I removed the change that requires changing the expected results of
three chains.sh tests (now in attachment 433658 [details] [diff] [review]), and added three new
test cases.

Comment 13

[Alexei Volkov]

Comment on attachment 433664 [details] [diff] [review]
Proposed patch v3 (checked in)

r=alexei

Comment 14

[Wan-Teh Chang]

Comment on attachment 433664 [details] [diff] [review]
Proposed patch v3 (checked in)

I checked in the patch on the NSS trunk (NSS 3.12.7).

Checking in lib/certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.54; previous revision: 1.53
done
Checking in lib/certhigh/certvfypkix.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfypkix.c,v  <--  certvfypkix.c
new revision: 1.46; previous revision: 1.45
done
Checking in lib/libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.35; previous revision: 1.34
done
Checking in lib/libpkix/pkix/checker/pkix_policychecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_policychecker.c,v  <--  pkix_policychecker.c
new revision: 1.3; previous revision: 1.2
done
Checking in tests/chains/scenarios/anypolicywithlevel.cfg;
/cvsroot/mozilla/security/nss/tests/chains/scenarios/anypolicywithlevel.cfg,v  <--  anypolicywithlevel.cfg
new revision: 1.3; previous revision: 1.2
done