287061 - CRL number should be a big integer, not ulong

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Hanfei Yu]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20041214

 
in certhigh/crlv2.c:

 CERT_FindCRLNumberExten (CERTCrl *crl, CERTCrlNumber *value) 
 decodes and returns CRL number as an integer. From RFC3280, Section 5.2.3,
 CRL number can be a values of 20 octets.


Reproducible: Always

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

-> Julien

Comment 2

[Julien Pierre]

This is a valid bug. However, currently, this function isn't exported from NSS,
nor is it used internally, so nobody can be running into this problem.
We should not export this function as-is, since the return type of the prototype
is incorrect.

A new function should be provided that returns a type usable for big numbers.
Based on my conversation with Nelson earlier today, it would probably be a
SECItem, which could be tested with SECItem_CompareItem . However, that only
works right if the octet string of the CRL number is unsigned. If not, special
decoding would be needed.

RFC3280 defines CRLNumber as

CRLNumber ::= INTEGER (0..MAX)

Therefore, I think a SECItem would work.

Comment 3

[Hanfei Yu]

Julien:

 I had the following functions in crlv2.c. (I renamed this function to
differentiate from the original one) and it had been tested with NIST CRL's.

********************** crlv2.c

SECStatus CERT_FindCRLNumberBytesExten (CERTCrl *crl, SECItem *value)
{
    SECItem encodedExtenValue;
    SECStatus rv;

    encodedExtenValue.data = NULL;
    encodedExtenValue.len = 0;

    rv = cert_FindExtension
	 (crl->extensions, SEC_OID_X509_CRL_NUMBER, &encodedExtenValue);
    if ( rv != SECSuccess )
	return (rv);

    rv = SEC_ASN1DecodeItem (NULL, value, SEC_IntegerTemplate,
			     &encodedExtenValue);
    if ( rv != SECSuccess )
	return (rv);

    PORT_Free (encodedExtenValue.data);
    return (rv);
}

Comment 4

[Julien Pierre]

Hanfei,

Please attach a patch generated with cvs diff -u10 . Please remove the old
function in your patch since it is unused.

Thanks.

Comment 5

[Hanfei Yu]

Attachment: diff with CRL number big int fix

Comment 6

[Wan-Teh Chang]

Comment on attachment 178489 [details] [diff] [review]
diff with CRL number big int fix

>-SECStatus CERT_FindCRLNumberExten (CERTCrl *crl, CERTCrlNumber *value)
>+SECStatus CERT_FindCRLNumberBytesExten (CERTCrl *crl, SECItem *value)

It is not necessary to rename this function because
it hasn't been exported.  We should remove the
CERTCrlNumber type though.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 178489 [details] [diff] [review]
diff with CRL number big int fix


>     rv = SEC_ASN1DecodeItem (NULL, value, SEC_IntegerTemplate,

Please change this to use a non-NULL arena. 
Basically we don't ever want to use the ASN1 decoders without arenas.
Thanks.

> 			     &encodedExtenValue);
>+    if ( rv != SECSuccess )
>+	return (rv);
>+

Comment 8

[Julien Pierre]

Also, since the CRL is DER-encoded, you can use the faster
SEC_QuickDERDecodeItem . Be aware it requires a non-NULL arena argument.

Comment 9

[Julien Pierre]

Attachment: patch including suggestions

As checked in on NSS_LIBPKIX_BRANCH .

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--	cert.h
new revision: 1.53.10.2; previous revision: 1.53.10.1
done
Checking in certhigh/crlv2.c;
/cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v  <--  crlv2.c
new revision: 1.3.10.1; previous revision: 1.3
done

Comment 10

[Julien Pierre]

Attachment: just export CERT_FindCRLNumberExten from nss.def

This patch won't apply to anything useful in the future, but I wanted to have a
record that the previous patch was incomplete since it was missing this export.


I checked this in to the NSS_LIBPKIX_BRANCH .

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.146.6.2; previous revision: 1.146.6.1

Comment 11

[Julien Pierre]

As coded, CERT_FindCRLNumberExten has a problem with the memory management.
The QuickDER decoder returns memory with a pointer that may point into memory
from the input buffer. Thus, you cannot decode directly into "value". You must
make a copy of the item into the target arena, using SECITEM_DupItem .

Also, the PORT_Free call should be removed, since we are using arenas.

Comment 12

[Julien Pierre]

Looking over this more closely - the PORT_Free call is correct and should stay,
but my first comment still stands - you need to make a copy of the item before
you return it.

Comment 13

[Hanfei Yu]

Attachment: duplicate extension SECItem data for decoding

Use SECITEM_ArenaDupItem() to duplicate extension data for decoding.

Comment 14

[Julien Pierre]

This didn't make 3.11 . Retargetting for 3.12 .

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Reassigning.  HanFei isn't working on NSS any more.

Comment 16

[Alexei Volkov]

Will be integrated to the trunk as a part of patch of bug 358785

Comment 17

[Julien Pierre]

This was fixed with the checkin to bug 358785 .