Last Comment Bug 136625 - nsslowcert_DecodeDERCertificate failure causes pk11_searchCertsAndTrust to deference a null 'cert' pointer
: nsslowcert_DecodeDERCertificate failure causes pk11_searchCertsAndTrust to de...
: crash
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4
: All All
P1 normal (vote)
: 3.4.2
Assigned To: Robert Relyea
: Bishakha Banerjee
Depends on:
  Show dependency treegraph
Reported: 2002-04-10 08:05 PDT by Wan-Teh Chang
Modified: 2002-05-17 16:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch to prevent pk11_cert_collect from storing NULL in certData.certs (468 bytes, patch)
2002-04-10 08:12 PDT, Wan-Teh Chang
wtc: review+
rjesup: approval+
Details | Diff | Splinter Review

Description User image Wan-Teh Chang 2002-04-10 08:05:07 PDT
This crash was reported to us recently.

=>[1] pk11_mkHandle(slot = 0xbd128, dbKey = 0x60, class = 1879048192U), line 
2495 in "pkcs11u.c"
  [2] pk11_searchCertsAndTrust(slot = 0xbd128, derCert = 0xfe1615e0, name = 
0xfe1615c8, derSubject = 0xfe1615d4, issuerSN = 0xfe161580, email = 0xfe1615bc, 
classFlags = 1U, handles = 0x340870, pTemplate = 0xfe1617c8, ulCount = 3), line 
3800 in "pkcs11.c"
  [3] pk11_searchTokenList(slot = 0xbd128, search = 0x340870, pTemplate = 
0xfe1617c8, ulCount = 3, tokenOnly = 0xfe161668, isLoggedIn = 0), line 4041 in 
  [4] NSC_FindObjectsInit(hSession = 16777217U, pTemplate = 0xfe1617c8, ulCount 
= 3U), line 4098 in "pkcs11.c"
  [5] traverse_objects_by_template(tok = 0x103648, sessionOpt = (nil), 
obj_template = 0xfe1617c8, otsize = 3U, callback = 0xfe6a3da0 = 
&``devobject.c`retrieve_cert(struct NSSTokenStr *t, struct 
nssSessionStr *session, CK_OBJECT_HANDLE h, void *arg), arg = 0xfe161860), line 
223 in "devobject.c"
  [6] nssToken_TraverseCertificatesBySubject(token = 0x103648, sessionOpt = 
(nil), subject = 0xfe161910, search = 0xfe161860), line 643 in "devobject.c"
  [7] NSSTrustDomain_FindBestCertificateBySubject(td = 0x1034b8, subject = 
0xfe161910, timeOpt = (nil), usage = 0xfe161904, policiesOpt = (nil)), line 638 
in "trustdomain.c"
  [8] CERT_FindCertByName(handle = 0x1034b8, name = 0x1342c4), line 337 in 

(Note that this stack is similar to the stack in

This crash is caused by defererencing a null pointer.
Here is what I think causes the crash:
  In pk11_searchCertsAndTrust(), the local variable
  'cert' is set to NULL because some other function
  failed earlier.  Then we pass &cert->certKey as the
  'dbKey' argument to pk11_mkHandle.

(If 'cert' is NULL, &cert->certKey is 0x60, which is
the value of 'dbKey' in the stack trace.)

I've determined that this is caused by
nsslowcert_TraversePermCertsForSubject, or
nsslowcert_TraversePermCerts passing a null 'cert'
argument to the callback function pk11_cert_collect.
pk11_cert_collect would store the null 'cert' in the
certData.certs array.

It would be easy to modify pk11_cert_collect so that
it does not store a null 'cert' in the certData.certs
array, but we might need to find out why the
nsslowcert_TraversePermCerts* functions get a null
'cert' in the first place.  Looking at the code, I
found it's because nsslowcert_FindCertByKey(pcertdb.c:2874)
or DecodeACert (pcertdb.c:3857 and 4111) or eventually
nsslowcert_DecodeDERCertificate (pcertdb.c:3799) failed.
Comment 1 User image Wan-Teh Chang 2002-04-10 08:12:13 PDT
Created attachment 78551 [details] [diff] [review]
Patch to prevent pk11_cert_collect from storing NULL in certData.certs

This patch may prevent the crash but I am wondering if
we should fix the underlying problem, that is, the
nsslowcert_DecodeDERCertificate failure.
Comment 2 User image Robert Relyea 2002-04-10 09:31:56 PDT
DER_DecodeCert could fail if the DER cert data isn't valid. This could occur
because the database had been corrupted at some point, or somehow we've injected
an invalid certificate into the database.

Wan-Teh's fix seems resonable to me. We may want to review the code and see
where else a corrupted certificate may cause us problems.

We probably should look at the cert and make sure it's a corruption problem and
not a valid cert that isn't being parsed by our templates.

Comment 3 User image Wan-Teh Chang 2002-04-10 11:10:30 PDT
Comment on attachment 78551 [details] [diff] [review]
Patch to prevent pk11_cert_collect from storing NULL in certData.certs

Just wanted to note that Bob has reviewed this patch
and I've checked it in on the trunk.

I specifically asked Bob whether pk11_cert_collect
should return SECSuccess if 'cert' is NULL.  I
explained that if pk11_cert_collect returns SECFailure,
nsslowcert_TraversePermCertsForSubject will abort
the cert traversal.  Bob said returning SECSuccess is
Comment 4 User image Kai Engert (:kaie) 2002-04-10 14:14:15 PDT
We should land this on the branch. Nominating adt1.0.0. I will send mail to drivers.
Comment 5 User image Randell Jesup [:jesup] 2002-04-10 15:00:38 PDT
Comment on attachment 78551 [details] [diff] [review]
Patch to prevent pk11_cert_collect from storing NULL in certData.certs
Comment 6 User image Jaime Rodriguez, Jr. 2002-04-11 11:45:55 PDT
adt1.0.0+ (on ADTs behalf) checkin to 1.0. Pls check this into the trunk and 1.0
Comment 7 User image Wan-Teh Chang 2002-04-11 15:21:08 PDT
I checked in the fix on the MOZILLA_1_0_0_BRANCH.
Comment 8 User image Kai Engert (:kaie) 2002-04-12 06:20:47 PDT
Thanks, Wan-Teh. Should we check this in to NSS_CLIENT_TAG, too?
Comment 9 User image Jaime Rodriguez, Jr. 2002-04-12 17:23:13 PDT
When checking a fix into 1.0, please add fixed1.0.0. Thanks!
Comment 10 User image Jaime Rodriguez, Jr. 2002-04-15 08:02:35 PDT
Removing adt1.0.0+, as this is fixed on 1.0 branch.
Comment 11 User image Wan-Teh Chang 2002-04-25 16:13:23 PDT
Changed the QA contact to Bishakha.
Comment 12 User image Wan-Teh Chang 2002-04-26 10:47:35 PDT
I checked in the fix on the NSS_3_4_BRANCH.
Comment 13 User image Wan-Teh Chang 2002-05-14 15:31:22 PDT
Set target milestone 3.4.2.
Comment 14 User image Wan-Teh Chang 2002-05-17 16:00:38 PDT
Marked the bug fixed.

Note You need to log in before you can comment on or make changes to this bug.