Last Comment Bug 666516 - overriding "@mozilla.org/security/certoverride;1" causes a crash in the certManager.xul
: overriding "@mozilla.org/security/certoverride;1" causes a crash in the certM...
Status: RESOLVED FIXED
[qa?]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
P1 critical (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-23 00:43 PDT by passfree
Modified: 2011-09-19 23:26 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Fix (5.52 KB, patch)
2011-08-23 13:09 PDT, Boris Zbarsky [:bz] (still a bit busy)
kaie: review+
blizzard: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image passfree 2011-06-23 00:43:14 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.797.0 Safari/535.1
Build Identifier: xulrunner (all versions)

This is an old bug and it was reported like 2 years ago. I thought it was fixed but that is not the case in xul2.0 and perhaps even xul5.0

In summary, if we override @mozilla.org/security/certoverride;1 via the following chrome manifest

component      {6c367300-5a77-11df-a08a-0800200c9a61}                    components/CertOverrideService.js
contract       @mozilla.org/security/certoverride;1                      {6c367300-5a77-11df-a08a-0800200c9a61}

A crash will occur within certManager.xul when the following line is invoked 

serverTreeView.loadCertsFromCache(certcache, nsIX509Cert.SERVER_CERT);

As far as I remember the issue was somehow related to the way the XPCOM instance was handled in the C code (old code conventions?).

This service @mozilla.org/security/certoverride;1 is actually overridden by several popular extensions and products such as Selenium. A legitimate reason for overriding it is to hide bad ssl certificate problems.

Reproducible: Always

Steps to Reproduce:
See above.

Actual Results:  
The app crashes when certManager.xul is opened.

Expected Results:  
It should not crash.
Comment 1 User image Matthias Versen [:Matti] 2011-06-23 03:06:48 PDT
You mean bug 564584 which is marked fixed
Comment 2 User image passfree 2011-06-23 03:16:10 PDT
Unfortunately, not fixed.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 07:46:13 PDT
If you're getting a crash here, what's the stack look like?  Is there a breakpad incident id?
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 07:51:38 PDT
Actually, nevermind.  nsCertTree::GetCertsByTypeFromCertList has exactly the sort of code that was causing crashes in bug 564584, and the thing it's working with was gotten by contract id...  <sigh>.
Comment 5 User image passfree 2011-08-21 02:23:47 PDT
I can confirm that Xulrunner 6.0, FF6 still crash.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 13:09:07 PDT
Created attachment 555170 [details] [diff] [review]
Fix

This is ridiculous.  This code should never have been written....
Comment 7 User image Kai Engert (:kaie) 2011-08-23 13:51:57 PDT
Comment on attachment 555170 [details] [diff] [review]
Fix

Yes, I agree this code wasn't written as future-proof as it should had.

And yes, we need more people caring for PSM and helping out with it, in particular with writing/reviewing patches and fixing correctness issues.

Thanks a lot for your fix, I appreciate it.

r=kaie
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 14:04:44 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c84cf6b4d2c
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 14:05:20 PDT
Comment on attachment 555170 [details] [diff] [review]
Fix

Given that this is causing extension crashes, we should at least consider it for aurora.
Comment 10 User image Christopher Blizzard (:blizzard) 2011-08-23 14:33:06 PDT
Comment on attachment 555170 [details] [diff] [review]
Fix

Approved, please land on Aurora (Firefox Update 8.)
Comment 11 User image Marco Bonardo [::mak] 2011-08-24 01:46:13 PDT
http://hg.mozilla.org/mozilla-central/rev/9c84cf6b4d2c
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2011-08-25 20:56:45 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/9546f9e05e7a
Comment 13 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-19 15:58:37 PDT
Can QA get some help verifying this bug fix? Either passfree, if you can verify this on Firefox 8 Aurora, or if you can provide some more details on how to use the testcase you provided. It's not completely clear to QA how to utilize your testcase.

Thanks
Comment 14 User image passfree 2011-09-19 23:26:51 PDT
I have verified that it works in 8. Though, I am not completely sure if this bug has been fully fixed everywhere.

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