130983 - function 'SEC_FindCrlByName' in crl.c does not set 'pkcs11ID' field in the 'CERTSignedCrl' it returns

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Rangan Sen]

This makes subsequent calls to 'SEC_DeletePermCRL' [using the 'CERTSignedCrl'
returned by 'SEC_FindCrlByName'] fail.

This blocks PSM bug# 130979

Comment 1

[Rangan Sen]

Attachment: Suggested Patch

Bob, can you please review...

Comment 2

[Robert Relyea]

Good catch, We must be getting CRLs some other way when delete *was* working for
CRLs. 

It looks like we have a similar problem in PK11_LookupCrls.
Also it looks like SEC_DeletePermCRL is not returning an erro code on failure.
I'll supply patches for these problems as well.

bob

Comment 3

[Robert Relyea]

Rangen, how are you freeing the CRLHeadNodes from PK11_LookupCrls?

bob

Comment 4

[Rangan Sen]

By calling PORT_FreeArena(head->arena, PR_FALSE);

Comment 5

[Wan-Teh Chang]

Comment on attachment 74131 [details] [diff] [review]
Suggested Patch

>+	if(crlHandle != CK_INVALID_HANDLE)
>+	 crl->pkcs11ID = crlHandle;

Can we simply say
	crl->pkcs11ID = crlHandle;
?

Do we not want to override the old value of crl->pkcs11ID
if crlHandle is invalid?

Comment 6

[Robert Relyea]

it should assert, or completely fail. assert is probably better, since
PK11_FindCrlByName does not return the DERCrl unless the crlHandle is valid.

bob

Comment 7

[Robert Relyea]

OK, that's what I was afraid of.

It looks like you can't call SEC_DeletePermCert on any of these CRLs. If I setup
the slot and handle, then we will leak references to the slot whenever we call
this function.

bob

Comment 8

[Rangan Sen]

OK, I'll update the patch to assert ....

Comment 9

[Wan-Teh Chang]

We should not use assertions to check for invalid input
arguments.  Invalid input arguments should be reported
with a failure status and the appropriate error code.

We should only use assertions to detect internal errors
or when there is no way to report an error (e.g, in a
function that returns void).

Comment 10

[Robert Relyea]

It's not an input value, it's generated by a function which we call withint this
function to retrieve the crl.
We should either assert or fail outright, though it would be an internal error
if we ever hit this condition.

Comment 11

[Rangan Sen]

Attachment: updated patch

I agree ... if PK11_FindCrlByName returns non-null, and crlhandle is still
invalid, it is an internal error - and it would make sense putting an assert
right there...

Comment 12

[Robert Relyea]

Comment on attachment 74361 [details] [diff] [review]
updated patch

Patch looks good rangen r=relyea

Comment 13

[Wan-Teh Chang]

I checked in Rangan's patch.

Comment 14

[Rangan Sen]

Thanks .... small correction, Bob - its Rangan, not Rangen :)