174188 - pp crash printing certificate request generated by certutil

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

I created a certificate request using certutil (from NSS 3.3) 
with a command like this:
certutil -d JAR -R -o JAR/req -a -s "CN=object signing cert, O=xxx.xxx"

Then I tried to print out the certificate request using NSS's "pretty print"
program pp (from NSS 3.6).  pp printed the version number, the requested 
subject name, and the requested public key, and then crashed when it tried
to print out the requests set of attributes (which should have been an empty 
set).  This was repeatable.  

The crash was in the function SECU_PrintCertificateRequest.  That function
calls SEC_ASN1DecodeItem to decode the request, which succeeds.  The result
is a CERTCertificateRequest struct with 4 members, the fourth of which,
cr->attributes, is NULL.  The code in SECU_PrintCertificateRequest clearly
doesn't ever expect to see it be null.  It would be easy to fix this code
to check for null pointers and avoid the crash.  But the question is:
does this bug reveal another bug, in the DER decoder?  
And, is the request perhaps encoded incorrectly?  

When encoding the cert request for a NULL list of attributes, as certutil 
does, CERT_CreateCertificateRequest allocates a small array of pointers to 
SECItems, and cr->attributes points to that array.  The first SECItem pointer 
in the array is then set to NULL. Apparently, the author of 
SECU_PrintCertificateRequest expected the decoder to return something similar.  
But it doesn't.  Should it?  

I suspect there are more bugs here than the simple lack of NULL pointer 
checking in SECU_PrintCertificateRequest and secu_PrintAny.  
I'm cc'ing our current DER encoder/decoder expert for his opinion.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Discover null pointers in secutil print routines

This patch stops the crashes in pp, and makes pp print the request signature.
But does it fix the bug?  or merely mask it?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

One more comment.  The pp command that crashed was

pp -t certificate-request -i JAR/req -a

Comment 3

[Julien Pierre]

Nelson,
can you try this with NSS 3.6 or the tip ?
This should use the new decoder which might have a different behavior. I'll
answer your other questions shortly.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Julien, the crash occurs using the NSS 3.6 version of pp and the libraries.
The cert request was generated wtih NSS 3.3, but the crash occurs when 
trying to pretty print it with NSS 3.6.  

I can regenerate the request using NSS 3.6, if you think that would be 
helpful.  

Comment 5

[Julien Pierre]

Nelson,

I think it's worth a try to do the cert request again with 3.6 , though most
likely it won't change the outcome.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: a cert request generated by NSS 3.6 (actually tip)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Patch checked in.  pp doesn't crash any more.  

The remaining questions are:

a) is the cert request correctly encoded, and 

b) is the decoder behaving properly when it returns NULL in cr->attributes?
Or should cr->attributes point to a NULL pointer?  (or ??)

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I now believe there IS a bug in SEC_ASN1DecodeItem revealed by this crash.
At Julien's suggestion, I tried using SEC_QuickDERDecodeItem instead of 
SEC_ASN1DecodeItem in SECU_PrintCertificateRequest(), and the result was
different.  The cr->attributes pointer was not NULL then, and it correctly
pointed to a NULL pointer.  

I will change SECU_PrintCertificateRequest to use SEC_QuickDERDecodeItem 
permanently, but someone needs to fix SEC_ASN1DecodeItem.  

I will change the subject of this bug to show that there IS a bug in 
SEC_ASN1DecodeItem.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I'm marking this bug fixed, because pp no longer crashes and displays
correct results.  I will file a separate bug about SEC_ASN1DecodeItem.