222180 - pp is broken for CRLs

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

Bishakha reported that pp currently dumps core on CRLs.
Earlier last month when I added support for GeneralizedTime, I modified the CRL
template CERT_CrlTemplate which is exported from nss3.dll, and used by pp.

What breaks in this case specifically is the nextUpdate field.
It used to be declared as
SEC_ASN1_UTC_TIME | SEC_ASN1_OPTIONAL .
With the GeneralizedTime changes, I had to define a CHOICE subtemplate
In the main template, nextUpdate is now declared as
SEC_ASN1_INLINE | SEC_ASN1_OPTIONAL

The above combination only works with the QuickDER decoder, but
SEC_ASN1DecodeItem has a bug and is broken in that regard. It just asserts.
pp was calling SEC_ASN1DecodeITem .
The short-term fix in this case is simply to replace the call to
SEC_ASN1DecodeItem with SEC_QuickDERDecodeItem .

Note that other existing applications may be calling SEC_ASN1DecodeItem against
CERT_CrlTemplate, and they will break, so there is a binary compatibility issue.
If we want to fix that, we need to do it by fixing SEC_ASN1DecodeItem to handle
this combination of flags ...

Comment 1

[Julien Pierre]

Attachment: switch to SEC_QuickDERDecodeItem for CRLs in tools

Besides fixing this bug, this change will also provide a huge performance boost
when printing CRLs ...

Comment 2

[Julien Pierre]

Taking bug.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Um, but aren't CRLs permitted to be BER encoded?

Comment 4

[Wan-Teh Chang]

Comment on attachment 133302 [details] [diff] [review]
switch to SEC_QuickDERDecodeItem for CRLs in tools

r=wtc.

By the way, we should delete the duplicate definition
of SECU_PrintCrl in cmd/signver/pk7print.c because that
function is already defined in cmd/lib/secutil.c.

Comment 5

[Julien Pierre]

Nelson,

To the best of my knowledge, CRLs are only DER encoded. At least, I did the work
last year on the Quick DER decoder specifically to improve CRL decoding performance.
Do you have a reference that says BER is allowed ?

Comment 6

[Julien Pierre]

cvs commit: Examining .
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.51; previous revision: 1.50
done

I have also removed the redundant code in pk7print.c .

Checking in pk7print.c;
/cvsroot/mozilla/security/nss/cmd/signver/pk7print.c,v  <--  pk7print.c
new revision: 1.5; previous revision: 1.4
done