After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 708847 - Integrate WebKit2 permission requests for TLS errors
Integrate WebKit2 permission requests for TLS errors
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Backend
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Michael Catanzaro
Epiphany Maintainers
: 542454 728300 737589 (view as bug list)
Depends on: 683266
Blocks: 639764 721283 726288
 
 
Reported: 2013-09-26 15:35 UTC by Brian Holt
Modified: 2014-09-29 17:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WIP patch (7.59 KB, patch)
2013-09-26 15:51 UTC, Brian Holt
none Details | Review
Do not ignore TLS errors (1.38 KB, patch)
2014-07-10 17:53 UTC, Michael Catanzaro
reviewed Details | Review
Add custom certificate authentication error page (6.92 KB, patch)
2014-07-10 17:53 UTC, Michael Catanzaro
reviewed Details | Review
TLS error page: Add button to bypass error (16.45 KB, patch)
2014-07-10 17:53 UTC, Michael Catanzaro
none Details | Review
TLS error page: Add button to bypass error (16.40 KB, patch)
2014-07-10 18:02 UTC, Michael Catanzaro
none Details | Review
TLS error page: Add buttons to bypass error or open cert viewer (22.63 KB, patch)
2014-07-10 20:38 UTC, Michael Catanzaro
needs-work Details | Review
Do not ignore TLS errors (23.97 KB, patch)
2014-07-16 21:45 UTC, Michael Catanzaro
none Details | Review
Do not ignore TLS errors (23.00 KB, patch)
2014-07-16 21:55 UTC, Michael Catanzaro
none Details | Review
Do not ignore TLS errors (22.67 KB, patch)
2014-07-16 23:10 UTC, Michael Catanzaro
none Details | Review
Do not ignore TLS errors (22.19 KB, patch)
2014-08-02 20:57 UTC, Michael Catanzaro
none Details | Review
Do not ignore TLS errors (22.16 KB, patch)
2014-08-02 21:06 UTC, Michael Catanzaro
reviewed Details | Review
Add helper functions for printing TLS errors (11.90 KB, patch)
2014-08-06 18:43 UTC, Michael Catanzaro
reviewed Details | Review
Do not ignore TLS errors (22.84 KB, patch)
2014-08-06 18:43 UTC, Michael Catanzaro
reviewed Details | Review
Do not ignore TLS errors (25.02 KB, patch)
2014-08-07 20:27 UTC, Michael Catanzaro
committed Details | Review

Description Brian Holt 2013-09-26 15:35:15 UTC
Integrate the new WK2 API for TLS errors into Ephy.  See https://bugs.webkit.org/show_bug.cgi?id=120160 for details of the implementation.
Comment 1 Brian Holt 2013-09-26 15:51:43 UTC
Created attachment 255846 [details] [review]
WIP patch
Comment 2 Brian Holt 2013-09-26 16:06:34 UTC
Review of attachment 255846 [details] [review]:

This is a work in progress which I wanted to upload while I still have a chance to work on this.

