Last Comment Bug 390499 - libpkix does not check cached cert chain for revocation
: libpkix does not check cached cert chain for revocation
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
P1 normal (vote)
: 3.12
Assigned To: Alexei Volkov
Depends on:
Blocks: 396598
  Show dependency treegraph
Reported: 2007-08-01 12:01 PDT by Alexei Volkov
Modified: 2007-09-28 09:55 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

initial patch (40.65 KB, patch)
2007-09-10 16:41 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v2. (24.49 KB, patch)
2007-09-24 17:03 PDT, Alexei Volkov
alvolkov.bgs: review-
Details | Diff | Splinter Review
Patch v3 (24.09 KB, patch)
2007-09-27 14:55 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description User image Alexei Volkov 2007-08-01 12:01:01 PDT
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 User image Julien Pierre 2007-08-01 15:00:55 PDT
Good catch, Alexei.
Comment 2 User image Alexei Volkov 2007-09-10 16:41:12 PDT
Created attachment 280403 [details] [diff] [review]
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 User image Nelson Bolyard (seldom reads bugmail) 2007-09-12 17:28:13 PDT
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 User image Nelson Bolyard (seldom reads bugmail) 2007-09-17 18:55:47 PDT
Comment on attachment 280403 [details] [diff] [review]
initial patch

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

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 User image Alexei Volkov 2007-09-24 14:33:42 PDT
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, 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 User image Alexei Volkov 2007-09-24 17:03:38 PDT
Created attachment 282190 [details] [diff] [review]
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 User image Alexei Volkov 2007-09-27 14:54:28 PDT
Comment on attachment 282190 [details] [diff] [review]
Patch v2.

got r- from Nelson for a several reasons. Will provide new patch shortly.
Comment 8 User image Alexei Volkov 2007-09-27 14:55:35 PDT
Created attachment 282612 [details] [diff] [review]
Patch v3
Comment 9 User image Nelson Bolyard (seldom reads bugmail) 2007-09-27 15:53:32 PDT
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 User image Alexei Volkov 2007-09-27 16:38:41 PDT
bug 397832 is created to take care of leaks create by processing "fatal" errors.
Comment 11 User image Alexei Volkov 2007-09-27 16:41:44 PDT
patch v3 integrated into trunk.

Note You need to log in before you can comment on or make changes to this bug.