390499 - libpkix does not check cached cert chain for revocation

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

libPkix cached all successfully validated chains. The problem uncovers when such successfully validated chain has been saved in the cache before a crl was updated.
As a result invalid chains with revoked certs reported us valid.

Comment 1

[Julien Pierre]

Good catch, Alexei.

Comment 2

[Alexei Volkov]

Attachment: initial patch

The are few changes here related to subject of the bug:
   * Moving of cert chain cache code into a function (pkix_Build_CheckInCache). The main changes here are to include revocation checker into the list of cert chain checkers and make pkix_Build_ValidateEntireChain do chain revocation checking(both those things are done by setting state->revCheckDelayed and state->revChecking flags respectively). Also, the function now removes cert chains that were invalidated revocation checker(in addition to chains that have root cert we don't trust any more).
   * chain validity period is now set by default to 8 hours, but can be decreased by update date of a crl or notAfter date of cert.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280403 [details] [diff] [review]
initial patch

My review of this patch is not yet complete.  But, 
Here are some review comments:

1. Including "pkix_pl_nsscontext.h" in pkix.h seems very inappropriate.
Code outside of the PL layer should not need to know how the PL Layer's
nsscontext is defined.  

Why does the patch do that?  What problem does it solve?
Is it necessary?  Can we just not do it?

2. This patch uses the word "offset" in many place to refer to something
that is measured in seconds.  The value commonly assigned to it has a 
symbolic name that uses the word "PERIOD", which seems more appropriate.
I think (but am not sure) that it is a cache entry lifetime.  If so, it 
must use a more appropriate name than "offset, such as "lifetime" (preferred)
or "period" or "interval", everywhere.

3. The effect of NIST policy on CRLs seems to be different than on Certs.
That is, we seem to be enforcing different policies for the two.  Why?

4. why does this patch change pkix_Build_ValidateEntireChain to set an error
string?  I thought we eliminated libPKIX's internal use of error strings.

5. Please explain the patch's change to function PKIX_PL_HashTable_Remove(.
What was the problem with the old code?
Does the proposed patch introduce any leak?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280403 [details] [diff] [review]
initial patch

Please see my five review comments in comment 3, plus these additional 
comments.

6. It appears that pkix_CacheCert_Lookup leaks objects, even after your
patch.  Let's go over that function together.

7.  There is a macro to convert from PRTime to seconds.  Please use it.
>+            diff = (topLimit - timeToCheck) / 1000000; /* convert to seconds */

My review comments have asked several questions about the code.
Please answer them in the next comment to this bug.  Thanks.

Comment 5

[Alexei Volkov]

This bug tries to resolve two issues: one - the bug regarding the fact that libpkix does not check cached cert chain against crl; second - enhancement request to add a way to control cached interval.

Since the enhancement is not needed to be fixed to run all.sh, but still required some work to be done to be complete, the new bug has been created( bug 397405 ) to handle the enhancement.

Comment 6

[Alexei Volkov]

Attachment: Patch v2.

Isolated patch that solves cached cert chain revocation checking.

Also memory leaks/null pointed dereferencing clean up in pkix_hashtable.c: remove function was coded to free pointers to keys object created by caller.

Comment 7

[Alexei Volkov]

Comment on attachment 282190 [details] [diff] [review]
Patch v2.

got r- from Nelson for a several reasons. Will provide new patch shortly.

Comment 8

[Alexei Volkov]

Attachment: Patch v3

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 282612 [details] [diff] [review]
Patch v3

There is still an issue that errors of the so-called "fatal" class cause leaks, but that is a pre-existing bug, not new to this patch, and needs to be addressed systemically throughout libPKIX.  Please file another bug about that, and put it in the correctness category.

Comment 10

[Alexei Volkov]

bug 397832 is created to take care of leaks create by processing "fatal" errors.

Comment 11

[Alexei Volkov]

patch v3 integrated into trunk.