618919 - Non-mobile 'Bad cert' dialog is shown for XHR

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

This happens a lot in the Mozilla MV office on the "Mozilla Guest" wifi. Any background XHR will cause a desktop dialog for the 'bad cert' dialog.

We should override the dialog for mobile. We do not need to show cert or anything like that.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip

Must be something wonky happening on creation of the component - I am segfaulting

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like bug 531303 is needed for letting my JS component get safely created.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Here is the place in NSS that uses the component:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp#1575

Comment 5

[Doug Turner (:dougt)]

kai, can we just create a sync xpcom proxy here?

Comment 6

[Kai Engert (:KaiE:)]

(In reply to comment #5)
> kai, can we just create a sync xpcom proxy here?

The call to "do_GetService(contract_id)" crashes. I don't know of a way to proxy this.

I believe what you propose is:
  "hello main thread, please create this service by contract id, 
   on the main thread, then return me a sync proxy 
   object to that service". 

When I asked on IRC "Do we already have this ability?", bsmedberg didn't like this idea and said: 
  "oh please oh please, don't use XPCOM proxies"

He continued to say that you shouldn't use sync-UI shouldn't be used at all.

Unfortunately, we already have such blocking UI... (so, getting rid of it would be another bug and another story).


IMHO, if you're looking for a reasonable solution, my proposal is:

=> Implement your override using a threadsafe C++ class.

(just as it's being done by PSM, see nsNSSDialogs::ShowCertError() )

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Kai - Thanks for offering some ideas. After thinking about this, I had an alternate approach. On mobile, we really don't need a full blown cert dialog. Technically, we don't want a dialog at all.

If I understand the way things work, we load the cert error in the actual web content, if error pages are enabled, which they are in Fennec.

The only time we would see this dialog is if some XHR and non-content channel tried to access a website in the background and triggered a cert error. In those cases, I'd rather be completely silent and just fail.

Is there a simple way to do that?

If there is not a simple way, we could add a mobile-only code block to either:
* skip showing the dialog at all
* just fire an observer notification (if that is even thread safe)

if we use an observer notification, Fennec could just listen for the notification and show a "Sorry, something bad happened in the background and you can't really fix it any way" dialog.

Obviously, I favor the "just silently fail" approach for background request cert failures.

What do you think?

Comment 8

[Kai Engert (:KaiE:)]

(In reply to comment #7)
> Kai - Thanks for offering some ideas. After thinking about this, I had an
> alternate approach. On mobile, we really don't need a full blown cert dialog.
> Technically, we don't want a dialog at all.
> 
> If I understand the way things work, we load the cert error in the actual web
> content, if error pages are enabled, which they are in Fennec.
> 
> The only time we would see this dialog is if some XHR and non-content channel
> tried to access a website in the background and triggered a cert error. In
> those cases, I'd rather be completely silent and just fail.
> 
> Is there a simple way to do that?


The simple way is to provide your own implementation, just as you attempted to do with your patch.

The only difference is that you must avoid using a JS implementation, and rather use a C++ implementation.


> If there is not a simple way,

Do you think the C++ code approach is "not simple"?
I think it should be straightforward to do that.

You only need a minimal implementation that provides one function and does nothing.


> we could add a mobile-only code block to either:
> * skip showing the dialog at all
> * just fire an observer notification (if that is even thread safe)


I think we should not start to patch PSM with a #ifdef mobile.
We already have a callback mechanism (look for implementation) that has been added exactly for this purpose, let the app decide what it wants to display (or not display).


> if we use an observer notification, Fennec could just listen for the
> notification and show a "Sorry, something bad happened in the background and
> you can't really fix it any way" dialog.
> 
> Obviously, I favor the "just silently fail" approach for background request
> cert failures.

The "fail silently" is fine with me.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

I can try to make a C++ threadsafe impl.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch adds a barebones nsISSLCertDialog impl, stubbed out to do nothing. We are OK with silently handling background SSL cert errors.

The patch needed to add a real module file and refactor the Makefile and nsPhoneSupport file a bit.

I tested this on desktop and it works fine. In fact, the test code is still in the patch - I will remove. Without the patch, I see a SSL Cert Error dialog. With the patch I see nothing.

I am going to test on Andriod next and will report findings. Patch is up for 1st pass review.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

oops, missed the new files

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Added missing new files and updated the package manifest

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

OK, the patch builds fine on Android, but it is not packaged. Micheal told me that we don't currently support packaging binary components from mobile-browser into the Android omnijar.

He said it could be done, but would affect performance. So I am looking for ideas now. I can't use a JS component (comment 2). The C++ component works, but isn't packaged. Kai wasn't thrilled with changing the NSS code, but another idea is to add a preference to disable the current XUL UI and check it here:
http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/src/nsNSSDialogs.cpp#654

If the preference is set, we would send an observer notification instead of displaying the dialog box.

Thoughts?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2 (m-c)

This patch takes a different approach. It uses a preference to decide whether to show the error in a XUL dialog (which Mobile does not want) or send the error information using the observer service, and the application can handle the cert error in whatever way it wants to.

Kai - I don't know if you have a convention for observer notifications in security/ or not. Also, I took a crack at a good pref name that fit into the existing security.* prefs.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

To be clear, the C++ component approach has the problem of not being supported currently on Android because it leads to a Ts regression when loading small, individual binary libraries.

We could move the C++ component into mozilla-central - but why? It's dead weight only adding to our code size for no good reason.

The preference approach uses the C++ component already in mozilla-central. The only difference is we add a way to control showing the dialog - which I think should be there already. This is the approach that doesn't regress Ts and minimizes the affect on codesize.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: best of both worlds patch

This patch builds a C++ component to silence the SSL Cert Dialog and builds it into libxul. We do not need to package the library separately. We do need to keep packaging the XPT file, but that is taken care of already.

Tested by forcing an XHR load using a known bad cert site with debugging output in the SSL Cert stub. Everything worked as desired: The SSL Cert override was being used and the SSL Cert dialog was not shown.

This patch uses the "standard" approach for building app components into libxul

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 515129 [details] [diff] [review]
best of both worlds patch


>+# Needed for building our components as part of libxul
>+APP_LIBXUL_DIRS += mobile/components/phone

Lets rename this directory now, or soon..  it has nothing to do with |phone| support now, and you are already renaming the module.


>+++ b/components/phone/nsBrowserModule.cpp
>@@ -0,0 +1,78 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/*
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mobile Browser SSL Cert Dialog Override.


not to be totally nit picky, but this file does more than provide an override for ssl certs.  How about just use the same description as what you did in the .h.

>+
>+#include "nsSSLCertErrorDialog.h"
>+
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsSSLCertErrorDialog, nsISSLCertErrorDialog);
>+
>+NS_IMETHODIMP
>+nsSSLCertErrorDialog::ShowCertError(nsIInterfaceRequestor *ctx, 
>+                                    nsISSLStatus *status, 
>+                                    nsIX509Cert *cert, 
>+                                    const nsAString & textErrorMessage, 
>+                                    const nsAString & htmlErrorMessage, 
>+                                    const nsACString & hostName, 
>+                                    PRUint32 portNumber)
>+{
>+  return NS_OK;
>+}

Please add a comment in this method that says something like:

"""
Returning NS_OK here does not mean that this connection will continue successful.  The connection has been or will be aborted regardless of what we do here.
"""

The reason for this, is that people that look at this code might false assume that we are ignoring security and/or have a hole.  This has happened in this past.


r+.  please fix up the nits, and at least file a bug to (or promise me that you will) rename this 'phone' directory.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with all changes requested:
http://hg.mozilla.org/mobile-browser/rev/bb6d2f3e295a

Comment 19

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre Fennec/4.0b6pre