363749 - SEC_FindCrlByKeyOnSlot never finds a CRL on a slot since it incorrectly forms decodeoption flag

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

In function  SEC_FindCrlByKeyOnSlot we check decodeoptions for having CRL_DECODE_DONT_COPY_DER bit set and if it was not set, then set CRL_DECODE_ADOPT_HEAP_DER. Then we call CERT_DecodeDERCrlWithFlags with
the modified "decodeoptions" flag.

http://lxr.mozilla.org/security/source/security/nss/lib/certdb/crl.c#656

CERT_DecodeDERCrlWithFlags is its turn check for "decodeoptions" flag and
sets error if the flag has CRL_DECODE_ADOPT_HEAP_DER bit set without CRL_DECODE_DONT_COPY_DER.

This makes CERT_DecodeDERCrlWithFlags always return an error then called from SEC_FindCrlByKeyOnSlot, whenever CRL_DECODE_DONT_COPY_DER bit is not set in decodeoptions.

Since SEC_FindCrlByKeyOnSlot dose not check for error code, it does not notice that CERT_DecodeDERCrlWithFlags returns pointer to crl qe to NULL for the reason of bad input parameters and not because a crl was not found on the slot.

I think the bug should be fixed by making code of SEC_FindCrlByKeyOnSlot to create proper decodeoptions for CERT_DecodeDERCrlWithFlags function.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Alexei, 
SEC_FindCrlByKeyOnSlot is only called by crl_storeCRL which is only called 
by PK11_ImportCRL.  

So, what is the implication of this bug? 
That CRLs are not imported?
That CRLs are not deleted when a newer CRL from the same issuer is imported?

Also, please change the version number for this bug to be the version of NSS
in which this flaw first appeared.  We haven't shipped NSS 3.12 yet, so it 
cannot be 3.12.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Julien, Not clear what the right thing is to do here.  You advice on fixing SEC_FindCrlByKeyOnSlot and/or CERT_DecodeDERCrlWithFlags would be appreciated.

Comment 3

[Alexei Volkov]

(In reply to comment #1)
> So, what is the implication of this bug? 
> That CRLs are not imported?
> That CRLs are not deleted when a newer CRL from the same issuer is imported?

Any time PK11_ImportCRL was called on a crl, the crl was imported regardless possibly old update time of the loading crl. I was done based on negative search result from SEC_FindCrlByKeyOnSlot.

Comment 4

[Julien Pierre]

I haven't looked at any of the code recently, but as I recall, CRL_DECODE_ADOPT_HEAP_DER means that the decoded CRL now owns the memory for the SECItem DER buffer, and destroying that CRL will free the DER memory using a PORT function.

CRL_DECODE_DONT_COPY_DER means that the input DER is not copied into the CRL's arena.

It is not possible to adopt the memory if we make a copy, so CERT_DecodeDERCrlWithflags is correct to require CRL_DECODE_DONT_COPY_DER if CRL_DECODE_ADOPT_HEAP_DER is set.

So, the bug is in SEC_FindCrlByKeyOnSlot, it should set both flags if neither was set in the input decodeoptions.

Comment 5

[Alexei Volkov]

Attachment: form proper decodeoptions

The fix reveals that crl generation and crl testing also have some flaws. This patch accumulates fixes for them.

first: thisUpdate time of crl should be set explicitly. But the time is set in Zulu.
second: tests needed to be modified to use update command to set thisUpdate time to avoid problems in time differences between Zulu time and local time.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 248572 [details] [diff] [review]
form proper decodeoptions

I'm giving  this patch r- for a bunch of reasons, including:

1) This patch tries to fix a bunch of issues that are not within 
the scope of this bug.  This bug report is about a problem in the
NSS shared libraries, but the path also changes a test program and
a test script.  The changes to the test code should be in one or 
two separate bugs.  The patch for this bug should only address the 
problem reported in this bug.  

2) This change can't be right:

