800386 - Self signed certificates cannot be whitelisted with FoxyProxy Plus extension

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Georg Koppen]

Attachment: test.xpi

User Agent: Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121010030605

Steps to reproduce:

A user installed FoxyProxy Plus on a recent Firefox (16) and tried to whitelist self-signed certificates.


Actual results:

The user could not click on "Confirm Security Exception" (it was greyed out) and he got a 

"No information available
 Unable to obtain identification status for the given site." 

on the usual "Add Security Exception" dialog.

I tracked the issue down a bit: The last working nightly is the one from 2012-03-31 and the first broken one is from 2012-04-01. Furthermore, comparing the mozilla-central pushlog (http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c43cfe73516&tochange=4a8a5e8ef78b) and the one from mozilla-inbound it seems the culprit is http://hg.mozilla.org/mozilla-central/rev/bbe5086163c9 which results in the following bug range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7bfb26afd19b&tochange=5b524a2b9ec1

Attached is a simple test case (extension) showing the issue. In order to reproduce it do the following:

1) Install the extension in a new profile in a recent Firefox (say 15.0.1)
2) Try to add a security exception for a site with a self-signed certificate (e.g. https://svn.jondos.de)


Expected results:

The user should be able to whitelist the sites with self-signed certificates.

Comment 1

[Renaud Allard]

I confirm this issue. I must admit it is annoying to not be able to even whitelist if information is unknown. For example, if you have HP ILO access, you cannot even access the ILO as you cannot whitelist it.

Comment 2

[Matthias Versen [:Matti]]

The first bad revision is:
changeset:   90729:1bdb337e3136
user:        Peter Van der Beken <[e-mail]>
date:        Fri Mar 30 21:42:20 2012 -0700
summary:     Fix for bug 740069 (Generate JS bindings in C++ with a python scrip
t for DOM objects on the main thread and in workers. Infrastructure and new bind
ings for XMLHttpRequest). Patch by bent/bz/bholley/jst/khuey/peterv, r=bent/bz/b
holley/jlebar/khuey/peterv/sicking/smaug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, what's foxyproxy doing to mess with that XHR?  ;)

Any exceptions in error console?

Comment 4

[Georg Koppen]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Well, what's foxyproxy doing to mess with that XHR?  ;)
Well, we are following your advice :):
"(06:56:39 PM) bz: ericjung: how about just returning the original notificationCallbacks?
 (06:56:51 PM) bz: ericjung: if asked for an nsIBadCertListener/2?"
The code is part of hooking nsIAuthPrompt, nsIAuthPrompt2 and nsIAuthPromprProvider in order to replace some of their functionality with a custom one.
 
> Any exceptions in error console?
Only this one:

Timestamp: 13.10.2012 20:17:25
Error: Attempted to connect to a site with a bad certificate in the add exception dialog. This results in a (mostly harmless) exception being thrown. Logged for information purposes only: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: checkCert :: line 135"  data: no]
Source File: chrome://pippki/content/exceptionDialog.js
Line: 143

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  So my first guess was bug 780529, but we fixed that in 16.

Then again, it might be a manifestation of the same issue.  What does the relevant foxyproxy code that sets up the notification callbacks and deal with nsIBadCertListener stuff look like?

Comment 6

[Eric Jung [:ericjung]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Then again, it might be a manifestation of the same issue.  What does the
> relevant foxyproxy code that sets up the notification callbacks and deal
> with nsIBadCertListener stuff look like?

This piece of FoxyProxy is not open-source. Is it possible to either secure this ticket or send you the relevant code in a non-public manner?

Comment 7

[Boris Zbarsky [:bzbarsky]]

You can certainly send it directly to my e-mail address.  I _could_ mark the entire bug security-sensitive, but that's not quite what we want in general....

Comment 8

[Georg Koppen]

(In reply to Boris Zbarsky (:bz) from comment #7)
> You can certainly send it directly to my e-mail address.  I _could_ mark the
> entire bug security-sensitive, but that's not quite what we want in
> general....
No need for such a drastic measure. Especially as this is no security issue and other users/add-on developers may be affected by it as well. I just sent you an e-mail with the code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Thanks.  Sorry for the lag; the upstream spam filters didn't like it...

So looking at your code, a question.  In your getInterface implementation, is it possible that the call through to getInterface on original notification callbacks is throwing?  It looks to me like if it did that exception would just get silently swallowed, maybe.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Anyway, trying it now with the attached test extension.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> is it possible that the call through to getInterface on original notification callbacks
> is throwing?

Yes, this is exactly what happens.  For some reason we can't seem to unwrap the argument to nsIJSIID.  Looking into why.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So we get into xpc_qsUnwrapArgImpl, wrapper is non-null and we have:

(gdb) p wrapper->mIdentity
$12 = (nsJSID *) 0x1106b5e40

But getInterface on XMLHttpRequest is declared to take "IID" and Bindings.conf says that maps to nsIJSIID.  And nsJSID does not implement nsIJSIID; it only implements nsIJSID.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and the XPCOM getInterface is declared as nsIIDRef which is then declared as:

[ref, nsid]   native nsIIDRef(nsIID);

Looking at XPCConvert, looks like XPCConvert::NativeData2JS in the nsXPTType::T_IID case uses xpc_NewIDObject, which calls nsJSID::NewID.  And going the other way, XPCConvert::JSData2Native in the nsXPTType::T_IID case does xpc_JSObjectToID, which basically also goes through nsIJSID.

All of which is to say we should be mapping IID to nsIJSID.  And we seriously need tests for this stuff.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes the attached extension. Tests coming up in a bit

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 671686 [details] [diff] [review]
This fixes the attached extension.  Tests coming up in a bit

Review of attachment 671686 [details] [diff] [review]:
-----------------------------------------------------------------

We were only one letter off!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix getInterface from JS on XMLHttpRequest objects to actually work.

Comment 17

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c317ac04497a

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 740069 
User impact if declined: Some extensions cause various functionality (the
   certificate exception dialog in this case, but other things could break too)
   to break for XMLHttpRequest network loads.
Testing completed (on m-c, etc.): Passes attached unit test, fixes attached
   extension.
Risk to taking this patch (and alternatives if risky): Risk should be low.  The
   test checks that the getInterface that used to work before in 16 still
   continue to work.
String or UUID changes made by this patch: None.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c317ac04497a

Comment 20

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Looking at XPCConvert, looks like XPCConvert::NativeData2JS in the
> nsXPTType::T_IID case uses xpc_NewIDObject, which calls nsJSID::NewID.  And
> going the other way, XPCConvert::JSData2Native in the nsXPTType::T_IID case
> does xpc_JSObjectToID, which basically also goes through nsIJSID.

This is probably because we only have one nsXPTType enum for both CID and IID (and they both get mapped to the native nsID type). We do have nsJSIID and nsJSCID classes too :-/.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> We do have nsJSIID and nsJSCID classes too :-/.

Yes, but both QI to nsIJSID, so this is fine here.

It does mean you can try to getInterface a CID, but that's life.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.

passes tests, low risk, approving for uplift.

Comment 23

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5aaba9acabc5
https://hg.mozilla.org/releases/mozilla-beta/rev/ba644e1f49e5