::: embed/ephy-web-view.c
@@ +1352,3 @@
+                                         int response,
+                                         WebKitPermissionRequest *request)
+{

The info_bar widget here is taken from the geolocation request and is likely to be replaced with the form approach similar to delete_web_app.

@@ +1421,3 @@
+  else if (WEBKIT_IS_TLS_PERMISSION_REQUEST (decision))
+  {
+			printf("\n*** [%s:%i] %s ***\n", __FILE__, __LINE__, __FUNCTION__);

Please forgive the printf (test code)
Comment 3 Michael Catanzaro 2013-10-03 14:45:20 UTC
I think we don't want to give the user an option to bypass TLS errors, because he will use it [1], which defeats the purpose of TLS.

I'm not sure if there's any designs for what to actually do.  I imagine displaying a variant of the "Oh no something has gone wrong" page with an evil face instead of a sad face.

[1] http://www.superlectures.com/guadec2013/more-secure-with-less-security
Comment 4 Brian Holt 2013-10-03 15:24:45 UTC
(In reply to comment #3)
> I think we don't want to give the user an option to bypass TLS errors, because
> he will use it [1], which defeats the purpose of TLS.
> 
> I'm not sure if there's any designs for what to actually do.  I imagine
> displaying a variant of the "Oh no something has gone wrong" page with an evil
> face instead of a sad face.
> 
> [1] http://www.superlectures.com/guadec2013/more-secure-with-less-security

Great talk, thanks for the link.  

Have you tried loading a page like https://bugs.gentoo.org in Ephy?  The security certificate is invalid, so the question you have now is what should Ephy do?  

Should it 
(a) fail always (remove any option to bypass security for the user), 
(b) ignore the errors or 
(c) load an error page with a button saying something like "Load anyway" 

I think you'll find that most other browsers do (c), which is what the patch provided does as well.
Comment 5 Michael Catanzaro 2013-10-03 18:17:30 UTC
I'm actually not aware of any browser that does not do (c).  But I also don't think that's an acceptable option for GNOME.  Very few people are going to say "no don't load that site," and they will lose their passwords and credit card numbers to the man in the middle.  By presenting that option, browsers become complicit in the attack. GNOME should aspire to be better than that.  (Of course, (c) is undoubtedly far, far superior to the current behavior, (b), so I do think this patch is very important.)

The "load anyway" button made sense back when self-signed or expired certs were relatively common, but legitimate sites don't (or should not) have those issues anymore.  Regarding bugs.gentoo.org... well I suppose there is an exception to every rule. I don't have an answer to or an opinion on the CAcert dilemma in general, but I believe that websites that use CAcert ought to be intelligent enough not to redirect HTTP requests to HTTPS.  If your website insists on a secure connection, and relies on a root authority that is not generally trusted, then I think Epiphany would be absolutely right to block access to that site.  Everyone visiting it in Chrome and Firefox must be getting the "this site is evil" warning the first time, until they click through it... and the Gentoo developers are OK with that?  Well, we should not be.
Comment 6 Claudio Saavedra 2013-10-04 08:41:24 UTC
I think we need to involve Stef and the design team in this if we want to make a good decision.
Comment 7 Stef Walter 2013-10-04 09:23:41 UTC
I agree with Michael, that GNOME with our focus on security and privacy should aspire to do better that browsers have typically done. 

That said, the browser is the hardest place to get this right, due to the myriad of security use cases that a browser in used for, so I don't think we should feel bad if it takes one or two iterations to get done right.

There is no question that the user sometimes needs to load a site whose certificate is not signed by the traditional pre-installed anchor certificate authorities. 

This breaks down into one of two broad use cases:

1. Non-standard CA use case:
 * A corporate or enterprise site for which a non-standard CA anchor 
   is available. 
 * We should be directing the user to instructions to install that non-standard
   CA, or ask for help from their admin.
 * This covers the majority of legitimate cases where SSL verification fails.
 * This also covers the CACert use case.

2. Self-signed certificate, on a security sensitive site
 * An admin has configured his site with an SSL self-signed certificate,
   and made the site available to others.
 * This is an advanced use case, and does not need to be made super simple
   for users.
 * This could be analogous to using a non-standard port for HTTP. The admin of
   such a site needs to provide its users with the somewhat complex information
   to make this work.

In the first use case, for enterprise use cases, the ideal solution would have been to have an administrator aware of Linux/GNOME and have preinstalled the necessary certificate anchor.

But obviously if the user got to this point, then the above didn't apply or happen.

So in both cases we want the user to be able to solve the problem (ideally with help from the admin). But what we don't want is a prompt-ignore-and-continue situation.

In my opinion, for use case 1 we want to build a UI in seahorse for installing a CA anchor, and link people to a document which helps them accomplish that, once and for all. This is really a much better approach for Enterpise or CACert users as they never see another prompt for another host using a similarly signed certificate.

For use case 2 I think we want some UI in epiphany, perhaps a bookmark UI, which lets the user create a bookmark to a site, and associate the expected certificate with that bookmark. When clicking on such a bookmark epiphany would expect to validate the host using that certificate, and only that certificate.

This allows advanced users to have the additional benefit of being discovering when the certificate changes.

I gave a talk about this at GUADEC, but as noted above I'm completely open to iterations on designs to solve these use cases. What I really do want to avoid is interrupting the user to prompt them about a security question.

http://www.superlectures.com/guadec2013/more-secure-with-less-security

What do you think?
Comment 8 Brian Holt 2013-10-04 13:27:19 UTC
As a relative newcomer to GNOME I don't have a strong opinion on this topic, except to say that I would be quite frustrated with a browser that didn't allow me to load a page even if I knew that the risk was minimal (e.g. https://bugs.gentoo.org).
Comment 9 Carlos Garcia Campos 2013-10-04 13:44:04 UTC
(In reply to comment #8)
> As a relative newcomer to GNOME I don't have a strong opinion on this topic,
> except to say that I would be quite frustrated with a browser that didn't allow
> me to load a page even if I knew that the risk was minimal (e.g.
> https://bugs.gentoo.org).

I agree, it's sad but it's quite common to find web sites with invalid certificates that I really want/need to access (under my own responsibility, of course) I agree we should not load those pages by default like we currently do.
Comment 10 Brian Holt 2013-10-04 16:04:10 UTC
http://www2013.wwwconference.org/proceedings/p59.pdf might be interesting reading relevant to this topic...
Comment 11 Michael Catanzaro 2013-10-04 22:58:24 UTC
I'm having trouble imagining how UI for use case 2 would be integrated into Epiphany. On the bright side, anything at all would be better than the current behavior (ignore the error).

Brian, that paper is definitely interesting and relevant.  My TL;DR book report: (1) it concludes that "unknown issuer" errors like bugs.gentoo.org are high-risk situations, and unfortunately represent the majority of TLS errors on the web.  (2) The paper recommends some changes to certificate validation to ignore common, low-risk errors, like www.example.com != example.com, *.*.example.com != *.example.com, etc. These recommendations seem relevant to GnuTLS. (3) It presents incomplete certificate chains as a serious issue, and shows that caching intermediate certificates significantly reduces TLS errors. It claims all major browsers do this, but Epiphany does not.  It also recommends supporting AIA during certificate validation.
Comment 12 Stef Walter 2013-10-07 09:46:22 UTC
(In reply to comment #8)
> As a relative newcomer to GNOME I don't have a strong opinion on this topic,
> except to say that I would be quite frustrated with a browser that didn't allow
> me to load a page even if I knew that the risk was minimal (e.g.
> https://bugs.gentoo.org).

First of all, bugs.gentoo.org uses CACert. Just install it using your distro's package (most have one) or using '$ trust anchor /path/to/cacert.crt'. This is "case 1" above. To stop ignoring security, and install the CA authority.

If your distro does not yet have p11-kit integrated (ie: the 'trust' tool above, then that's the way forward).

But in a general case, where actual self-signed certificates are in use (ie: not bugs.gentoo.org) it should be possible to allow this, but not just with a "Load Anyway" button. The reason goverments and hackers can work around encryption on the internet is not (usually) because of broken protocols, but because of broken surrounding implementation issues: such as "Load Anyway" buttons, which cause users to ignore security *precisely* when they're being targeted.
Comment 13 Stef Walter 2013-10-07 09:49:42 UTC
(In reply to comment #11)
> I'm having trouble imagining how UI for use case 2 would be integrated into
> Epiphany. 

For 'case 2' What I would suggest is the following. In the bookmark 'Properties' and 'Add bookmark' dialogs, have an "Advanced Security" button, which provides the functionality to pin a certificate (always expect that certificate) to that bookmark.

Would you like me to mock-up a design?
Comment 14 Brian Holt 2013-10-07 09:59:45 UTC
(In reply to comment #13)
> (In reply to comment #11)
> > I'm having trouble imagining how UI for use case 2 would be integrated into
> > Epiphany. 
> 
> For 'case 2' What I would suggest is the following. In the bookmark
> 'Properties' and 'Add bookmark' dialogs, have an "Advanced Security" button,
> which provides the functionality to pin a certificate (always expect that
> certificate) to that bookmark.
> 
> Would you like me to mock-up a design?

Sounds good to me.

FYI, after a long discussion we at WebKitGtk+ have decided not to use the permission-request model for tls errors, and instead to emit a signal load-failed-with-tls-errors with a WebKitCertificateInfo struct containing the cert, flags and host when the policy is WEBKIT_TLS_ERRORS_POLICY_ASK.  When its in and reviewed I hope to help to integrate it here.
Comment 15 Michael Catanzaro 2013-10-07 16:29:06 UTC
Somewhat related: when a cert check fails, it would be really nice to know more about why; currently in Epiphany you see a simple reason like "The signing certificate authority is not known," but that's not much much compared to e.g. Rekonq which shows you information about every cert in the chain so you can see exactly where the chain of trust is broken.  I know we don't want to make Epiphany UI too complicated, but the certificate info viewer is of limited usefulness currently.
Comment 16 Michael Catanzaro 2013-12-31 17:45:28 UTC
(In reply to comment #14)
> FYI, after a long discussion we at WebKitGtk+ have decided not to use the
> permission-request model for tls errors, and instead to emit a signal
> load-failed-with-tls-errors with a WebKitCertificateInfo struct containing the
> cert, flags and host when the policy is WEBKIT_TLS_ERRORS_POLICY_ASK.  When its
> in and reviewed I hope to help to integrate it here.

Hey Brian, thanks for your work on this. Any recent progress? This would be great to have before 3.12, either with or without the Load Anyway button.
Comment 17 Brian Holt 2014-01-02 15:03:08 UTC
(In reply to comment #16)

> Hey Brian, thanks for your work on this. Any recent progress? This would be
> great to have before 3.12, either with or without the Load Anyway button.

The API has landed in WebKitGTK, so its just a question of what policy should be implemented.

Reading through all the comments above again, I'm left with the sense that policy should be to fail on TLS errors webkit_web_context_set_tls_errors_policy(context, WEBKIT_TLS_ERRORS_POLICY_FAIL)
, unless there is a known certificate for the site (e.g. stored in the bookmarks) in which case we add the exception with webkit_web_context_allow_tls_certificate_for_host() before loading the page.

If we did this the user will never be prompted to "Load anyway" but has a clear way to allow a site to load with a given certificate if needed, which I think addresses both of Stef's use cases.

I don't have much capacity to work on this given downstream requirements, so please feel free to continue where my WIP patch left off.  Did anything come of the UI mockup for the bookmarks?
Comment 18 Michael Catanzaro 2014-04-15 23:53:08 UTC
*** Bug 728300 has been marked as a duplicate of this bug. ***
Comment 19 Michael Catanzaro 2014-07-07 00:51:45 UTC
*** Bug 542454 has been marked as a duplicate of this bug. ***
Comment 20 Yosef Or Boczko 2014-07-07 21:27:29 UTC
Michael, I just looked now one the code in the tls-errors branch, and I have one
comment (before I run the code):
Remove markup in translatable messages, as on of the GNOME Goals:
https://wiki.gnome.org/Initiatives/GnomeGoals/RemoveMarkupInMessages
Comment 21 Michael Catanzaro 2014-07-08 14:39:15 UTC
Fixed that, thanks Yosef
Comment 22 Yosef Or Boczko 2014-07-08 14:44:48 UTC
(In reply to comment #21)
> Fixed that, thanks Yosef

Thanks!
Comment 23 Michael Catanzaro 2014-07-10 17:53:40 UTC
Created attachment 280439 [details] [review]
Do not ignore TLS errors

Currently, Epiphany loads web pages even though it realizes the
connection may be insecure, displaying a broken lock in the address bar.
By this point, it's too late: the attacker already has your session
cookies. Display an error page instead. Based on groundwork by Brian
Holt.

This is the minimal reasonable implementation. We might also want:

* A user-friendly description
* A button to open the certificate viewer
* A mechanism by which to add the certificate to the trust database
Comment 24 Michael Catanzaro 2014-07-10 17:53:44 UTC
Created attachment 280440 [details] [review]
Add custom certificate authentication error page

Explain to the user why loading an untrusted page might be dangerous.

It might be desirable to additionally explain the particular type of
failure (lack of trust, mismatched URI, expired or revoked cert, etc.)

This patch is based on groundwork by Brian Holt.
Comment 25 Michael Catanzaro 2014-07-10 17:53:48 UTC
Created attachment 280441 [details] [review]
TLS error page: Add button to bypass error

If the user experiences a TLS error, he needs some way to override it.
We can either give the user a button that allows him to click straight
through to the website, like Chromium does, or send him to a new dialog
designed to explain the implications of adding a security exception,
like Firefox does. Another possibility is to disallow exceptions
altogether and require the user to install the certificate. See bug
708847 for discussion.

We're going to take the Chromium approach for now. This can be revisited
in the future.
Comment 26 Michael Catanzaro 2014-07-10 18:02:03 UTC
Created attachment 280442 [details] [review]
TLS error page: Add button to bypass error

Fix formatting errors
Comment 27 Michael Catanzaro 2014-07-10 20:38:39 UTC
Created attachment 280452 [details] [review]
TLS error page: Add buttons to bypass error or open cert viewer

If the user experiences a TLS error, he needs some way to override it.
We can either give the user a button that allows him to click straight
through to the website, like Chromium does, or send him to a new dialog
designed to explain the implications of adding a security exception,
like Firefox does. Another possibility is to disallow exceptions
altogether and require the user to install the certificate. See bug
708847 for discussion.

We're going to take the Chromium approach for now. This can be revisited
in the future.

Also, add the ability to open the cert viewer to get technical details
on why the cert did not validate.
Comment 28 Carlos Garcia Campos 2014-07-11 07:36:43 UTC
Review of attachment 280439 [details] [review]:

This should probably squashed in the next commit that adds the error page.
Comment 29 Carlos Garcia Campos 2014-07-11 07:39:26 UTC
Review of attachment 280440 [details] [review]:

::: embed/ephy-web-view.c
@@ +1869,3 @@
 
+static gboolean
+tls_error_cb (WebKitWebView *web_view,

I would call this load_failed_with_tls_error_cb

@@ +1933,3 @@
+  uri = webkit_web_view_get_uri (web_view);
+  webkit_web_view_load_alternate_html (WEBKIT_WEB_VIEW (view), html->str, uri, 0);
+  g_string_free (html, TRUE);

There's too much code duplicated here. I think we could somehoe add a new kind of error page for TLS errors and adapt ephy_web_view_load_error_page if needed to support all kind of error pages.
Comment 30 Carlos Garcia Campos 2014-07-11 08:29:20 UTC
Review of attachment 280452 [details] [review]:

Instead of adding another button to open the cert viewer, we could set the security level also when loading the TLS error page, so the you can open the cert dialog from the URL icon. I think it's more consistent and the code is a lot simpler. We can add further technical details in an expander like chromium does.

::: embed/ephy-embed-shell.c
@@ +283,3 @@
+                                     EphyEmbedShell *shell)
+{
+  g_signal_emit (shell, signals[ALLOW_TLS_CERTIFICATE], 0);

This should be emitted by EphyWebExtensionProxy, here you get the page_id from the signal args, and looks for the web extension calling ephy_embed_shell_find_web_extension. See how web_extension_form_auth_save_requested works.

@@ +866,3 @@
+                  0, NULL, NULL,
+                  g_cclosure_marshal_VOID__VOID,
+                  G_TYPE_NONE, 0);

This signal should be in EphyWebExtensionProxy.

::: embed/ephy-web-view.c
@@ +111,3 @@
+  gboolean loading_tls_error_page;
+  /* Valid only if we are currently viewing the TLS error page */
+  WebKitCertificateInfo *tls_error_page_info;

I would call this certificate_info to make it clear this is the webkit certificate info we save only to be able to add an exception for it. I wonder if it would be simpler if we add webkit_certificate_info_new() to webkit that receives certificate and tls_errors, and we could simply reuse certificate and tls_errors members. Then we could simply create a new WebKitCertificate right before calling webkit_web_context_allow_tls_certificate_for_host().

@@ +770,3 @@
+  } else {
+    g_warn_if_fail (priv->tls_error_page_host == NULL);
+  }

Here we can simply g_clear_pointer(&priv->tls_error_page_info, webkit_certificate_info_free);

@@ +773,3 @@
+
+  if (priv->tls_error_page_host != NULL)
+    g_free (priv->tls_error_page_host);

g_free is null safe and finalize is called once, so you can simply call g_free here.

@@ +1629,3 @@
+        priv->tls_error_page_host = NULL;
+      }
+    }

And we can simply all this too, by reusing the certificate and tls_errors and setting the security level also for the tls errors page. We could do something like:

    /* Security status. */
    g_clear_object (&priv->certificate);
    priv->tls_errors = 0;
    if (priv->loading_tls_error_page) {
      priv->loading_tls_error_page = FALSE;
      priv->tls_errors = webkit_certificate_info_get_tls_errors (priv->tls_error_page_info);
      priv->certificate = g_object_ref (webkit_certificate_info_get_tls_certificate (priv->tls_error_page_info));
    } else if (webkit_web_view_get_tls_info (web_view, &priv->certificate, &priv->tls_errors)) {
      g_object_ref (priv->certificate);
    }

    security_level = priv->tls_errors == 0 ?
      EPHY_WEB_VIEW_STATE_IS_SECURE_HIGH : EPHY_WEB_VIEW_STATE_IS_BROKEN;

    ephy_web_view_set_security_level (EPHY_WEB_VIEW (web_view), security_level);

If we could reuse certificate and tls_errors instead of saving the WebKitCertificateInfo, this would be even simpler

@@ +1925,3 @@
 
+  if (priv->tls_error_page_info != NULL)
+    webkit_certificate_info_free (priv->tls_error_page_info);

g_clear_pointer.

@@ +1927,3 @@
+    webkit_certificate_info_free (priv->tls_error_page_info);
+  if (priv->tls_error_page_host != NULL)
+    g_free (priv->tls_error_page_host);

g_clear_pointer

@@ +1929,3 @@
+    g_free (priv->tls_error_page_host);
+  priv->loading_tls_error_page = TRUE;
+  priv->tls_error_page_info = webkit_certificate_info_copy (info);

If we could reuse certificate and tls_errors instead of saving the WebKitCertificateInfo, we would save certificate and tls_errors here diredctly.

@@ +2951,3 @@
+ **/
+void
+ephy_web_view_allow_certificate_exception (EphyWebView *view)

Instead of a public method, this should be the callback of the allow-tls-certificate signal emitted by EphyWebPageProxy. The signal should receive the page_id, so you can check this is actually for this particular web view (in single web process model, the same extension proxy si shared by all web views).

::: embed/web-extension/ephy-web-extension.c
@@ +1269,3 @@
+                                 EPHY_WEB_EXTENSION_INTERFACE,
+                                 "AllowTLSCertificate",
+                                 NULL,

We should pass the page id as parameter

@@ +1340,3 @@
+
+  class = JSClassCreate (&class_def);
+  class_object = JSObjectMake (context, class, extension);

Instead of passing only the extension as private data of the object, we could pass a custom struct containing the extention and the page id, and use that page id as parameter of the signal. Then you would also need to define JSClassDefinition::finalize to free the private struct

::: src/ephy-window.c
@@ +3088,2 @@
 static void
+allow_tls_certificate_cb (EphyEmbedShell *embed_shell,

This could be done by the web view internally if the signal is emitted for a particular page
Comment 31 Carlos Garcia Campos 2014-07-11 08:59:44 UTC
Review of attachment 280452 [details] [review]:

::: embed/web-extension/ephy-web-extension.c
@@ +1377,3 @@
+
+  /* EphyWebView ensures this JS can only take effect when used from our TLS error page. */
+  prepare_certificate_exception_js (world, web_page, frame, extension);

I'm thinking that it would be much better if we detect here if this is the TLS error page or not, to avoid injecting this custom JS for every page we load. We might use DOM API for this, a very simple approach could be to check the document url and title. If webkit_web_page_get_uri is not about:blank and webkit_dom_document_get_url is about:blank, is becase we are likely loading alternate content. Then we could check the title, or we can add a page identifier somewhere in the html, for example in the body, and here we get the body with webkit_dom_document_get_body() and then check its ID. There might be better ways, though.
Comment 32 Carlos Garcia Campos 2014-07-11 10:27:03 UTC
See https://bugs.webkit.org/show_bug.cgi?id=134830
Comment 33 Michael Catanzaro 2014-07-16 21:45:45 UTC
Created attachment 280889 [details] [review]
Do not ignore TLS errors

Thanks for the detailed review!

This patch handles the web view security status on the error page a bit differently than you suggested, leaving it unchanged from the failed load.

The trick with comparing webkit_web_page_get_uri() with webkit_dom_document_get_url() to decide whether to create the custom js worked great, but calling webkit_dom_document_get_url() inside window_object_cleared_cb() returns NULL (why?) and checking the title would not work well because the second half of the title varies based on the page we're visiting, and the first half is translated, so I currently only have the first check in there. That's probably enough to restrict its use to Epiphany's own error pages, though, right?
Comment 34 Michael Catanzaro 2014-07-16 21:50:44 UTC
(In reply to comment #33)
> but calling webkit_dom_document_get_url() inside
> window_object_cleared_cb() returns NULL (why?) 

I meant webkit_dom_document_get_body()
Comment 35 Michael Catanzaro 2014-07-16 21:55:27 UTC
Created attachment 280891 [details] [review]
Do not ignore TLS errors
Comment 36 Michael Catanzaro 2014-07-16 23:10:19 UTC
Created attachment 280893 [details] [review]
Do not ignore TLS errors
Comment 37 Michael Catanzaro 2014-08-02 20:57:10 UTC
Created attachment 282351 [details] [review]
Do not ignore TLS errors

Updated for WebKitCertificateInfo API removal
Comment 38 Michael Catanzaro 2014-08-02 21:06:39 UTC
Created attachment 282352 [details] [review]
Do not ignore TLS errors

Fix leak in previous patch (oops)
Comment 39 Carlos Garcia Campos 2014-08-05 13:18:29 UTC
Review of attachment 282352 [details] [review]:

Ok, this looks much better, I have only a few more comments

::: embed/ephy-web-view.c
@@ +1622,3 @@
+      }
+
+      ephy_web_view_set_security_level (EPHY_WEB_VIEW (web_view), security_level);

So, we don't set the security_level for the tls error page? As I said I think we should set the security level so that you can check the certificate from the url bar icon. At this point we should already have the certificate and errors saved.

@@ +1747,3 @@
+                         /* Good advice from Firefox; displays when a site's TLS certificate is invalid. */
+                         _("Legitimate banks, stores, and other public sites will "
+                           "not ask you to do this."));

Is all this information true/accurate for all possible certificate errors? I think we should probably show a different error message depending on the error.

@@ +1854,3 @@
+
+  if (load_anyway_js == NULL)
+    load_anyway_js = g_strconcat ("window.location = '", uri, "';", NULL);

maybe g_strdup_printf ("window.location='%s';", uri); is easier to read than the concat

@@ +1963,3 @@
+  priv->tls_errors = errors;
+  priv->tls_error_page_host = g_strdup (host);
+  ephy_web_view_set_security_level (EPHY_WEB_VIEW (web_view), EPHY_WEB_VIEW_STATE_IS_BROKEN);

Ah, you are doing it here, ok.

::: embed/web-extension/ephy-web-extension.c
@@ +1252,3 @@
+  EphyWebExtension *extension;
+  guint64 page_id;
+} allow_tls_certificate_cb_data_t;

allow_tls_certificate_cb_data_t -> AllowTLSCertificateData

@@ +1297,3 @@
+tls_certificate_error_page_finalize (JSObjectRef object)
+{
+  g_free (JSObjectGetPrivate (object));

Use g_slice_free

@@ +1312,3 @@
+  JSObjectRef class_object;
+  JSStringRef str;
+  allow_tls_certificate_cb_data_t *cb_data;

cb_data -> data 
This is private data of the class, not only for the allow TLS method.

@@ +1322,3 @@
+  class_def.finalize = tls_certificate_error_page_finalize;
+
+  cb_data = g_new (allow_tls_certificate_cb_data_t, 1);

And g_slice_new here

@@ +1371,3 @@
+
+  if (g_strcmp0 (webkit_web_page_get_uri (web_page), "about:blank") != 0 &&
+      g_strcmp0 (dom_url, "about:blank") == 0) {

We should probably add a comment here explaining this hack :-)
Comment 40 Michael Catanzaro 2014-08-05 15:43:57 UTC
Is there some rule for when to use g_slice_alloc instead of g_new?

> @@ +1747,3 @@
> +                         /* Good advice from Firefox; displays when a site's
> TLS certificate is invalid. */
> +                         _("Legitimate banks, stores, and other public sites
> will "
> +                           "not ask you to do this."));
> 
> Is all this information true/accurate for all possible certificate errors? I
> think we should probably show a different error message depending on the error.

We should definitely display at least a generic reason why the cert was blocked ("The site's identification has expired" or "The site's identification is only valid for a different site"); I just forgot to do this. :(

At least the certificate dialog should provide a more detailed explanation (e.g. "This certificate expired on May 1, 2014") but I don't think the main error page should need to mention what a certificate is, since that's a confusing technical detail. 

Anyway, I picked this text since it's what Firefox uses to get the point across that continuing is dangerous. It's usually true but there are exceptions: if (a) the site is misconfigured (nowadays only common if a site is offered via HTTPS but not by default, so the user had to manually type https into the address bar) or (b) the site is not public, or not intended for use without prior configuration (see comment #7).

Practically, there are a couple types of misconfigurations that are less likely to represent security threats:

* The site sends an illegal unordered certificate chain.  All major browsers reorder these chains, but we don't. Major problem, bug #683266.
* If a cert has expired within the past week, probably the site admin has just not updated it yet by mistake. All major browsers block these sites, so it's OK that we do too.
* If a cert is for a subdomain of the current site, e.g. you are on example.com but the cert presented is for www.example.com or *.example.com.  Or if you are visiting one.two.example.com but the cert is for *.example.com instead of *.*.example.com. All major browsers block these subdomains, so it's OK that we do too, but I'm really unconvinced that it's the right thing to do.

Anything besides those two errors is probably either (a) an attacker, or else (b) a chain that's hopelessly broken and the site admin just never noticed because the missing cert is often cached by users' browsers. Most major browsers cache intermediate certs when they're validated, so an intermediate cert effectively becomes a CA root. It'd be independently valuable for us to cache intermediate certificates -- I think glib-networking's WIP NSS backend addresses this issue -- but this is just a fallback for broken sites.

I'm pretty sure we need to fail expired certs, since allowing certs one week past expiration accomplishes nothing besides moving back the effective expiration date a bit. I'm imagining some sort of future expiration date war between browsers, each browser trying to allow certs just a bit longer than its competitors to ensure the certs fail in some other browser first.  Epiphany might be small enough to get away with it, but I think it'd be bad to change this any lower in the stack.

I think allowing certs that are valid for a parent domain but not the current subdomain would be safe, since it's not typical that users would actually want to protect information on subdomain.example.com from example.com. I don't think it's a priority compared to some of our other bugs I'd rather work. Another reason to hesitate is that I'm not sure it's a great idea for our behavior to differ from every major browser (conceivably some sites are actually structured so that subdomains want to protect information from the parent domain; still, this seems unlikely). But if other browsers begin to allow such certs in the future, then it would definitely be a priority for us to do so as well.
Comment 41 Michael Catanzaro 2014-08-06 18:43:19 UTC
Created attachment 282729 [details] [review]
Add helper functions for printing TLS errors

Previously this was only needed by the certificate dialog. Now we need
these error messages to be available on the TLS errors page as well.
Comment 42 Michael Catanzaro 2014-08-06 18:43:32 UTC
Created attachment 282730 [details] [review]
Do not ignore TLS errors

Currently, Epiphany loads web pages even though it realizes the
connection may be insecure, displaying a broken lock in the address bar.
By this point, it's too late: the attacker already has your session
cookies. Display an error page instead. Based on groundwork by Brian
Holt.
Comment 43 Carlos Garcia Campos 2014-08-07 11:11:07 UTC
Review of attachment 282729 [details] [review]:

I'm not sure about this, because in the end this is not actually shared, the dialog uses one method and the error page the other.
Comment 44 Carlos Garcia Campos 2014-08-07 11:16:16 UTC
Review of attachment 282730 [details] [review]:

This looks great, there's only one detail.

::: embed/ephy-web-view.c
@@ +1736,3 @@
+
+  bold_hostname = g_strconcat ("<strong>", hostname, "</strong>", NULL);
+  details = ephy_certificate_get_simple_error_messages_from_tls_errors (priv->tls_errors);

Instead of a text string with the bullets, I think we could use actual HTML here, if we get the array of errors we could add a list in HTML. We could add a function that returns the HTML directly from the GTlsErrorFlags, and we don't need the previous patch at all.
Comment 45 Michael Catanzaro 2014-08-07 20:27:41 UTC
Created attachment 282844 [details] [review]
Do not ignore TLS errors

Currently, Epiphany loads web pages even though it realizes the
connection may be insecure, displaying a broken lock in the address bar.
By this point, it's too late: the attacker already has your session
cookies. Display an error page instead. Based on groundwork by Brian
Holt.
Comment 46 Michael Catanzaro 2014-08-07 20:34:12 UTC
Using HTML does make more sense. Is this what you had in mind?

Also:

(In reply to comment #40)
> Is there some rule for when to use g_slice_alloc instead of g_new?
Comment 47 Carlos Garcia Campos 2014-08-08 10:57:32 UTC
Review of attachment 282844 [details] [review]:

Yes!
Comment 48 Carlos Garcia Campos 2014-08-08 10:59:26 UTC
(In reply to comment #46)
> Using HTML does make more sense. Is this what you had in mind?
> 
> Also:
> 
> (In reply to comment #40)
> > Is there some rule for when to use g_slice_alloc instead of g_new?

slice allocator is normally used when you allocate small structs and more than one, like in a linked list, but I think it's always safe to use it anyway.
Comment 49 Michael Catanzaro 2014-08-08 14:43:44 UTC
Attachment 282844 [details] pushed as f0e7ab8 - Do not ignore TLS errors
Comment 50 Michael Catanzaro 2014-09-29 17:11:03 UTC
*** Bug 737589 has been marked as a duplicate of this bug. ***