@mozilla.org/security/certoverride;1 overrides crashes the application [@ nsNSSComponent::LogoutAuthenticatedPK11]

RESOLVED FIXED in mozilla2.0b8

Status

Core Graveyard
Security: UI
--
critical
RESOLVED FIXED
7 years ago
8 months ago

People

(Reporter: Petko D. (pdp) Petkov, Assigned: timeless)

Tracking

({crash})

unspecified
mozilla2.0b8
x86_64
Mac OS X
crash

Firefox Tracking Flags

(status1.9.2 .14-fixed, status1.9.1 .17-fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_3; en-us) AppleWebKit/531.22.7 (KHTML, like Gecko) Version/4.0.5 Safari/531.22.7
Build Identifier: 1.9.2

If we override the default @mozilla.org/security/certoverride;1 service with out own in order to handle calls to hasMatchingOverride ourselves but pass the rest of the functions to the original service, a crash will occur when opening chrome://pippki/content/certManager.xul inside the LoadCerts function, lines:

serverTreeView = Components.classes[nsCertTree].createInstance(nsICertTree);
serverTreeView.loadCertsFromCache(certcache, nsIX509Cert.SERVER_CERT);

I have not investigated why it crashes, just patched the function at runtime to avoid the behaviour.

Reproducible: Always

Steps to Reproduce:
1. I think the best way to reproduce the bug is to check the source here: http://code.google.com/p/websecurify/source/browse/trunk/xul/distribution/bundles/basic%40websecurify.gnucitizen.org/components/WebsecurifyBasicCertOverrideService.js
2.
3.
Actual Results:  
The application crashes.

Expected Results:  
The application should not crash.

Although my code now patches the certManager.xul at runtime, if patch removed, you can reproduce the crash by running the following tool:http://code.google.com/p/websecurify/source/browse/#svn/trunk/xul
(Assignee)

Comment 1

