Last Comment Bug 800386 - Self signed certificates cannot be whitelisted with FoxyProxy Plus extension
: Self signed certificates cannot be whitelisted with FoxyProxy Plus extension
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: 14 Branch
: x86 All
: -- normal (vote)
: mozilla19
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 740069
  Show dependency treegraph
 
Reported: 2012-10-11 08:17 PDT by Georg Koppen
Modified: 2012-10-17 18:07 PDT (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
test.xpi (3.55 KB, application/octet-stream)
2012-10-11 08:17 PDT, Georg Koppen
no flags Details
This fixes the attached extension. Tests coming up in a bit (2.51 KB, patch)
2012-10-15 18:29 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
khuey: review+
Details | Diff | Review
Fix getInterface from JS on XMLHttpRequest objects to actually work. (5.46 KB, patch)
2012-10-15 19:42 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
khuey: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Georg Koppen 2012-10-11 08:17:13 PDT
Created attachment 670401 [details]
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 2012-10-11 12:30:13 PDT
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] 2012-10-12 19:28:48 PDT
The first bad revision is:
changeset:   90729:1bdb337e3136
user:        Peter Van der Beken <peterv@propagandism.org>
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 [:bz] (Out June 25-July 6) 2012-10-13 09:05:13 PDT
Well, what's foxyproxy doing to mess with that XHR?  ;)

Any exceptions in error console?
Comment 4 Georg Koppen 2012-10-13 11:19:04 PDT
(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 [:bz] (Out June 25-July 6) 2012-10-13 17:50:21 PDT
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 H. Jung 2012-10-14 09:06:50 PDT
(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 [:bz] (Out June 25-July 6) 2012-10-14 14:29:48 PDT
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 2012-10-15 13:35:01 PDT
(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 [:bz] (Out June 25-July 6) 2012-10-15 17:48:50 PDT
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 [:bz] (Out June 25-July 6) 2012-10-15 17:56:13 PDT
Anyway, trying it now with the attached test extension.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-15 18:07:04 PDT
> 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 [:bz] (Out June 25-July 6) 2012-10-15 18:13:36 PDT
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 [:bz] (Out June 25-July 6) 2012-10-15 18:21:32 PDT
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 [:bz] (Out June 25-July 6) 2012-10-15 18:29:16 PDT
Created attachment 671686 [details] [diff] [review]
This fixes the attached extension.  Tests coming up in a bit
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-15 18:30:53 PDT
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 [:bz] (Out June 25-July 6) 2012-10-15 19:42:51 PDT
Created attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-15 20:13:12 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c317ac04497a
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-15 20:15:09 PDT
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] 2012-10-16 01:17:36 PDT
https://hg.mozilla.org/mozilla-central/rev/c317ac04497a
Comment 20 Peter Van der Beken [:peterv] 2012-10-16 01:56:16 PDT
(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 [:bz] (Out June 25-July 6) 2012-10-16 07:58:07 PDT
> 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 2012-10-17 11:15:21 PDT
Comment on attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.

passes tests, low risk, approving for uplift.

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