>-    SEC_FindCrlByKeyOnSlot(slot, &newCrl->crl.derName, type,
>+    rv = SEC_FindCrlByKeyOnSlot(slot, &newCrl->crl.derName, type,
>                                 &oldCrl, CRL_DECODE_SKIP_ENTRIES);
>+    if (rv == SECFailure) {
>+        /* CRL was not found. Let check if error code was set. */
>+        if (PORT_GetError() != 0) {
>+            return NULL;
>+        }
>+    }

This change implies that SEC_FindCrlByKeyOnSlot can return SECFailure
with an error code of zero.  That should be a fatal error.  The code
should make a core file if that happens.  The code should not try
to make that be an OK acceptable event.  SEC_FindCrlByKeyOnSlot must
either return SECSuccess or SECFailure with a non-zero error code.

I suspect that this is a case where we're trying to distinguish 
among 3 different cases:
a) found a CRL
b) didn't find a CRL, but no error occurred.  The CRL just wasn't there.
c) some processing error occurred that prevented us from returning the
   decoded CRL.

IMO, for case b, we should return SECSuccess and a NULL value for the 
pointer to the returned CRL structure.  Perhaps the NSS group needs to 
discuss this, especially if this appears to be a change in the spec for
this function or some subfunction.

3) I'm not yet convinced that this change is the right solution to the 
decodeoptions problem.  I am discussing this with Julien.  

