397805 - Avoid revalidating the whole cached chain for new cert KU and EKU

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Alexei Volkov]

Libpkix tries to revalidate chain that was already successfully validated and places in cache. The are several reasons for that: one) need to insure that chain valid for requested key usages and extended key usages; two) check to changed revocation status of the cert; three) perform check for the new set of policies. 

Because cached chain revalidation we are loosing most of all performance improvements that we gain by caching valid chain.

Need to come up with new approach to minimize number of things that we are rechecking on already validated chains.

Comment 1

[Julien Pierre]

Alexei,

In principle, given 2 identical queries, the only things that need to be rechecked are trust and revocation - things that are not part of the chain itself.

For different queries (KU, EKU, policies) there should be separate cache entries, IMO. If the chain cache does not have enough granularity, it should be extended.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The question is: will this bug lead to false positives?
If we have a chain that is (say) valid for SSL but not S/MIME, and we check 
it first for SSL, will a subsequent check for S/MIME show it to also be 
valid for that usage?  
If yes, this is P1 for the next 3.12.x release
If no, then this is mostly a performance enhancement.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

If we determine that this bug does not result in false positives, then
the "security sensitive" flag can be removed from this bug.

Comment 4

[Brandon Sterne (:bsterne)]

Who can determine the answer to the false positive question raised in comment 3?  Whether or not we decide to unhide this bug, we should assign a severity rating.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

My questions in comment 2 and comment 3 are for Alexei.

Comment 6

[Alexei Volkov]

Attachment: Patch v1 - make libpkix check eku of cached chain

This patch does not solve the problem of revalidation for different eku of the whole cached chain. This patch makes sure we check eku of the cached chain.

"Security cencetive" flag can be dropped one this patch is integrated.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 350180 [details] [diff] [review]
Patch v1 - make libpkix check eku of cached chain

This patch moves two file from one directory to another, and it changes a bunch of other files.  The files being moved are not only being moved, but big changes are made to them as well.
I have reviewed the changes to the files that are not being moved and those changes all seem good.  r+ for those changes.
However, it is difficult to review the changes to the files that are being moved.  So, I have not yet reviewed those. 
So, overall, this patch still does not have a completed review.

Comment 8

[Alexei Volkov]

Attachment: difference between versions of ekuchecker.[ch]

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 353115 [details] [diff] [review]
difference between versions of ekuchecker.[ch]

It seems that the RegisterSelf function got moved to a different 
place in the source file.  Please move it back to its old place, 
so that the diff will show how it has changed, and regenerate this 
diff.  

One more observation: This comment bears the wrong function name.

> /*
>- * FUNCTION: pkix_pl_EkuChecker_Initialize
>+ * FUNCTION: pkix_EkuChecker_Initialize
>  * (see comments in pkix_sample_modules.h)
>  */
> PKIX_Error *
>-PKIX_PL_EkuChecker_Create(
>+PKIX_EkuChecker_Create(
>         PKIX_ProcessingParams *params,
>+        PKIX_CertChainChecker **pEkuChecker,
>         void *plContext)

Comment 10

[Alexei Volkov]

Attachment: differences between ekuchecker.[ch]. patch 2

Comment 11

[Alexei Volkov]

Comment on attachment 355472 [details] [diff] [review]
differences between ekuchecker.[ch]. patch 2

This patch looks better. Requesting review.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 355472 [details] [diff] [review]
differences between ekuchecker.[ch]. patch 2

r=nelson.
I suggested a few cosmetic changes.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 350180 [details] [diff] [review]
Patch v1 - make libpkix check eku of cached chain

r=nelson

Comment 14

[Alexei Volkov]

attachment 350180 [details] [diff] [review] is integrated.

Comment 15

[Alexei Volkov]

Patch fixes the problem of possible false positive answer when verifying chain with a different EKU. The remain work will be done to optimizing verification phase, but it will not be done for 3.12.3 release.

Lowing the priority of the bug, but still keeping security-sensitive flag since firefox still uses old version of the library where the bug is not fixed.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Alexei, I'm marking this bug fixed in 3.12.3.  
Please keep it security-sensitive. 
Please file a new bug for the optimizations you mentioned in comment 15.
You can decide if that new bug must also be security sensitive or not.

Comment 17

[Daniel Veditz [:dveditz]]

The reason for keeping security-sensitive in comment 15 is no longer true