delay child certificate signature check until after trusted anchor is found

RESOLVED FIXED in 3.12.3

Status

NSS
Libraries
P1
major
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos] PKIX SUN_MUST_HAVE; keep sensitive until 356756 fixed or unhidden)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Unless the signature check is delayed, libpkix will try to validate signatures on the certificated in the chain during chain building before a trusted anchor has been found. There is a possibility for DoS attack based on high CPU consumption during cert signature check using big(max) keys. (see CVE-2006-2940 for the details).

Libpkix is capable to delay signature check, until after a trust anchor is found, but the current code does it only exceptionally for the dsa certs that where missing dsa params during chain building process(bug 461544). Will need to modify it to delay signature check for all the certs.
(Assignee)

Updated

9 years ago
Severity: normal → major
Priority: -- → P1
(Assignee)

Updated

9 years ago
Whiteboard: PKIX SUN_MUST_HAVE
This is a duplicate of bug 356756, right??
(Assignee)

Comment 2

9 years ago
Yes it is. This bug limit the scope of the problem to libpkix. Our old code will be still vulnerable after a fix to libpkix is integrated. I'd have this bug as blocking for bug 356756.
Blocks: 356756
Whiteboard: PKIX SUN_MUST_HAVE → [sg:low dos] PKIX SUN_MUST_HAVE
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:low dos] PKIX SUN_MUST_HAVE → [sg:dos] PKIX SUN_MUST_HAVE
Dan, the solution for the browser will necessitate that the browser 
switch to using libPKIX exclusively for cert verification and stop 
using the old cert verification API, which will always have this problem.
(We're not investing heavily in the old code at this time, so the fix will
go into libPKIX and those who want it must switch.)

Now, switching to libPKIX is not as difficult as you might guess, but it 
requires work outside of NSS.  Maybe this bug (or bug 356756) should morph 
into a PSM bug for that change?
(Assignee)

Comment 4

9 years ago
Created attachment 357199 [details] [diff] [review]
Patch 1 - postpone cert signature check untill chain validation phase. 

Number of things in this patch:

* postpone signature check: removes code that checks certificate signature during chain building(functions pkix_Build_CheckCertAgainstAnchor and pkix_Build_VerifyCertificate), and modifies code of pkix_Build_ValidationCheckers to enable cert signature check during chain validation phase.

* fixs object leak in function pkix_Build_ValidateEntireChain where subjPubKey value was overwritten and lost.

* Fix to in pkix_build.c to return a proper chain validation error.

* Adds test to a chains suite with attempt to validate a cert with a corrupt signature.
Attachment #357199 - Flags: review?(nelson)
(Assignee)

Comment 5

9 years ago
Comment on attachment 357199 [details] [diff] [review]
Patch 1 - postpone cert signature check untill chain validation phase. 

Additional changes are needed to the fix. Canceling the review.
Attachment #357199 - Flags: review?(nelson) → review-
(Assignee)

Comment 6

9 years ago
Created attachment 358932 [details] [diff] [review]
Patch to crl.c - Do not validate crl if validation date is not specified.

We also verify crl signature when we build the chain. This is done if a new crl was placed into crl cache and the cache lock was acquired by pkix crl check call. At this time we bring cache up to date by making sure we have decoded and verified all the imported crls that are related to an issuer.

IMHO we do not need to delay the signature check on the crl as a security measure, since the signature will be verified only with valid cert. Still we may want to decide to delay signature check in libpkix when we building chain up to the trusted root. This way we will speed up the chain building process, result of which can be useful for over chain verification attempts.

Anyway, while going through the crl cache code I found that FetchFromTokens function tries to verify crl even when verify date is set to 0 - the action that basically completely invalidates the cache for a particular issuer. I think this is the bug and I'm attaching the patch to fix it.
Attachment #358932 - Flags: review?(julien.pierre.boogz)
Version: unspecified → trunk

Comment 7

9 years ago
Comment on attachment 358932 [details] [diff] [review]
Patch to crl.c - Do not validate crl if validation date is not specified.

There are a couple things wrong with this patch.