>-    if (!(decodeoptions & CRL_DECODE_DONT_COPY_DER) ) {
>+    if (decodeoptions & CRL_DECODE_DONT_COPY_DER) {
>         /* force adoption of the DER from the heap - this will cause it to be
>            automatically freed when SEC_DestroyCrl is invoked */
>         decodeoptions |= CRL_DECODE_ADOPT_HEAP_DER;
>     }
I think that whole block should probably just be deleted.  The decoder 
should ensure that it doesn't copy if/when it adopts.  See below.

Julien and I have agreed that any time the ADOPT option flag is set, the 
DONT_COPY option should also be set.  That should be done in the decoder.
The decoder should have code that reads like this:
      if (decodeoptions & CRL_DECODE_ADOPT_HEAP_DER) 
          decodeoptions |= CRL_DECODE_DONT_COPY_DER;

I think the root of this problem is the set of options that crl_storeCRL 
passes to SEC_FindCrlByKeyOnSlot.  Present it passes CRL_DECODE_SKIP_ENTRIES
which effectively means:  copy CRL, don't adopt, skip decoding of entries. 
I think we don't want to copy here, and neither do we want to adopt.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

Julien and I discussed this at length today.  
I think this patch accurately reflects what we agreed. 
I'm going to ask Julien to review it first, to make sure I didn't 
misunderstand anything first.  

Alexei says we need the NSS test programs and scripts fixed before we can 
apply and test this (or any) fix to this bug.  I believe Alexei is going to 
file a separate bug about the testing issues, which will block this bug.
I haven't tested this patch yet, but will when Alexei has filed his patch
against his new test bug.

Comment 8

[Wan-Teh Chang]

Comment on attachment 248705 [details] [diff] [review]
patch v2

This patch has a bug.  In CERT_DecodeDERCrlWithFlags

>+    /* Adopting DER requires not copying it.  Code that sets ADOPT flag 
>+     * but doesn't set DONT_COPY probably doesn't know What it is doing.  
>+     * That condition is a programming error in the caller.
>+     */
>+    testOptions &= (CRL_DECODE_ADOPT_HEAP_DER | CRL_DECODE_DONT_COPY_DER);
>+    PORT_Assert(options != CRL_DECODE_ADOPT_HEAP_DER);
>+    if (options == CRL_DECODE_ADOPT_HEAP_DER) {
>         PORT_SetError(SEC_ERROR_INVALID_ARGS);
>         return NULL;
>     }

In the PORT_Assert and if statements, 'options' should be 'testOptions'.
In the comment, 'What' should be 'what' (not capitalized).

The original code, which tests the two bit flags separately, is easier
to understand.

In SEC_FindCrlByKeyOnSlot:

>-    /* XXX it would be really useful to be able to fetch the CRL directly into
>-       an arena. This would avoid a copy later on in the decode step */
>     PORT_SetError(0);
>     derCrl = PK11_FindCrlByName(&slot, &crlHandle, crlKey, type, &url);
>     if (derCrl == NULL) {
> 	/* if we had a problem other than the CRL just didn't exist, return
> 	 * a failure to the upper level */
>-	nsserror = PORT_GetError();
>-	if ((nsserror != 0) && (nsserror != SEC_ERROR_CRL_NOT_FOUND)) {
>+	int nsserror = PORT_GetError();
>+	PORT_Assert(nsserror);
>+	if (!nsserror)
>+	    PORT_SetError(nsserror = SEC_ERROR_CRL_NOT_FOUND);
>+	if (nsserror != SEC_ERROR_CRL_NOT_FOUND) {
> 	    rv = SECFailure;
> 	}
> 	goto loser;
>     }

The PORT_SetError call around "nsserror = SEC_ERROR_CRL_NOT_FOUND" is
not necessary because we test nsserror, not the return value of another
PORT_GetError call, next and this function only fail if nsserror is not
SEC_ERROR_CRL_NOT_FOUND.  If you want to make sure PK11_FindCrlByName
always sets the error code on failure, it's better to do that inside
PK11_FindCrlByName as follows:

- change the five "return NULL" statements to "goto loser".
- add the following at the end of the function:

  loser:
      if (PORT_GetError() == 0) {
          PORT_SetError(SEC_ERROR_CRL_NOT_FOUND);
      }
      return NULL;

Clearly there are more elegant ways to fix PK11_FindCrlByName.

Comment 9

[Wan-Teh Chang]

Comment on attachment 248705 [details] [diff] [review]
patch v2

Continuing my previous comment: we'd also need to move the PORT_SetError(0) call to
the beginning of the PK11_FindCrlByName function.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - incorporate Wan-Teh's suggestions

This newer patch incorporates Wan-Teh's comments.

Comment 11

[Wan-Teh Chang]

Comment on attachment 250339 [details] [diff] [review]
patch v3 - incorporate Wan-Teh's suggestions

You forgot to remove the following code from SEC_FindCrlByKeyOnSlot:

>+	if (!nsserror)
>+	    PORT_SetError(nsserror = SEC_ERROR_CRL_NOT_FOUND);

Comment 12

[Alexei Volkov]

Comment on attachment 250339 [details] [diff] [review]
patch v3 - incorporate Wan-Teh's suggestions

I didn't find any problems with the patch, but used CRL store procedure has a flaw.

From comments to SEC_FindCrlByKeyOnSlot:
"SECFailure means we got a fatal error - most likely, we found a CRL,
and it failed decoding, or there was an out of memory error. Do NOT ignore and it failed decoding, or there was an out of memory error. Do NOT ignore it and specifically do NOT treat it the same as having no CRL".

Guess what, our code completely ignores the error code returned by SEC_FindCrlByKeyOnSlot, and the patch does not solves the problem, resulting in rejecting a good CRL from installing on a slot.

I think this patch could've also fixed this problem.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Re: comment 11,
I didn't forget to remove it.  It is necessary to ensure that this function
sets the right error code if/when PK11_FindCrlByName returns NULL without 
setting the error code.  

Now, admittedly, this patch tries to prevent PK11_FindCrlByName from ever 
doing that.  But I'm just following the rule that says that every assert
should be followed by code that detects the same problem and handles it 
correctly in non-DEBUG builds.  If I was going to remove that assertion,
I would remove this code too.  Do you think I should do that?

Comment 14

[Wan-Teh Chang]

Yes, you can remove that assertion.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v4 - incorporate additional suggestions from Wan-Teh and Alexei (checked in)

This patch incorporates the latest suggestions from Wan-Teh and Alexei.
Since they both gave r+ to patch v3, I will commit this patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Checked in.
> When storing new CRL, Find old CRL and if it can be decoded, delete it.
> Bug 363749. r=wtchang,alexei.volkov

pk11wrap/pk11nobj.c; new revision: 1.6.2.2;   previous revision: 1.6.2.1
certdb/crl.c;        new revision: 1.49.24.5; previous revision: 1.49.24.4

pk11wrap/pk11nobj.c; new revision: 1.9; previous revision: 1.8
certdb/crl.c;        new revision: 1.55; previous revision: 1.54

Alexei observes that if we can find a (old) CRL for this issuer in the DB,
but cannot decode it, we will not delete it from the DB.  This may cause a
failure to store the new CRL in the DB.  I would hope that we never put an 
undecodable CRL into the DB, but I agree that we should handle this case.
So I am leaving this bug open for now.  This patch fixes the problem for 
the case where the old CRL is decodable, which should be the normal case,
so I am comfortable with committing it, even if it's not a 100% complete fix.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

I'm going to mark this bug resolved/fixed.  
If undecodable CRLs are later found to be a problem, we'll file a separate
bug on that.