The default bug view has changed. See this FAQ.

CRL number should be a big integer, not ulong

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Hanfei Yu, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> Julien
Assignee: wtchang → julien.pierre.bugs

Comment 2

12 years ago
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.
Status: NEW → ASSIGNED
OS: Solaris → All
Hardware: Sun → All

Updated

12 years ago
Priority: -- → P1
Target Milestone: --- → 3.11
Version: unspecified → 3.10
(Reporter)

Comment 3

12 years ago
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

12 years ago
Hanfei,

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

Thanks.
(Reporter)

Comment 5

12 years ago
Created attachment 178489 [details] [diff] [review]
diff with CRL number big int fix

Comment 6

12 years ago
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 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);
>+
Attachment #178489 - Attachment is patch: true

Comment 8

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

QA Contact: bishakhabanerjee → jason.m.reid

Comment 9

12 years ago
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
Attachment #178489 - Attachment is obsolete: true

Comment 10

12 years ago
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

12 years ago
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.
Assignee: julien.pierre.bugs → hanfei.yu
Status: ASSIGNED → NEW

Comment 12

12 years ago
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.
(Reporter)

Comment 13

12 years ago
Created attachment 194549 [details] [diff] [review]
duplicate extension SECItem data for decoding

Use SECITEM_ArenaDupItem() to duplicate extension data for decoding.

Comment 14

12 years ago
This didn't make 3.11 . Retargetting for 3.12 .

Updated

12 years ago
Target Milestone: 3.11 → 3.12
Reassigning.  HanFei isn't working on NSS any more.
Assignee: hanfei.yu → julien.pierre.bugs
QA Contact: jason.m.reid → libraries
(Assignee)

Updated

10 years ago
Whiteboard: PKIX
(Assignee)

Comment 16

10 years ago
Will be integrated to the trunk as a part of patch of bug 358785
Status: NEW → ASSIGNED
Depends on: 358785
(Assignee)

Updated

10 years ago
Assignee: julien.pierre.boogz → alexei.volkov.bugs
Status: ASSIGNED → NEW

Comment 17

10 years ago
This was fixed with the checkin to bug 358785 .
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.