7 years ago
2318 nsresult nsNSSComponent::LogoutAuthenticatedPK11()
2319 {
2320   nsCOMPtr<nsICertOverrideService> icos = 
2321     do_GetService("@mozilla.org/security/certoverride;1");
2322     
2323   nsCertOverrideService *cos = 
2324     reinterpret_cast<nsCertOverrideService*>(icos.get());

Thanks for the report, indeed this is totally bogus. very sorry.
Assignee: nobody → kaie
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: Embedding: GRE Core → Security: UI
Ever confirmed: true
Keywords: crash
QA Contact: gre → ui
Summary: @mozilla.org/security/certoverride;1 overrides crashes the application → @mozilla.org/security/certoverride;1 overrides crashes the application [@ nsNSSComponent::LogoutAuthenticatedPK11]
Blocks: 460829
(Assignee)

Comment 2

7 years ago
Created attachment 444292 [details] [diff] [review]
replace static cast with magic number+message
Assignee: kaie → timeless
Status: NEW → ASSIGNED
Attachment #444292 - Flags: review?(kaie)
(Assignee)

Comment 3

7 years ago
Created attachment 444293 [details] [diff] [review]
 replace static cast with magic number+message
Attachment #444292 - Attachment is obsolete: true
Attachment #444293 - Flags: review?(kaie)
Attachment #444292 - Flags: review?(kaie)
(Assignee)

Comment 4

7 years ago
Created attachment 444294 [details] [diff] [review]
replace static cast with magic number+message

sorry. reed noted an extraneous comma, and then i raced and hg qref and camino won which means i lost :(
Attachment #444293 - Attachment is obsolete: true
Attachment #444294 - Flags: review?(kaie)
Attachment #444293 - Flags: review?(kaie)
Shouldn't we rather publish RemoveAllTemporaryOverrides in IDL?  There is an API to add a temp override (RememberValidityOverride), so there should be an API to remove it/remove all.
(Assignee)

Comment 6

7 years ago
not for 1.9.1 or 1.9.2. If we want to do that for trunk, we can do that. but we can't go screwing branches like someone did the last time.
(In reply to comment #6)
> not for 1.9.1 or 1.9.2. If we want to do that for trunk, we can do that. but we
> can't go screwing branches like someone did the last time.

nsICertOverrideService_1_9_1 and nsICertOverrideService_1_9_2 ?

Comment 8

7 years ago
Comment on attachment 444294 [details] [diff] [review]
replace static cast with magic number+message

>diff --git a/security/manager/ssl/src/nsCertOverrideService.cpp b/security/manager/ssl/src/nsCertOverrideService.cpp
>--- a/security/manager/ssl/src/nsCertOverrideService.cpp
>+++ b/security/manager/ssl/src/nsCertOverrideService.cpp
>@@ -696,6 +696,14 @@ nsCertOverrideService::AddEntryToList(co
> NS_IMETHODIMP
> nsCertOverrideService::ClearValidityOverride(const nsACString & aHostName, PRInt32 aPort)
> {
>+  if (aPort == 0) {
>+    if (aHostName.EqualsLiteral("all-temporary-certificates")) {
>+      RemoveAllTemporaryOverrides();
>+    } else {
>+      NS_ERROR("ClearValidityOverride called with unknown magic string"); 
>+    }
>+    return NS_OK;
>+  }


I'd prefer a magic string that is never a valid DNS name, please change 
  "all-temporary-certificates"
to
  "all:temporary:certificates"


I believe zero may be a valid port number, so you should "fall through" to the remainder of the function, if the hostname string doesn't matches the magic string, e.g.:

>+  if (aPort == 0 && aHostName.EqualsLiteral("all:temporary:certificates")) {
>+    RemoveAllTemporaryOverrides();
>+    return NS_OK;
>+  }


>diff --git a/security/manager/ssl/src/nsNSSComponent.cpp b/security/manager/ssl/src/nsNSSComponent.cpp
>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>@@ -2319,12 +2319,10 @@ nsresult nsNSSComponent::LogoutAuthentic
> {
>   nsCOMPtr<nsICertOverrideService> icos = 
>     do_GetService("@mozilla.org/security/certoverride;1");
>-    
>-  nsCertOverrideService *cos = 
>-    reinterpret_cast<nsCertOverrideService*>(icos.get());
>-
>-  if (cos) {
>-    cos->RemoveAllTemporaryOverrides();
>+  if (icos) {
>+    icos->ClearValidityOverride(
>+            NS_LITERAL_CSTRING("all-temporary-certificates"),

adjust string to "all:temporary:certificates", too


r=kaie with the above changes, and thanks for the patch!
Attachment #444294 - Flags: review?(kaie) → review-

Updated

7 years ago
Whiteboard: [psm-fatal]

Comment 9

7 years ago
I agree we need a workaround like this patch for the stable branches.

I agree it would be better to add the new API on trunk.
(Reporter)

Comment 10

7 years ago
The patch works but it hasn't been applied to either to mozilla-central or any other branch. Any idea when this will get included? I know of at least two projects/extensions which crash firefox because of this bug.
(Assignee)

Comment 11

7 years ago
Created attachment 492620 [details] [diff] [review]
all: and proper chaining for port 0

we need this to fix an api botch in 1.9.1, 1.9.2, and trunk.

At some later point we can investigate writing a better api.
Attachment #444294 - Attachment is obsolete: true
Attachment #492620 - Flags: review+
Attachment #492620 - Flags: approval2.0?
Attachment #492620 - Flags: approval1.9.2.14?
Attachment #492620 - Flags: approval1.9.1.17?
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Attachment #492620 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/0d7dd95a1d9a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Comment on attachment 492620 [details] [diff] [review]
all: and proper chaining for port 0

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers
Attachment #492620 - Flags: approval1.9.2.14?
Attachment #492620 - Flags: approval1.9.2.14+
Attachment #492620 - Flags: approval1.9.1.17?
Attachment #492620 - Flags: approval1.9.1.17+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Comment 14

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dfad864f20a0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7feca19ea3b0
status1.9.1: --- → .17-fixed
status1.9.2: --- → .14-fixed
Keywords: checkin-needed
Can I get clear steps to reproduce?

I notice that http://code.google.com/p/websecurify/source/browse/trunk/xul/distribution/bundles/basic%40websecurify.gnucitizen.org/components/WebsecurifyBasicCertOverrideService.js is a 404.
Whiteboard: [psm-fatal] → [psm-fatal] [qa-examined-191] [qa-examined-192]
Crash Signature: [@ nsNSSComponent::LogoutAuthenticatedPK11]

Comment 16

6 years ago
Can we get this patch into the trunk please? As of now any extension which overrides this service will crash Firefox, including Selenium which makes firefox a bit unstable for development purposes.

Comment 17

6 years ago
I am referring to moz2.0 and moz5.0
http://hg.mozilla.org/releases/mozilla-2.0/rev/0d7dd95a1d9a
http://hg.mozilla.org/releases/mozilla-release/rev/0d7dd95a1d9a (5.0)
Actually, the patch didn't fix this bug as filed.  There were multiple places in PSM that had this issue, but the patch only fixed the first one hit by certManager.xul....  It's too bad no one answered Al's question from comment 15.  :(

Bug 666516 covers the remaining issues here.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.