PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails

VERIFIED FIXED

Status

()

Core
Security: PSM
--
critical
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: Chris Lawson (gone), Assigned: timeless)

Tracking

({crash})

unspecified
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -, status1.9.2 .17-fixed, status1.9.1 .19-fixed)

Details

(Whiteboard: [psm-fatal] [qa-examined-191] [qa-examined-192] [qa-needs-STR], crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
We've got three reports of this in Camino now, and four in Firefox:

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=ProcessAuthKeyId&version=Camino%3A2.0 (Camino, all on Mac OS X 10.6.2)

http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=ProcessAuthKeyId&date=&range_value=4&range_unit=weeks&do_query=1&signature=ProcessAuthKeyId (Firefox, two on OS X 10.6.2 with 3.6b, and two on XP SP2 with 3.0.x)

The ones in Camino, at least, appear to have been triggered by local imports of certificates (2) and by adding a certificate exception (1).

It looks to me from basic code inspection around the crash:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp&rev=1.38&mark=1258#1258

that if CERT_DecodeAuthKeyID (arena, extData) fails for whatever reason (line 1256), |ret| could be null, causing a crash on line 1258.

Kai, is there any way that could fail (e.g., with self-signed certificates or any other sorts of non-kosher things that someone might be doing locally) and be causing this?
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash → CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ProcessAuthKeyId ]

Comment 1

8 years ago
I think I see reports of the same problem in firefox 3.6b3 data with comments

20091119-crashdata.csv:ProcessAuthKeyId 3.6b3 I tried to view the self-signed certificate used for internal web server and signed by local CA.

20091119-crashdata.csv:ProcessAuthKeyId 3.6b3 Tried to view just imported self-signed certificate.

not sure how much kai is able to look at bugs these days.  is there someone else that can take a look?   might be a good test case to add.

I spotted a report on windows as well but most of these seem to be mac.
(Assignee)

Comment 2

8 years ago
Signature	ProcessAuthKeyId
UUID	c7faa097-3192-4a5b-8d36-642942091110
Time 	2009-11-10 04:51:31.201930
Uptime	107
Product	Camino
Version	2.0
Build ID	2009102617
Branch	1.9.0
OS	Mac OS X
OS Version	10.6.2 10C540
CPU	x86
CPU Info	GenuineIntel family 6 model 14 stepping 8
Crash Reason	EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
Crash Address	0x8
User Comments	Importing certificate lead to the crash.
Processor Notes 	WARNING: Json file missing Add-ons
Related Bugs

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	Camino 	ProcessAuthKeyId 	mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1258
1 	Camino 	ProcessExtensionData 	mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1649
2 	Camino 	nsNSSCertificate::CreateTBSCertificateASN1Struct 	mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1713
3 	Camino 	nsNSSCertificate::CreateASN1Struct 	mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:2164
4 	Camino 	nsNSSCertificate::GetASN1Structure 	mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:1450
5 	Camino 	-[CertificateItem ensureASN1Info] 	mozilla/camino/src/security/CertificateItem.mm:747
6 	Camino 	-[CertificateItem ASN1PropertyWithKeyPath:] 	mozilla/camino/src/security/CertificateItem.mm:761
7 	Camino 	-[CertificateItem version] 	mozilla/camino/src/security/CertificateItem.mm:322
8 	Camino 	-[CertificateView rebuildDetails] 	mozilla/camino/src/security/CertificateView.mm:618
9 	Camino 	+[ViewCertificateDialogController showCertificateWindowWithCertificateItem:certTypeForTrustSettings:] 	mozilla/camino/src/security/ViewCertificateDialogController.mm:86
10 	Camino 	-[CertificatesWindowController viewSelectedCertificates:] 	mozilla/camino/src/security/CertificatesWindowController.mm:468
Depends on: 259031
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ProcessAuthKeyId ] → CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ ProcessAuthKeyId ]
(Assignee)

Comment 3

8 years ago
Created attachment 413904 [details] [diff] [review]
patch v1, with whitespace changes and renamed variables

this is a changeset sequence generated with hg export. ideally importing it should result in 3 changesets....
Assignee: kaie → timeless
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #413904 - Flags: review?(kaie)
Comment on attachment 413904 [details] [diff] [review]
patch v1, with whitespace changes and renamed variables

I suppose that somewhere inside this patch there's a change that addresses this bug, but it is completely obscured by many lines that change nothing but whitespace. :(
If bugzilla's patch viewer gave us an option to ignore whitespace (bug 285814) we could easily ignore all that, but as things are, please attach a patch which ignores whitespace changes.  Thanks.

Comment 5

