Closed Bug 174188 Opened 22 years ago Closed 22 years ago

pp crash printing certificate request generated by certutil

Categories

(NSS :: Tools, defect, P1)

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: wtc)

Details

Attachments

(2 files)

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.
This patch stops the crashes in pp, and makes pp print the request signature.
But does it fix the bug?  or merely mask it?
One more comment.  The pp command that crashed was

pp -t certificate-request -i JAR/req -a
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.
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.  
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.
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 ??)
Status: NEW → ASSIGNED
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.

Summary: pp crash implies possible DER en/decoder error (?) → SEC_ASN1DecodeItem bug revealed by pp crash
I'm marking this bug fixed, because pp no longer crashes and displays
correct results.  I will file a separate bug about SEC_ASN1DecodeItem.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Priority: -- → P1
Resolution: --- → FIXED
Summary: SEC_ASN1DecodeItem bug revealed by pp crash → pp crash printing certificate request generated by certutil
Target Milestone: --- → 3.7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: