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 778253 - Portal-helper fails to access captive portals with bogus certs
Portal-helper fails to access captive portals with bogus certs
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: portal-helper
3.22.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 749197
Blocks:
 
 
Reported: 2017-02-06 16:32 UTC by Jiri Eischmann
Modified: 2017-02-15 13:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
portalHelper: Don't fail to load because of TLS errors (2.62 KB, patch)
2017-02-13 10:29 UTC, Bastien Nocera
committed Details | Review

Description Jiri Eischmann 2017-02-06 16:32:18 UTC
Unfortunately in many private networks captive portals use self-signed certificates, this results in a fullscreen white page with just some black text about a failure to access the page.

The portal should probably accept expired/bogus or just self-signed certs.

In my case the portal si redirecting to a https://<ip.address>/ site with a self signed cert.

Even though bogus certs are normally bad, there isn't much we can do here, w/o signon on the portal tehre is no network access anyway.

Originally reported in the Red Hat bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1419655
Comment 1 Michael Catanzaro 2017-02-06 16:40:05 UTC
After talking to some captive portal experts, I agree that we should ignore TLS errors in the portal helper. When we see a TLS error, we should ignore it, load the page anyway, and also degrade the state of a security indicator. That requires that we first add a security indicator, which means we have to first fix bug #749197. Apparently some insane captive portals rely on secure connections to Google and Google auth, so users are being trained to enter Google credentials to log in to captive portals... hence they legitimately need to know whether their https:// connection is actually secure or not.
Comment 2 Michael Catanzaro 2017-02-06 16:41:53 UTC
(In reply to Michael Catanzaro from comment #1)
> After talking to some captive portal experts

For the record: it's advice from Paul Wouters. We can pretend he is plural.
Comment 3 Michael Catanzaro 2017-02-11 03:46:51 UTC
To implement this, call webkit_web_context_set_tls_errors_policy() with WEBKIT_TLS_ERRORS_POLICY_IGNORE. It needs to be called on the WebKitWebContext that is used to create the WebKitWebView. Should be called before the view is created, right after the WebKitWebContext is created.
Comment 4 Bastien Nocera 2017-02-13 10:28:49 UTC
(In reply to Michael Catanzaro from comment #3)
> To implement this, call webkit_web_context_set_tls_errors_policy() with
> WEBKIT_TLS_ERRORS_POLICY_IGNORE. It needs to be called on the
> WebKitWebContext that is used to create the WebKitWebView. Should be called
> before the view is created, right after the WebKitWebContext is created.

I don't think this has the right behaviour, when combined with our call to:
let tlsInfo = this._webView.get_tls_info();
This made "expired.badssl.com" not show any insecure padlock.

So, I'd need to set WEBKIT_TLS_ERRORS_POLICY_FAIL as the policy, and handle "load-failed-with-tls-errors". This worked in my tests (which really doesn't mean much, see bug 778552).
Comment 5 Bastien Nocera 2017-02-13 10:29:07 UTC
Created attachment 345610 [details] [review]
portalHelper: Don't fail to load because of TLS errors

Accept self-signed and expired certificates as if we errored out, we
couldn't access the network at all. Consider this insecure though.
Comment 6 Michael Catanzaro 2017-02-13 15:07:23 UTC
Review of attachment 345610 [details] [review]:

Did my one-liner suggestion not work? WebKit.TLSErrorsPolicy.FAIL is the default policy and will always be; there's no need to set it manually. I was thinking we could just set WebKit.TLSErrorsPolicy.IGNORE and let the code you added in the previous patch handle updating the security indicator. Did you find some problem with that approach; did it stop WebKitWebView::insecure-content-detected from being emitted? I would be surprised if it did... IGNORE is a bit of a misnomer as the errors should still be reported, they just won't block the load.

Note that WebKitWebView::load-failed-with-tls-errors will not be emitted for subresources. There exists WebKitWebResource::failed-with-tls-errors that can be used to discover failures on subresources, but there's no way to choose to load them anyway. So this patch only allows TLS errors for the main resource and subresources for the same domain; errors for subresources from other domains will still cause those loads to be blocked. So this is not good.
Comment 7 Michael Catanzaro 2017-02-13 15:10:44 UTC
(In reply to Michael Catanzaro from comment #6)
> Review of attachment 345610 [details] [review] [review]:
> 
> Did my one-liner suggestion not work?

Sorry, I missed your comment above.

(In reply to Bastien Nocera from comment #4)
> I don't think this has the right behaviour, when combined with our call to:
> let tlsInfo = this._webView.get_tls_info();
> This made "expired.badssl.com" not show any insecure padlock.

:O

So WebKit is *really* ignoring the errors... that's unexpected to me. I don't think this is desirable behavior. No reasonable application would ever set WEBKIT_TLS_ERRORS_POLICY_IGNORE and then call webkit_web_view_get_tls_info() expecting to never see any errors reported there. This would technically be an API change, but I think we should change this behavior in WebKit. I'll investigate.
Comment 8 Bastien Nocera 2017-02-15 13:23:00 UTC
On IRC:
<KaL> it works as expected, if you are ignoring them, theya re not happening
<KaL> that's the expected behavior
<KaL> ignore means I don't care about tls
<KaL> if you care about tls but want to make exceptions use FAIL
Comment 9 Bastien Nocera 2017-02-15 13:31:31 UTC
Discussions about the WebKitGTK+ API about TLS certs is still on-going, but that'll
have to do for 3.24.
Thanks Carlos and Michael for the reviews and help.

Attachment 345610 [details] pushed as a768692 - portalHelper: Don't fail to load because of TLS errors
Comment 10 Bastien Nocera 2017-02-15 13:50:51 UTC
See also https://bugs.webkit.org/show_bug.cgi?id=168365