Closed
Bug 529485
Opened 15 years ago
Closed 13 years ago
PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
VERIFIED
FIXED
People
(Reporter: bugzilla-graveyard, Assigned: timeless)
References
Details
(Keywords: crash, Whiteboard: [psm-fatal] [qa-examined-191] [qa-examined-192] [qa-needs-STR])
Crash Data
Attachments
(3 files, 4 obsolete files)
28.68 KB,
patch
|
KaiE
:
review+
christian
:
approval1.9.2.14-
christian
:
approval1.9.1.17-
|
Details | Diff | Splinter Review |
9.70 KB,
patch
|
Details | Diff | Splinter Review | |
884 bytes,
patch
|
briansmith
:
review+
Dolske
:
approval2.0+
dveditz
:
approval1.9.2.14-
dveditz
:
approval1.9.2.17+
dveditz
:
approval1.9.1.17-
dveditz
:
approval1.9.1.19+
|
Details | Diff | Splinter Review |
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•15 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.
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 ]
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 4•15 years ago
|
||
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•15 years ago
|
||
or, just break the patch up into something that fixes the bug, and another patch that cleans up the code.
chofmann: i did. there are three patches, the patch nelson wants is: Node ID a016173e025e277f07f10dbb710fc864aabcd0e5
Comment 7•15 years ago
|
||
What's the bugzilla attachment number of that patch?
Updated•14 years ago
|
Whiteboard: [psm-fatal]
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.
Comment 10•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #494370 -
Flags: review? → review?(kaie)
Comment 11•14 years ago
|
||
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)
Comment 12•14 years ago
|
||
This is the last of Timeless's 3 original patches to this file.
Attachment #494373 -
Flags: review?(kaie)
Updated•14 years ago
|
Attachment #494370 -
Attachment description: Large white-space only patch by Timeless → Large white-space and variable renaming patch by Timeless
Comment 13•14 years ago
|
||
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.
Updated•14 years ago
|
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ ProcessAuthKeyId ] → PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails
Comment 14•14 years ago
|
||
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•14 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•14 years ago
|
||
Attachment #499301 -
Attachment is obsolete: true
Attachment #499301 -
Flags: review?(kaie)
Comment 16•14 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•14 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•14 years ago
|
blocking2.0: --- → ?
Comment 18•14 years ago
|
||
FWIW, alternatively, this is the minimal patch sufficient to fix the crash, not depending on anything else, applies to latest trunk.
Updated•14 years ago
|
Attachment #413904 -
Flags: approval2.0?
Attachment #413904 -
Flags: approval1.9.2.14?
Attachment #413904 -
Flags: approval1.9.1.17?
Comment 19•14 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•14 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•14 years ago
|
||
I'll wait for trunk approval before landing any of these.
Comment 22•13 years ago
|
||
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•13 years ago
|
||
Why are FF 4 drivers not interested in a crash fix?
Comment 24•13 years ago
|
||
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]
Comment 25•13 years ago
|
||
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•13 years ago
|
Attachment #499309 -
Flags: review?(bsmith)
Updated•13 years ago
|
Attachment #499309 -
Flags: review?(honzab.moz)
Comment 26•13 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•13 years ago
|
Attachment #499309 -
Flags: approval2.0?
Updated•13 years ago
|
Attachment #413904 -
Flags: approval2.0?
Updated•13 years ago
|
Attachment #499309 -
Flags: review?(bsmith) → review+
Updated•13 years ago
|
Attachment #499309 -
Flags: review?(honzab.moz)
Attachment #499309 -
Flags: approval2.0?
Attachment #499309 -
Flags: approval2.0+
Comment 27•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Comment 28•13 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?
Comment 30•13 years ago
|
||
(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 31•13 years ago
|
||
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+
Keywords: checkin-needed
Whiteboard: [psm-fatal] → [psm-fatal][cn: 1.9.2.15, 1.9.1.18]
Comment 32•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/20c82a064ebf
status1.9.1:
--- → .18-fixed
Comment 33•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/602a39517f69
status1.9.2:
--- → .15-fixed
Comment 34•13 years ago
|
||
Can't find recent reports for this crash -- marking VERIFIED.
Status: RESOLVED → VERIFIED
Comment 35•13 years ago
|
||
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]
Comment 36•13 years ago
|
||
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.
Comment 37•13 years ago
|
||
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]
Updated•13 years ago
|
Crash Signature: [@ ProcessAuthKeyId ]
You need to log in
before you can comment on or make changes to this bug.
Description
•