287052 - Function to get CRL Entry reason code has incorrect prototype and implementation

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

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

 There are two problems with the function CERT_FindCRLReasonExten():
 1) parameter passed was CERTCrl *crl. Since ReasonCode is in CRLEntry
 extension, and there could be multiple CRLEntries associated with one CRL,
 the parameter should be CERTCrlEntry. Suggest to rename it as
 CERT_FindCRLEntryReasonExten().
 2) ReasonCode is an enumerate type, the decoder function should be fixed
 accordingly.


Reproducible: Always



Expected Results:  
As suggested in Details block.

Comment 1

[Wan-Teh Chang]

Nelson, please reassign to the appropriate engineer
on your team.

I found that CERT_FindCRLReasonExten is dead
code.  It is not exported, and not used by NSS
internally.  So the correctness of this function
is unknown.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Julien is Mr. CRL  :)

We should either expunge this dead code, or consider this to be an RFE,
asking that it be made public.  

HanFei, are you asking that this function be made public?  
Do you want to use it?

Comment 3

[Hanfei Yu]

Yes, I would think this function need to be public. (Am I the the first person
trying to get CRL reason code ? :-) )

I had the fix local in our NSS source, if you need a copy of it, let me know.

Comment 4

[Julien Pierre]

Han-Fei,

You are right, the CRL reason code extension cannot appear in CRL extensions, 
only in CRL entry extensions .

For a minute I thought this might be legal for distribution points that only
list certain reason codes, but RFC3280 says in fact that's done by setting the
ReasonFlags in an issuingDistributionPoint of onlySomeReasons .

This function should be re-implemented as CERT_FindCRLEntryReasonExten.

It would use the internal function cert_FindExtension to locate the extension
within the CertCrlEntryStr 's extensions field .

Comment 5

[Julien Pierre]

Hanfei,

BTW, yes, I think you are the first NSS user who wanted to look at this
extension, since there is no way to get at it with the current public API. Even
NSS itself never checks it currently, since it only supports full CRLs, and from
that perspective, all reason codes lead to the same conclusion - that the cert
is revoked.

This is of course no longer true in the case of delta CRLs and distribution
points - you really need to have it. Do you want to take on this bug and create
this new function, since it is needed for libpkix NSS integration ?

Comment 6

[Julien Pierre]

Targetting this bug and all other libpkix related bugs to 3.11 .

Comment 7

[Hanfei Yu]

Julien:

  Since I don't have the setup here for NSS pushback and testing, can you 
  integrate it in your setup? This fix had been tested using NIST CRL files.

  Please note that I created another function named CERT_FindCRLEntryReasonExten
  just to call CERT_FindCRLReasonExten. Because I wasn't sure anyone had been
  using the original one.

Hanfei

******************** in crlv2.c:

SECStatus CERT_FindCRLReasonExten (CERTCrlEntry *crlEntry, int *value)
{
    SECStatus rv;
    SECItem tmpItem;

    tmpItem.data = NULL;
    tmpItem.len = 0;

    rv = CERT_FindExtensionWithTemplate
	(crlEntry->extensions, SEC_OID_X509_REASON_CODE, 
	 SEC_EnumeratedTemplate, &tmpItem);

    if ( rv != SECSuccess )
	return (rv);

    *value = DER_GetInteger(&tmpItem);
    SECITEM_FreeItem(&tmpItem, PR_FALSE);

    return (rv);
}

SECStatus CERT_FindCRLEntryReasonExten (CERTCrlEntry *crlEntry, int *value)
{
    return (CERT_FindCRLReasonExten(crlEntry, value));
}

Comment 8

[Julien Pierre]

Hanfei,

It would be very useful for you to setup an NSS build environment.

For now, please fix your function - you can get rid of the old one altogether
since it's not in use - and generate a diff with cvs -q diff -u10 . Then use the
"create new attachment" link at the top of this bug to attach this file .

As to the pushback, I targeted this bug for NSS 3.11 . We can't add new
functions to 3.10 at this time. We haven't branched for 3.10 yet. It's probably
going to be a few weeks before this fix gets put back.

Comment 9

[Hanfei Yu]

Attachment: CVS diff for fixing bug 287052

Comment 10

[Wan-Teh Chang]

Comment on attachment 178485 [details]
CVS diff for fixing bug 287052

This function needs to be declared in a public
header file (certdb.h?) and exported in lib/nss/nss.def.

Comment 11

[Julien Pierre]

Hanfei,

The patch you attached depends on the function CERT_FindExtensionWithTemplate,
which is not in NSS yet. Please attach a patch that includes all the dependencies.

Thanks.

Comment 12

[Hanfei Yu]

Attachment: Fixed incorrect behavior for CERT_FindCRLEntryReasonExten()

CERT_FindCRLEntryReasonExten is not working correctly. Modify changes at
cert.h, certxutl.c and crlv2.c .

Comment 13

