647364 - CERT_PKIXVerifyCert doesn't check cert->trust of the leaf certificate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Incomplete patch to illustrate a possible fix

In bug 642815 we marked the fraudulent certificates issued
by Comodo CA as untrusted in libnssckbi.so.  I found that
they have no effect on CERT_PKIXVerifyCert, and it is
because CERT_PKIXVerifyCert doesn't have any code equivalent
to the following code in CERT_VerifyCert:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfy.c&rev=1.71&mark=1414-1488#1414

The CERT_VerifyCert code does two things:
1. If the leaf cert is explicitly trusted, it skips
chain building and returns successfully.
2. If the leaf cert is explicitly untrusted, it sets
SEC_ERROR_UNTRUSTED_CERT.

#2 is easy to simulate in CERT_PKIXVerifyCert.  There
are two or three places where I can check the trust
flags of the leaf cert.  The attached patch shows one
possible solution.  Note that pkix_CertSelector_DefaultMatch
is invoked here for the leaf (end entity) cert:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c&rev=1.59&mark=3351,3363-3365#3350

#1 is hard to simulate because it's not easy to skip
chain building in CERT_PKIXVerifyCert while still
returning the output arguments correctly.

Comment 1

[Robert Relyea]

We should make this related to bug 642503.

I had noticed this as part of looking at that bug, but planned on fixing is in that bug as well.

Currently the following cert types cannot be revoked using the trust store:

1) Intermediate certs.
2) Email certs.
3) OCSP responder certs
4) Any cert processed in libpkix.
Also
5) Revoked root certs currently look the same as a missing root cert.

I'm working on a two phase patch. The larger patch is a rename patch with little or not functionality change. I think it's important to do this first so that our names match their actual usage.

the second patch will fix the 4 issues above

Comment 2

[Kai Engert (:KaiE:)]

We are waiting for clarification (in bug 642503) about the status of this bug, and whether or not it still blocks bug 651246.

Comment 3

[Robert Relyea]

The patch in 642503 only partially solves this bug. There are two aspects of checking the trust value. One is to explicitly distrust a leaf cert. The second it to explicitly trust a leaf cert. The patch in 642503 implements explicitly distrusting the leaf. Unfortunately in libpkix, it's easy to blow out when we know the cert is bad. It's not so easy to set up the appropriate tests when the cert is good.... so this bug still needs to stay open.

Comment 4

[Robert Relyea]

If leaf trust is an issue for 651246, it's most likely to be an issue with the yet to be implemented functionality.

Comment 5

[Wan-Teh Chang]

Problem 2 (explicit distrust) described in comment 0 has been fixed
by the patch in bug 642503.

The remaining work is to fix problem 1 (explicit trust).

Comment 7

[Wan-Teh Chang]

Attachment: Proposed patch

I have written a fix for supporting explicitly trusted leaf certificate.
We need to decide what to report as the trust anchor in this case.
This patch reports a null trust anchor and depends on the patch in
bug 726134.

This patch also does some cleanup.
1. Fix comment typos.
2. Remove the unused matchingAnchor variable.  It is initialized to
   NULL and never changed.
3. Fix a reference count bug.  After releasing the reference held
   in firstHintCert, we must set firstHintCert to NULL.  Otherwise
   we will release the reference in firstHintCert again at the end
   of the function.

Comment 8

[Wan-Teh Chang]

Attachment: Proposed patch v2

Fixed a similar reference count bug with 'certStore'.

Added a comment to explain the consequence of not returning a
PKIX_ForwardBuilderState in *pState.

Comment 9

[Robert Relyea]

Comment on attachment 596178 [details] [diff] [review]
Proposed patch v2

r+ rrelyea

Good catch on matchingAnchor being basically dead code as well.

RE XXX do we need to check revocation?

I don't think so. Certainly we don't in our previous code. I don't think we check revocation status on explicitly trusted intermediates either.

bob

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch (as checked in)

I removed the "XXX comment" (a question for Bob) and checked this
patch in on the NSS trunk (NSS 3.13.4).

Checking in pkix_build.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c,v  <--  pkix_build.c
new revision: 1.62; previous revision: 1.61
done

Comment 11

[Wan-Teh Chang]

Attachment: Revert the incorrect reference count bug fixes

The reference count bugs I mentioned in comment 7 and comment 8
are not bugs.  The PKIX_DECREF macro sets its argument to NULL
after decrementing the reference count.

Patch checked in on the NSS trunk.

Checking in pkix_build.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c,v  <--  pkix_bui
ld.c
new revision: 1.63; previous revision: 1.62
done