The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.12

Status

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

People

(Reporter: Hanfei Yu, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(4 attachments, 3 obsolete attachments)

(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

 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

12 years ago
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.
Assignee: wtchang → nelson
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?
Assignee: nelson → julien.pierre.bugs
(Reporter)

Comment 3

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

Comment 4

12 years ago
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 .
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

12 years ago
OS: Solaris → All
Hardware: Sun → All
Version: unspecified → 3.10
(Assignee)

Comment 5

12 years ago
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 ?
(Assignee)

Comment 6

12 years ago
Targetting this bug and all other libpkix related bugs to 3.11 .
Priority: -- → P1
Target Milestone: --- → 3.11
(Reporter)

Comment 7

12 years ago
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));
}
(Assignee)

Comment 8

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

Comment 9

12 years ago
Created attachment 178485 [details]
CVS diff for fixing bug 287052

Comment 10

12 years ago
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.
QA Contact: bishakhabanerjee → jason.m.reid
(Assignee)

Comment 11

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

Comment 12

12 years ago
Created attachment 189447 [details] [diff] [review]
Fixed incorrect behavior for CERT_FindCRLEntryReasonExten()

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

Updated

12 years ago
Attachment #178485 - Attachment is obsolete: true
Attachment #189447 - Flags: review?(julien.pierre.bugs)
(Assignee)

Comment 13

12 years ago
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 .
Attachment #189447 - Flags: review?(julien.pierre.bugs) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 189456 [details] [diff] [review]
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 .
Attachment #189447 - Attachment is obsolete: true
(Assignee)

Comment 15

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

Comment 16

12 years ago
Created attachment 189888 [details] [diff] [review]
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

Updated

12 years ago
Attachment #189888 - Attachment is patch: true
Attachment #189888 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 17

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

Comment 18

12 years ago
Created attachment 190182 [details] [diff] [review]
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
Attachment #189888 - Attachment is obsolete: true
(Reporter)

Comment 19

12 years ago
Created attachment 190398 [details] [diff] [review]
Use the name CERT_FindCRLEntryReasonExten

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

Comment 20

12 years ago
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
Summary: Function to get CRL Entry reason code is incorrect → Function to get CRL Entry reason code has incorrect prototype and implementation
(Assignee)

Comment 21

12 years ago
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.
Assignee: julien.pierre.bugs → hanfei.yu
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.
Attachment #190398 - Attachment is patch: true
(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?

(Assignee)

Comment 24

12 years ago
Nelson,

Yes, that's what I meant.
(Reporter)

Comment 25

12 years ago
Created attachment 194536 [details] [diff] [review]
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.
(Assignee)

Comment 26

12 years ago
This didn't make 3.11 . Retargetting to 3.12 .
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

Updated

10 years ago
Whiteboard: PKIX

Updated

10 years ago
Depends on: 358785
Priority: P1 → P2
(Assignee)

Comment 28

10 years ago
This was fixed as part of the checkin for 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.