[Julien Pierre]

Comment on attachment 189447 [details] [diff] [review]
Fixed incorrect behavior for CERT_FindCRLEntryReasonExten()

Unfortunately, this patch does not apply to NSS_LIBPKIX_BRANCH due to other
changes .

Comment 14

[Julien Pierre]

Attachment: fix that applies to NSS_LIBPKIX_BRANCH

Also declare the missing CERT_FindExtensionWithTemplate function in cert.h .
Remove the bogus and unused CERT_FindCRLReasonExten function from crl.c .

Comment 15

[Julien Pierre]

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.4; previous revision: 1.53.10.3
done
Checking in certdb/certxutl.c;
/cvsroot/mozilla/security/nss/lib/certdb/certxutl.c,v  <--  certxutl.c
new revision: 1.5.10.1; previous revision: 1.5
done
Checking in certhigh/crlv2.c;
/cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v  <--  crlv2.c
new revision: 1.3.10.2; previous revision: 1.3.10.1
done

Comment 16

[Hanfei Yu]

Attachment: mismatch header with function names

Pleaase apply the attachment diff file, which includes:

1) correct function name for CERT_FindCRLReasonExten in cert.h
2) include ReleaseDPCache in certi.h (was a static function)
3) add CERT_FindCRLReasonExten and CERT_FindCRLNumberExten in nss.def

Thanks.

Hanfei

Comment 17

[Julien Pierre]

Hanfei,

Re: your patch from 7/20, we really do want to change the function name from
CERT_FindCRLReasonExten to CERT_FindCRLEntryReasonExten . The function was not
previously exported or used internally, so it is OK to change both its prototype
and name. The new name is more appropriate, since a CRL doesn't have a reason
extension, only a CRL entry does.

Comment 18

[Julien Pierre]

Attachment: rename CERT_FindCRLReasonExten to CERT_FindCRLEntryReasonExten

As checked in to NSS_LIBPKIX_BRANCH .

Checking in crlv2.c;
/cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v  <--  crlv2.c
new revision: 1.3.10.3; previous revision: 1.3.10.2

Comment 19

[Hanfei Yu]

Attachment: Use the name CERT_FindCRLEntryReasonExten

The patch difference is created for using CERT_FindCRLEntryReasonExten call
instead of CERT_FindCRLReasonExten.

Comment 20

[Julien Pierre]

There are several problems with the function CERT_FindExtensionWithTemplate :
1. When you call SEC_QuickDERDecodeItem, you are passing a pointer to tmpItem as
the second argument, which is the destination .

The destination is defined as a void* to QuickDER, because its type actually
depends on the content of the template. The way you have written your code, only
very simple templates, representing primitive ASN.1 types that fit in a single
SECItem, can be passed in to CERT_FindExtensionWithTemplate . Anything bigger
will cause a stack overflow, which could cause remote code to be executed . This
function needs to be fixed so that this is not possible. If you are always
calling CERT_FindExtensionWithTemplate with templates representing primitive
types, then I think this function doesn't really need to exist. You should just
integrate the call to SEC_QuickDERDecodeItem into the calling function .

2. if you find a way to fix the design of the function and keep it around, the
following would need to be resolved :

- you should agree on a common way to initialize both tmpItem and wrapperItem
- you should use one of the SECITEM_ functions to copy items; there is no need
to write your own copy code

Comment 21

[Julien Pierre]

Another problem is with CERT_FindCRLReasonExten . It is written to return an int.
It would be much better to return an enum type of the various reason codes.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190398 [details] [diff] [review]
Use the name CERT_FindCRLEntryReasonExten

Let's not add any functions to NSS whose names end with "Ex" or "Exten".  
Exten tells me nothing about what changed, and is easily confused with
Extension (as in CertExtension).
If the name is unique without any such suffix, just omit the suffix. 
Otherwise, please add a meaningful suffix.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #21)
> Another problem is with CERT_FindCRLReasonExten . It is written to return an int.
> It would be much better to return an enum type of the various reason codes.

Julien, it returns a SECStatus.  I think you meant that it outputs an int,
and you'd prefer that it output a new reason code (enumerated type).  Right?

Comment 24

[Julien Pierre]

Nelson,

Yes, that's what I meant.

Comment 25

[Hanfei Yu]

Attachment: combine calls to avoid memory storage issue and provides reason code as enum type.

This patch includes two changes. One is to consolidate the call
CERT_FindExtensionWithTemplate() into CERT_FindCRLReasonExten(). The other is
for API CERT_FindCRLReasonExten() to return CRL entry reason code as enum in
its argument list.

Please note, since pkix_pl_crlentry.c (which I believe is not in the main line
yet) is affected by the API change, its diff is also in the patch file.

Comment 26

[Julien Pierre]

This didn't make 3.11 . Retargetting to 3.12 .

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 28

[Julien Pierre]

This was fixed as part of the checkin for bug 358785 .