339921 - Coverity 410, NULL ptr crash in certdb_SaveSingleProfile

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

In function certdb_SaveSingleProfile in file nss/lib/certdb/stanpcertdb.c
there are lots of ways to reach this statement:

808 	stanProfile->profileTime = nssItem_Create(arena, 
809 	                                          NULL,
810 	                                          profileTime->len,
811 	                                          profileTime->data);

with profileTime being NULL, and hence crashing here.
It appears to me that the very next statement has an equal probability
of crashing in the same way, due to NULL emailProfile.

812  	stanProfile->profileData = nssItem_Create(arena, 
813  	                                          NULL,
814  	                                          emailProfile->len,
815  	                                          emailProfile->data);

Figuring out all the right checks, to eliminate all the crashing paths,
without breaking NSS code that depends on this function, will be a real
head scratcher, I think.

Comment 1

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1.

Not sure if this is the correct fix.

Comment 3

[Robert Relyea]

Comment on attachment 242781 [details] [diff] [review]
Patch v1

r-

So in this code it is permissible to write NULL email and profile times.

It's also permissible to write a NULL oldProfileTime.

The question because what is the correct semantic for if the crypto context exists. My initial though is the crypto context profile should be destroyed and removed from the list, but more investigation is required.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242781 [details] [diff] [review]
Patch v1

cancelling SR request, since the patch got r-.

Comment 5

[Julien Pierre]

3.11.7 is done. Retargeting to 3.11.8 .

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I found this when doing code review on changes to p7decode.c.

It seems like this crash will occur in cases where certificate chain verification fails inside of SEC_PKCS7VerifySignature/SEC_PKCS7VerifyDetachedSignature when verifying for an email cert usage, since we still try to write an S/MIME profile, passing in a NULL emailProfile (which will cause a NULL profileTime, I believe).

In bug 808224, I will create aa alternative API for verifying PKCS7 signatures in bug that will not deail with S/MIME profile management stuff at all. Then it will be the application's responsibility to save the S/MIME profile using an explicit CERT_SaveSMIMEProfile call.

I recommend that Thunderbird switch to use the API I add in bug 808224. I'm not sure how to change SEC_PKCS7VerifySignature/SEC_PKCS7VerifyDetachedSignature and the code it calls to deal with S/MIME profiles to be safe and still have the same semantics. And, also, it is unclear to me why we try to save an S/MIME profile at all if signature and/or certificate chain validation fails in these functions.