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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: 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 (dead)]

Attachment: 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)]

Attachment: 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:)]

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:)]

Comment on attachment 315075 [details] [diff] [review]
Fix

Bob should review this code.

Comment 5

[Kai Engert (:KaiE:)]

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)]

Attachment: 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]

Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

r=alexei

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Bob, Do we want to get this into 3.12 or wait for 3.12.1 ?

Comment 9

[Robert Relyea]

Comment on attachment 315142 [details] [diff] [review]
Fix all the incorrect usages of sizeof

3.12.1

r+ rrelyea

Comment 10

[Robert Relyea]

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)]

lib/pkcs7/certread.c; new revision: 1.12; previous revision: 1.11

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I'm not planning to put this into 3.12.0
Somebody holler if that's a problem.