Closed Bug 414003 Opened 16 years ago Closed 15 years ago

Crash [@ CERT_DecodeCertPackage] sometimes with this testcase, that imports a certificate


(NSS :: Libraries, defect, P2)

Windows XP


(Not tracked)



(Reporter: martijn.martijn, Assigned: mayhemer)



(Keywords: crash, testcase)

Crash Data


(3 files, 1 obsolete file)

Attached file certificate
I sometimes get a crash when importing this certificate with a testcase, that continuously imports that certificate.
I'll attach that testcase, after this file has been attached.
0  	CERT_DecodeCertPackage  	 mozilla/security/nss/lib/pkcs7/certread.c:474
1 	nsNSSCertificateDB::getCertsFromPackage(PLArenaPool*, unsigned char*, unsigned int) 	mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:266
2 	nsNSSCertificateDB::ImportCertificates(unsigned char*, unsigned int, unsigned int, nsIInterfaceRequestor*) 	mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:477
3 	nsNSSCertificateDB::ImportCertsFromFile(nsISupports*, nsILocalFile*, unsigned int) 	mozilla/security/manager/ssl/src/nsNSSCertificateDB.cpp:1146
4 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
5 	JS_SuspendRequest 	
6 	@0x2

It also seems to crash on branch.

The crash can occur within 10 seconds, but it also can take 2 minutes, it seems  to take a random amount of time.
Attached file testcase
To reproduce:
- Download the testcase and certificate to your computer
- Open the testcase, a file-picker pops up, choose the certificate file
- Wait for the crash to happen.
Attached patch Fix (obsolete) — Splinter Review
Crash was caused by incorrectly formatted ASCII cert, -----END CERITIFICATE----- was broken, and because cl counter of content left to parse was not moved forward with cp pointer after -----BEGIN CERTIFICATE----- was found we were trying to parse behind the data of the cert.

I changed conditions to (cl > sizeof(NS_CERT_TRAILER)-1) because sizeof returns also the trailing zero. (cl > sizeof(NS_CERT_TRAILER)) expect two additional chars behind. For mac/linux files where is not any CR char this would not pass, needlessly.
Assignee: kengert → honzab
Attachment #315075 - Flags: review?(kengert)
The patch you attached is in NSS code, so I'm changing the component of this bug and assigning to default owners.
Assignee: honzab → nobody
Component: Security: PSM → Libraries
Product: Core → NSS
QA Contact: psm → libraries
Version: Trunk → unspecified
Comment on attachment 315075 [details] [diff] [review]

Bob should review this code.
Attachment #315075 - Flags: review?(kengert) → review?(rrelyea)

CC'ing Nelson and Wan-Teh who have touched this code and hopefully can review it.
Assignee: nobody → honzab
Thanks to Honza Bambas (:mayhemer) for identifying the cause of this bug.
I've written an alternative patch that fixes ALL the incorrect usages of 
sizeof, and also eliminates duplicate copies of constant strings.
Attachment #315142 - Flags: superreview?(alexei.volkov.bugs)
Attachment #315142 - Flags: review?(kengert)
Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

Attachment #315142 - Flags: superreview?(alexei.volkov.bugs) → superreview+
Bob, Do we want to get this into 3.12 or wait for 3.12.1 ?
Priority: -- → P2
Target Milestone: --- → 3.12
Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof


r+ rrelyea
Attachment #315142 - Flags: review?(kengert) → review+
Comment on attachment 315075 [details] [diff] [review]

obsoleted by nelson's patch. Good investigation work Honza

Attachment #315075 - Attachment is obsolete: true
Attachment #315075 - Flags: review?(rrelyea)
Keywords: checkin-needed
lib/pkcs7/certread.c; new revision: 1.12; previous revision: 1.11
Closed: 15 years ago
Resolution: --- → FIXED
I'm not planning to put this into 3.12.0
Somebody holler if that's a problem.
Target Milestone: 3.12 → 3.12.1
Depends on: 453227
Crash Signature: [@ CERT_DecodeCertPackage]
You need to log in before you can comment on or make changes to this bug.