8 years ago
or, just break the patch up into something that fixes the bug, and another patch that cleans up the code.
(Assignee)

Comment 6

8 years ago
chofmann: i did. there are three patches, the patch nelson wants is:
Node ID a016173e025e277f07f10dbb710fc864aabcd0e5
What's the bugzilla attachment number of that patch?

Updated

7 years ago
Whiteboard: [psm-fatal]
Duplicate of this bug: 614342
Blocks: 614342
(Assignee)

Comment 9

7 years ago
nelson, it's the section after
https://bugzilla.mozilla.org/attachment.cgi?id=413904&action=diff#a/security/manager/ssl/src/nsNSSCertHelper.cpp_sec38

-- bugzilla's attachment viewer is buggy, it can't link to it.
Created attachment 494370 [details] [diff] [review]
Large white-space and variable renaming patch by Timeless

The patch file originally attached to this bug was actually 3 separate 
patches, all made to the same file.  Small wonder that bugzilla couldn't
create links to the second and third ones, since it assumes that each 
patch file contains at most one patch to any source files.  

I split that patch into three separate patches, which I will attach here.
One of them clearly addresses the crash reported in this bug.  The other 
two may serve some other purpose.
Attachment #494370 - Flags: review?
Attachment #494370 - Flags: review? → review?(kaie)
Created attachment 494372 [details] [diff] [review]
fix crash, written by Timeless, depends on Timeless's previous variable renaming patch.

This is the patch that addresses the crash.  One of the people who works on 
PSM should review it.  But it's so small that maybe I will do that.
Attachment #413904 - Attachment is obsolete: true
Attachment #494372 - Flags: review?(kaie)
Attachment #413904 - Flags: review?(kaie)
Created attachment 494373 [details] [diff] [review]
Third patch by Timeless, changes error propagation

This is the last of Timeless's 3 original patches to this file.
Attachment #494373 - Flags: review?(kaie)
Attachment #494370 - Attachment description: Large white-space only patch by Timeless → Large white-space and variable renaming patch by Timeless
Comment on attachment 494372 [details] [diff] [review]
fix crash, written by Timeless, depends on Timeless's previous variable renaming patch.

If this patch did not depend on the other, much larger patch attached to this 
bug, which changes whitespace and renames variables, I would be inclined to 
recommend that it get a positive review.
Attachment #494372 - Attachment description: patch to fix crash, written by timeless → fix crash, written by Timeless, depends on Timeless's previous variable renaming patch.
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ ProcessAuthKeyId ] → PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails

Comment 14

6 years ago
Created attachment 499301 [details] [diff] [review]
timeless patch, cleaned up, v3

I also have trouble to read patches where the meat is hidden.

I applied the patches, did a -w diff, revert the original patch, applied the -w patch, reverted the key/ret rename, etc.

The result is this new patch, which I believe contains all the effective changes from the original patch.
Attachment #494370 - Attachment is obsolete: true
Attachment #494372 - Attachment is obsolete: true
Attachment #494373 - Attachment is obsolete: true
Attachment #499301 - Flags: review?(kaie)
Attachment #494370 - Flags: review?(kaie)
Attachment #494372 - Flags: review?(kaie)
Attachment #494373 - Flags: review?(kaie)

Updated

6 years ago
Attachment #413904 - Attachment description: this should fix it → patch v1, with whitespace changes and renamed variables
Attachment #413904 - Attachment is obsolete: false

Comment 15

6 years ago
Created attachment 499306 [details] [diff] [review]
timeless patch, cleaned up, v3, -U20
Attachment #499301 - Attachment is obsolete: true
Attachment #499301 - Flags: review?(kaie)

Comment 16

6 years ago
If you had attached a patch like attachment 494372 [details] [diff] [review] right away, which seems to be sufficient to fix the crash, you would have gotten an r+ much earlier.

I'm sorry that we're slow and have too much work, but that's the way it is.
Doing minimal patches helps to keep turnaround times small.

Now, I have reviewed attachment 499306 [details] [diff] [review], and I'm OK with it.
I'm also OK with whitespace changes and I don't mind renaming that variable, so I'll r+ the original patch.

Comment 17

6 years ago
Comment on attachment 413904 [details] [diff] [review]
patch v1, with whitespace changes and renamed variables

I have reviewed attachment 499306 [details] [diff] [review], and this patch is functionally equivalent to it.

r=kaie
Attachment #413904 - Flags: review+

Updated

6 years ago
blocking2.0: --- → ?

Comment 18

6 years ago
Created attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

FWIW, alternatively,

this is the minimal patch sufficient to fix the crash, not depending on anything else, applies to latest trunk.

Updated

