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

RESOLVED FIXED in 3.12.1

Status

NSS
Libraries
P2
critical
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mayhemer)

Tracking

(Blocks: 1 bug, {crash, testcase})

unspecified
3.12.1
x86
Windows XP
crash, testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
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.
(Assignee)

Comment 2

9 years ago
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.
Assignee: kengert → honzab
Status: NEW → ASSIGNED
Attachment #315075 - Flags: review?(kengert)

Comment 3

9 years ago
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
Status: ASSIGNED → NEW
Component: Security: PSM → Libraries
Product: Core → NSS
QA Contact: psm → libraries
Version: Trunk → unspecified

Comment 4

9 years ago
Comment on attachment 315075 [details] [diff] [review]
Fix

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

Comment 5

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: nobody → honzab
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.
Attachment #315142 - Flags: superreview?(alexei.volkov.bugs)
Attachment #315142 - Flags: review?(kengert)

Comment 7

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

r=alexei
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 9

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

3.12.1

r+ rrelyea
Attachment #315142 - Flags: review?(kengert) → review+

Comment 10

9 years ago
Comment on attachment 315075 [details] [diff] [review]
Fix

obsoleted by nelson's patch. Good investigation work Honza

bob
Attachment #315075 - Attachment is obsolete: true
Attachment #315075 - Flags: review?(rrelyea)

Updated

9 years ago
Keywords: checkin-needed
lib/pkcs7/certread.c; new revision: 1.12; previous revision: 1.11
Status: NEW → RESOLVED
Last Resolved: 9 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
Keywords: checkin-needed
Depends on: 453227
Crash Signature: [@ CERT_DecodeCertPackage]
You need to log in before you can comment on or make changes to this bug.