1) Testing is vfdate is zero is the wrong test. vfdate is a PRTime, and any value specifies a particular time. There is no reserved value. I think you may want to change the API to pass a PRTime* instead, and test for a NULL pointer. This would need to be propagated all the way in the CRL cache API. This change is probably OK, since it's a private API.

2) you need to make sure the CRL cache object for the issuer is not left or put in the valid state. I think that means you may need to set rv to SECFailure if the date is not passed and the CRL is unverified.
Attachment #358932 - Flags: superreview-
(Assignee)

Comment 8

9 years ago
Created attachment 359320 [details] [diff] [review]
Patch v2 - delay signature check on certs and crls while building the chian
Attachment #357199 - Attachment is obsolete: true
Attachment #359320 - Flags: review?(nelson)
(Assignee)

Comment 9

9 years ago
(In reply to comment #7)
> (From update of attachment 358932 [details] [diff] [review])
> There are a couple things wrong with this patch.
> 
> 1) Testing is vfdate is zero is the wrong test. vfdate is a PRTime, and any
> value specifies a particular time. There is no reserved value.

I've created a patch following your logic at crl.c:1913 where you are using 0 time as indicator of date that was unavailable:

    /* verify CRLs that couldn't be checked when inserted into the cache
       because the issuer cert or a verification date was unavailable.
       These are CRLs that were inserted into the cache through
       SEC_FindCrlByName, or through manual insertion, rather than through a
       certificate verification (CERT_CheckCRL) */

    if (cache->issuer && vfdate ) {
                    ...
                    CachedCrl_Verify(cache, savcrl, vfdate, wincx);
                    ...
    }

Agree, that 0 PRTime is a valid time, but in this case zero time is good as a NULL. The time is used for checking that it is with in crl issuer validity dates.

> 2) you need to make sure the CRL cache object for the issuer is not left or put
> in the valid state. I think that means you may need to set rv to SECFailure if
> the date is not passed and the CRL is unverified.

The only function that can put it into the valid state is the function that get skipped(CachedCrl_Verify) if the time is unavailable. No need to return SECFailure. In it enough that cache for the issuer is marked as invalid in case when signature was not checked/or invalid on any of the crls that present in the cache.

IMHO the patch does not do any harm. I agree that the perfect patch would change the PRTime argument to PRTime*, but this patch fixes the problem that the code had before, and that is putting the crl cache into permanent invalid state if the first call to acquire the cache was not supplied with valid date.

Comment 10

9 years ago
Comment on attachment 358932 [details] [diff] [review]
Patch to crl.c - Do not validate crl if validation date is not specified.

Alexei,

1) You are right, looks I may have been overzealous in my review with the PRTime test.

2) The intent is for the CRL cache to only be valid if the state is consistent, ie. all CRLs have successfully fetched and verified. If the cache was previously in the valid state, and you add a CRL that cannot be verified, then you need to ensure that the cache will be put into the invalid state at the next lookup. It looks like that should automatically happen in DPCache_GetUpTodate, so no further code changes are needeed.
Attachment #358932 - Flags: superreview-
Attachment #358932 - Flags: review?(julien.pierre.boogz)
Attachment #358932 - Flags: review+
Comment on attachment 359320 [details] [diff] [review]
Patch v2 - delay signature check on certs and crls while building the chian

One line was accidentally deleted from pkix_ForwardBuilderState_Create(
line 272.  After putting it back, r=nelson
Attachment #359320 - Flags: review?(nelson) → review+
(Assignee)

Comment 12

9 years ago
Created attachment 359982 [details] [diff] [review]
Additional patch to fix tests failures.
Attachment #359982 - Flags: review?(nelson)
Comment on attachment 359982 [details] [diff] [review]
Additional patch to fix tests failures.

Why did this patch land before it was reviewed?
Comment on attachment 359982 [details] [diff] [review]
Additional patch to fix tests failures.

r=nelson
Alexei, thanks for fixing the tree over the weekend with this low risk patch.  
I think you made the right judgement call to commit it while awaiting review.  We could always have backed out both patches if needed.
Attachment #359982 - Flags: review?(nelson) → review+
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:dos] PKIX SUN_MUST_HAVE → [sg:dos] PKIX SUN_MUST_HAVE; keep sensitive until 356756 fixed or unhidden
Group: core-security
You need to log in before you can comment on or make changes to this bug.