Last Comment Bug 287061 - CRL number should be a big integer, not ulong
: CRL number should be a big integer, not ulong
Status: RESOLVED FIXED
PKIX
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.10
: All All
: P1 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
Mentors:
Depends on: 358785
Blocks:
  Show dependency treegraph
 
Reported: 2005-03-21 07:00 PST by Hanfei Yu
Modified: 2007-05-31 19:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
diff with CRL number big int fix (2.02 KB, patch)
2005-03-24 09:38 PST, Hanfei Yu
no flags Details | Diff | Review
patch including suggestions (2.62 KB, patch)
2005-07-15 10:40 PDT, Julien Pierre
no flags Details | Diff | Review
just export CERT_FindCRLNumberExten from nss.def (995 bytes, patch)
2005-07-22 14:52 PDT, Julien Pierre
no flags Details | Diff | Review
duplicate extension SECItem data for decoding (1.32 KB, patch)
2005-09-01 10:17 PDT, Hanfei Yu
no flags Details | Diff | Review

Description Hanfei Yu 2005-03-21 07:00:50 PST
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) 2005-03-21 11:26:52 PST
-> Julien
Comment 2 Julien Pierre 2005-03-21 17:03:21 PST
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 2005-03-22 05:01:59 PST
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 2005-03-23 16:02:47 PST
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 2005-03-24 09:38:01 PST
Created attachment 178489 [details] [diff] [review]
diff with CRL number big int fix
Comment 6 Wan-Teh Chang 2005-03-24 09:59:28 PST
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) 2005-03-24 12:57:52 PST
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 2005-03-24 13:44:17 PST
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 2005-07-15 10:40:20 PDT
Created attachment 189444 [details] [diff] [review]
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 2005-07-22 14:52:04 PDT
Created attachment 190181 [details] [diff] [review]
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 2005-08-24 18:55:35 PDT
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 2005-08-24 19:23:38 PDT
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 2005-09-01 10:17:51 PDT
Created attachment 194549 [details] [diff] [review]
duplicate extension SECItem data for decoding

Use SECITEM_ArenaDupItem() to duplicate extension data for decoding.
Comment 14 Julien Pierre 2005-11-15 16:11:50 PST
This didn't make 3.11 . Retargetting for 3.12 .
Comment 15 Nelson Bolyard (seldom reads bugmail) 2006-03-14 14:33:24 PST
Reassigning.  HanFei isn't working on NSS any more.
Comment 16 Alexei Volkov 2007-05-03 11:14:43 PDT
Will be integrated to the trunk as a part of patch of bug 358785
Comment 17 Julien Pierre 2007-05-31 19:59:53 PDT
This was fixed with the checkin to bug 358785 .

Note You need to log in before you can comment on or make changes to this bug.