Last Comment Bug 529485 - PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails
: PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) f...
Status: VERIFIED FIXED
[psm-fatal] [qa-examined-191] [qa-exa...
: crash
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: timeless
:
Mentors:
: 614342 (view as bug list)
Depends on: 259031
Blocks: 614342
  Show dependency treegraph
 
Reported: 2009-11-17 22:56 PST by Chris Lawson (gone)
Modified: 2011-06-13 10:01 PDT (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
.17-fixed
.19-fixed


Attachments
patch v1, with whitespace changes and renamed variables (28.68 KB, patch)
2009-11-22 04:41 PST, timeless
kaie: review+
christian: approval1.9.2.14-
christian: approval1.9.1.17-
Details | Diff | Review
Large white-space and variable renaming patch by Timeless (24.91 KB, patch)
2010-12-01 07:27 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
fix crash, written by Timeless, depends on Timeless's previous variable renaming patch. (805 bytes, patch)
2010-12-01 07:30 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
Third patch by Timeless, changes error propagation (2.98 KB, patch)
2010-12-01 07:33 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Review
timeless patch, cleaned up, v3 (6.33 KB, patch)
2010-12-22 08:39 PST, Kai Engert (:kaie)
no flags Details | Diff | Review
timeless patch, cleaned up, v3, -U20 (9.70 KB, patch)
2010-12-22 08:47 PST, Kai Engert (:kaie)
no flags Details | Diff | Review
alternative, minimal patch (meat without cleanup) (884 bytes, patch)
2010-12-22 09:11 PST, Kai Engert (:kaie)
brian: 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 | Review

Description Chris Lawson (gone) 2009-11-17 22:56:31 PST
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?
Comment 1 chris hofmann 2009-11-21 18:23:47 PST
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.
Comment 2 timeless 2009-11-22 00:06:26 PST
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
Comment 3 timeless 2009-11-22 04:41:46 PST
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....
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-11-22 12:46:44 PST
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 chris hofmann 2009-11-22 12:56:03 PST
or, just break the patch up into something that fixes the bug, and another patch that cleans up the code.
Comment 6 timeless 2009-11-22 13:52:37 PST
chofmann: i did. there are three patches, the patch nelson wants is:
Node ID a016173e025e277f07f10dbb710fc864aabcd0e5
Comment 7 Nelson Bolyard (seldom reads bugmail) 2009-11-22 14:01:09 PST
What's the bugzilla attachment number of that patch?
Comment 8 Daniel Veditz [:dveditz] 2010-11-30 12:34:17 PST
*** Bug 614342 has been marked as a duplicate of this bug. ***
Comment 9 timeless 2010-11-30 15:42:26 PST
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 Nelson Bolyard (seldom reads bugmail) 2010-12-01 07:27:48 PST
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.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2010-12-01 07:30:50 PST
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.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2010-12-01 07:33:03 PST
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.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2010-12-01 07:45:08 PST
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.
Comment 14 Kai Engert (:kaie) 2010-12-22 08:39:02 PST
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.
Comment 15 Kai Engert (:kaie) 2010-12-22 08:47:55 PST
Created attachment 499306 [details] [diff] [review]
timeless patch, cleaned up, v3, -U20
Comment 16 Kai Engert (:kaie) 2010-12-22 09:04:34 PST
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 Kai Engert (:kaie) 2010-12-22 09:05:56 PST
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
Comment 18 Kai Engert (:kaie) 2010-12-22 09:11:08 PST
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.
Comment 19 christian 2010-12-27 10:30:41 PST
Comment on attachment 413904 [details] [diff] [review]
patch v1, with whitespace changes and renamed variables

I'll approve the minimal patch.
Comment 20 christian 2010-12-27 10:31:10 PST
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
Comment 21 Kai Engert (:kaie) 2010-12-27 11:31:01 PST
I'll wait for trunk approval before landing any of these.
Comment 22 Daniel Veditz [:dveditz] 2011-01-12 10:46:54 PST
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.
Comment 23 Kai Engert (:kaie) 2011-01-12 10:51:03 PST
Why are FF 4 drivers not interested in a crash fix?
Comment 24 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-01-13 12:59:50 PST
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.
Comment 25 Justin Dolske [:Dolske] 2011-01-13 14:07:40 PST
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.
Comment 26 Kai Engert (:kaie) 2011-01-13 14:14:31 PST
Brian or Honza, could you please review this obvious minimal patch?
Drivers don't want to take the large patch for 2.0
Thanks
Comment 27 Kai Engert (:kaie) 2011-01-13 14:35:21 PST
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.
Comment 28 Kai Engert (:kaie) 2011-01-13 14:36:06 PST
Comment on attachment 499309 [details] [diff] [review]
alternative, minimal patch (meat without cleanup)

now that trunk is fixed, rerequesting approval for stable branches
Comment 29 Kai Engert (:kaie) 2011-01-13 14:40:12 PST
*** Bug 614342 has been marked as a duplicate of this bug. ***
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-01-22 10:23:46 PST
(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 Daniel Veditz [:dveditz] 2011-01-24 10:40:16 PST
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
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-02-18 14:58:30 PST
Can't find recent reports for this crash -- marking VERIFIED.
Comment 35 Marco Bonardo [::mak] 2011-02-19 05:45:05 PST
this was pushed to all branches, removing checkin-needed request, correct me if I'm wrong please.
Comment 36 Daniel Veditz [:dveditz] 2011-03-04 10:15:00 PST
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 [On PTO until 6/29] 2011-03-22 17:30:58 PDT
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.

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