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

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Chris Lawson (gone)]

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]

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]

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]

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

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]

or, just break the patch up into something that fixes the bug, and another patch that cleans up the code.

Comment 6

[timeless]

chofmann: i did. there are three patches, the patch nelson wants is:
Node ID a016173e025e277f07f10dbb710fc864aabcd0e5

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

What's the bugzilla attachment number of that patch?

Comment 9

[timeless]

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

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

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

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

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

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

Attachment: timeless patch, cleaned up, v3, -U20

Comment 16

[Kai Engert [:KaiE:]]

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

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

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

[LegNeato]

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

I'll approve the minimal patch.

Comment 20

[LegNeato]

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

I'll wait for trunk approval before landing any of these.

Comment 22

[Daniel Veditz [:dveditz]]

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

Why are FF 4 drivers not interested in a crash fix?

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

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

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

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

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

now that trunk is fixed, rerequesting approval for stable branches

Comment 30

[u279076]

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

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 32

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/20c82a064ebf

Comment 33

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/602a39517f69

Comment 34

[u279076]

Can't find recent reports for this crash -- marking VERIFIED.

Comment 35

[Marco Bonardo [:mak]]

this was pushed to all branches, removing checkin-needed request, correct me if I'm wrong please.

Comment 36

[Daniel Veditz [:dveditz]]

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

[Al Billings [:abillings - ex-MoCo]]

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.