6 years ago
Attachment #413904 - Flags: approval2.0?
Attachment #413904 - Flags: approval1.9.2.14?
Attachment #413904 - Flags: approval1.9.1.17?

Comment 19

6 years ago
Comment on attachment 413904 [details] [diff] [review]
patch v1, with whitespace changes and renamed variables

I'll approve the minimal patch.
Attachment #413904 - Flags: approval1.9.2.14?
Attachment #413904 - Flags: approval1.9.2.14-
Attachment #413904 - Flags: approval1.9.1.17?
Attachment #413904 - Flags: approval1.9.1.17-

Comment 20

6 years ago
Comment on attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

a=LegNeato for 1.9.2.14 and 1.9.1.17
Attachment #499309 - Flags: approval1.9.2.14+
Attachment #499309 - Flags: approval1.9.1.17+

Comment 21

6 years ago
I'll wait for trunk approval before landing any of these.
Comment on attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

Missed non-blocker code-freeze for 1.9.1.17 and 1.9.2.14 -- rescinding approval, you can try again next time.
Attachment #499309 - Flags: approval1.9.2.14-
Attachment #499309 - Flags: approval1.9.2.14+
Attachment #499309 - Flags: approval1.9.1.17-
Attachment #499309 - Flags: approval1.9.1.17+

Comment 23

6 years ago
Why are FF 4 drivers not interested in a crash fix?
This is a crash with a patch that has been r+d and so we should just consider it a blocker and/or give explicit 2.0 approval. The patch simply prevents a null-pointer dereference so there's no chance of regression.
Whiteboard: [psm-fatal] → [psm-fatal][hardblocker]
It seems this bug exists in Firefox 3.6, so nearly by definition it's not a blocker (soft or hard) at this point. [Alas triage may have been slow here if no one was looking for requests here.]

But I'll go ahead and a+ the simplified version since it's simple and comment 24 says it's low risk.
blocking2.0: ? → -
Whiteboard: [psm-fatal][hardblocker] → [psm-fatal]

Updated

6 years ago
Attachment #499309 - Flags: review?(bsmith)

Updated

6 years ago
Attachment #499309 - Flags: review?(honzab.moz)

Comment 26

6 years ago
Brian or Honza, could you please review this obvious minimal patch?
Drivers don't want to take the large patch for 2.0
Thanks

Updated

6 years ago
Attachment #499309 - Flags: approval2.0?

Updated

6 years ago
Attachment #413904 - Flags: approval2.0?
Attachment #499309 - Flags: review?(bsmith) → review+
Attachment #499309 - Flags: review?(honzab.moz)
Attachment #499309 - Flags: approval2.0?
Attachment #499309 - Flags: approval2.0+

Comment 27

6 years ago
http://hg.mozilla.org/mozilla-central/rev/d0798ddf795c

Minimal patch landed.

Given how much time we spent on this, we should land the full patch later.
Let's hope we won't forget.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 28

6 years ago
Comment on attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

now that trunk is fixed, rerequesting approval for stable branches
Attachment #499309 - Flags: approval1.9.2.15?
Attachment #499309 - Flags: approval1.9.1.18?

Updated

6 years ago
Duplicate of this bug: 614342
(In reply to comment #28)
> Comment on attachment 499309 [details] [diff] [review]
> alternative, minimal patch (meat without cleanup)
> 
> now that trunk is fixed, rerequesting approval for stable branches

Was this landed on the branches? I don't see the request flag anymore.
Comment on attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Attachment #499309 - Flags: approval1.9.2.15?
Attachment #499309 - Flags: approval1.9.2.15+
Attachment #499309 - Flags: approval1.9.1.18?
Attachment #499309 - Flags: approval1.9.1.18+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [psm-fatal] → [psm-fatal][cn: 1.9.2.15, 1.9.1.18]

Comment 32

6 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/20c82a064ebf
status1.9.1: --- → .18-fixed

Comment 33

6 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/602a39517f69
status1.9.2: --- → .15-fixed
Can't find recent reports for this crash -- marking VERIFIED.
Status: RESOLVED → VERIFIED
this was pushed to all branches, removing checkin-needed request, correct me if I'm wrong please.
Keywords: checkin-needed
Whiteboard: [psm-fatal][cn: 1.9.2.15, 1.9.1.18] → [psm-fatal]
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Are there any STR for 1.9.1 or 1.9.2 for this? I don't see any way to verify the fix here.
Whiteboard: [psm-fatal] → [psm-fatal] [qa-examined-191] [qa-examined-192] [qa-needs-STR]
Crash Signature: [@ ProcessAuthKeyId ]
You need to log in before you can comment on or make changes to this bug.