Last Comment Bug 414003 - Crash [@ CERT_DecodeCertPackage] sometimes with this testcase, that imports a certificate
: Crash [@ CERT_DecodeCertPackage] sometimes with this testcase, that imports a...
Status: RESOLVED FIXED
: crash, testcase
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: x86 Windows XP
: P2 critical (vote)
: 3.12.1
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 453227
Blocks: 413380
  Show dependency treegraph
 
Reported: 2008-01-25 09:15 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-09 14:58 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
certificate (2.66 KB, application/octet-stream)
2008-01-25 09:15 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (1.57 KB, text/html)
2008-01-25 09:17 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Fix (1.42 KB, patch)
2008-04-11 03:50 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
Fix all the incorrect usages of sizeof (1.95 KB, patch)
2008-04-11 11:13 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
alvolkov.bgs: superreview+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-01-25 09:15:33 PST
Created attachment 299219 [details]
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.

http://crash-stats.mozilla.com/report/index/9e0851ee-cb4a-11dc-b5ff-001a4bd43ed6
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.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-01-25 09:17:06 PST
Created attachment 299220 [details]
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.
Comment 2 Honza Bambas (:mayhemer) 2008-04-11 03:50:13 PDT
Created attachment 315075 [details] [diff] [review]
Fix

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.
Comment 3 Kai Engert (:kaie) 2008-04-11 03:56:23 PDT
The patch you attached is in NSS code, so I'm changing the component of this bug and assigning to default owners.
Comment 4 Kai Engert (:kaie) 2008-04-11 03:57:11 PDT
Comment on attachment 315075 [details] [diff] [review]
Fix

Bob should review this code.
Comment 5 Kai Engert (:kaie) 2008-04-11 04:01:44 PDT
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/nss/lib/pkcs7/certread.c&rev=1.11

CC'ing Nelson and Wan-Teh who have touched this code and hopefully can review it.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-04-11 11:13:15 PDT
Created attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

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.
Comment 7 Alexei Volkov 2008-04-14 16:23:20 PDT
Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

r=alexei
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-04-15 11:45:07 PDT
Bob, Do we want to get this into 3.12 or wait for 3.12.1 ?
Comment 9 Robert Relyea 2008-04-21 11:10:50 PDT
Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

3.12.1

r+ rrelyea
Comment 10 Robert Relyea 2008-04-21 11:11:57 PDT
Comment on attachment 315075 [details] [diff] [review]
Fix

obsoleted by nelson's patch. Good investigation work Honza

bob
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-04-26 19:09:32 PDT
lib/pkcs7/certread.c; new revision: 1.12; previous revision: 1.11
Comment 12 Nelson Bolyard (seldom reads bugmail) 2008-04-26 19:10:34 PDT
I'm not planning to put this into 3.12.0
Somebody holler if that's a problem.

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