The default bug view has changed. See this FAQ.

libpkix does not check cached cert chain for revocation

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 2 obsolete attachments)

24.09 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Priority: -- → P1
Whiteboard: PKIX

Comment 1

10 years ago
Good catch, Alexei.
(Assignee)

Updated

10 years ago
Blocks: 390888
Version: 3.12 → trunk
(Assignee)

Updated

10 years ago
Priority: P1 → P2
(Assignee)

Comment 2

10 years ago
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.
Attachment #280403 - Flags: review?(nelson)
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 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.
Attachment #280403 - Flags: review?(nelson) → review-
(Assignee)

Updated

10 years ago
Blocks: 396598
(Assignee)

Updated

10 years ago
No longer blocks: 390888
(Assignee)

Updated

10 years ago
Priority: P2 → P1
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
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.
Attachment #280403 - Attachment is obsolete: true
Attachment #282190 - Flags: review?(nelson)
(Assignee)

Comment 7

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

got r- from Nelson for a several reasons. Will provide new patch shortly.
Attachment #282190 - Flags: review?(nelson) → review-
(Assignee)

Comment 8

10 years ago
Created attachment 282612 [details] [diff] [review]
Patch v3
Attachment #282190 - Attachment is obsolete: true
Attachment #282612 - Flags: review?(nelson)
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.
Attachment #282612 - Flags: review?(nelson) → review+
(Assignee)

Comment 10

10 years ago
bug 397832 is created to take care of leaks create by processing "fatal" errors.
(Assignee)

Comment 11

10 years ago
patch v3 integrated into trunk.
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Summary: lib pkix does not check cached cert chain for revocation → libpkix does not check cached cert chain for revocation
You need to log in before you can comment on or make changes to this bug.