How is this phishing vector different from any "normal" browser window getting opened / overwritten when trying to access that network?

Which version of gnome-shell is this about?

For a normal webpage, either it's insecure http, in which case the browser cheering indicates that, or it's protected by TLS, in which case the browser chrome indicates the security status.  A malicious network can't fake a TLS certificate.

I think that gnome-shell-portal-helper should go further and add chrome that very clearly indicates that it's for hotspot sign-in only.  I have no idea what its cookie policy is, but I think it should be stateless and wipe itself every time it pops up a window.

This is whatever version Fedora 21 uses, but I think it's unchanged in newer versions.

(In reply to Andy Lutomirski from comment #2)
> For a normal webpage, either it's insecure http, in which case the browser
> cheering indicates that, or it's protected by TLS, in which case the browser
> chrome indicates the security status.  A malicious network can't fake a TLS
> certificate.
> 
> I think that gnome-shell-portal-helper should go further and add chrome that
> very clearly indicates that it's for hotspot sign-in only.  I have no idea
> what its cookie policy is, but I think it should be stateless and wipe
> itself every time it pops up a window.

Currently it will show clearly "Network Login" in the app menu.
Is it not enough?

For the record, the UI is very similar to what OS X provides: a simple login window with minimal chrome. At least OS X users are trained to recognize it as a network login page, and not, say, a fake antivirus or system modal dialog, or who knows what. Maybe. 

Note btw that a URL bar showing some random IP address will not help in this case - it's not a website the user knows, and he cannot distinguish 192.168.1.1 from 192.168.1.254.
In practice, for the hotel login use case, this means that an attacker on the same network can steal the credentials. I don't think there is much we can do here, and phishing is the least of our concerns.

As for the cookie policy, yeah it's all stateless: cookies, local storage, even cache (for obvious reasons), all in memory, all gone between successive login attempts.

Note btw that TLS on its own means nothing: the first step has to be HTTP (otherwise the redirect will not work), and you can easily get certificates for verysign.co, visasecurepay.net, securepaypal.mobi, loginxfinitywifi.biz or who knows what.
As a tech user, you can see that all of them are sketchy, but would a user know that? And what if the motel/coffee place/airport itself is sketchy, how do you know what's legit and what's phishing?

(Yes, all those are available, at least today, checking GoDaddy)

One can certainly argue that TLS is weaker than it should be.  Nonetheless, gnome-shell-portal-helper shouldn't negate whatever level of security TLS can provide.

Imagine an attacker that sets up a rogue AP that redirects to a malicious captive portal.  The captive portal page could have a title of "Sign In - Google Accounts - Mozilla Firefox".  The contents would be a pixel-perfect reproduction of Firefox's chrome, including "https://accounts.google.com/..." with the appropriate colors.

The result would be a window that is indistinguishable from a real Firefox window, except that, if you focus it, you see "Captive Portal" on the left side of the top bar.  Even if you notice that, you'd need to realize that the "Captive Portal" program doesn't implement any form of UI security in order to realize that you're being phished.

There's a reason that no real browser allows popups to suppress the address bar.

I think we are conflating two different problems here.

TLS matters for a network attacker trying to eavesdrop legitimate credentials by downgrading the connection.
As mentioned, this problem is fundamentally unsolvable because the first step is insecure by design, and the latter steps can have TLS just fine, using a semantic attack.
Conversely, a phishing attacker does not need/care for TLS, because he can reproduce whatever positive security indicators a browser provides, even where there is no TLS.

Therefore, let's keep TLS out of the discussion, because it doesn't really matter.

As for the phishing itself, would keeping "- Captive Portal" in the title suffice to you?
I disagree that showing the address bar has any benefit - the attacker can have another address bar just right under, and the user would be confused enough to think the second one is the right one. Or the attacker could again go creative and choose accountsgoogle.co.

Alternatively, if we really want to be pedantic we could show an infobar explaining what a captive portal his, and what are the security implications of writing stuff in there.
But again, if OS X is any lesson, I don't think we need it.

I see no reason to invent a new paradigm here.  To use the web safely, users already need to understand the security indicators in the URL bar.  Asking users to learn, in addition, that "Captive Portal", on GNOME, means "don't type secrets" is unnecessary.

Also, failing to show an address bar prevents even knowledgeable users from safely typing secrets into the many captive portals that want an account.  These things are all over (e.g. Boingo).

IOW, the current design has two separate problems, and adding an address bar solves both.  Also, if you don't think an address bar + certificates is a good idea in general, take it up with Mozilla, Google, Apple, and Microsoft.  gnome-shell is not the place to reinvent the web.

Thanks for making this bug public; I don't see much value in secrecy here.

Tangents first:

> As for the cookie policy, yeah it's all stateless: cookies, local storage,
> even cache (for obvious reasons), all in memory, all gone between successive
> login attempts.

TBH, just looking at the source code for the portal-helper, I am not really confident of that: I would expect that since you don't use webkit_settings_set_enable_private_browsing() anywhere, stuff will be saved to default directories like $XDG_DATA_HOME/webkit and $XDG_DATA_HOME/webkitgtk. I guess that is probably happening for local storage, the icon database, and the soup cache, at least. I presume you're right that cookies won't be saved by default, but we _really_ ought to document that (I will file a WebKit bug).

That said, none of the stuff that will be saved is any security risk worse than browsing to an http:// URI. The captive portal will only be able to screw with local storage for http:// URIs, for example, which it could do any time after the portal-helper is closed anyway. Same with the cache. So I think no changes are required here.

> Alternatively, if we really want to be pedantic we could show an infobar
> explaining what a captive portal his, and what are the security implications
> of writing stuff in there.
> But again, if OS X is any lesson, I don't think we need it.

I think it would be good to display a warning infobar if the user selects a password form and the page is not delivered via TLS with a trusted certificate. More than that is probably overkill: setting the window title properly should suffice.


OK, those were my tangents. Now, I see two big issues here.


Issue #1:

(In reply to Giovanni Campagna from comment #3) 
> Currently it will show clearly "Network Login" in the app menu.
> Is it not enough?

That's a lot better than nothing, but it's outside the window and potentially far from it. I think we can do better than that. The first change I would make is to take control of the window title away from the attacker: it should display "Network Login" (or something more descriptive: "Log in to controlled-access network") rather than allow the attacker to put whatever he wants up there.

> For the record, the UI is very similar to what OS X provides: a simple login
> window with minimal chrome. At least OS X users are trained to recognize it
> as a network login page, and not, say, a fake antivirus or system modal
> dialog, or who knows what. Maybe. 

For one example of what we need to defend against: http://arstechnica.com/security/2015/06/evil-wifi-captive-portal-could-fool-users-into-giving-up-apple-pay-data/

Note how the system-controlled title bar used by iOS (switch to the second screenshot in the image gallery) makes the attack look significantly more suspicious. That should help alert the user that something is up.

> As for the phishing itself, would keeping "- Captive Portal" in the title
> suffice to you?

That would be a big step forward, although I think we should use a nontechnical term here.


Issue #2


> Note btw that a URL bar showing some random IP address will not help in this
> case - it's not a website the user knows, and he cannot distinguish
> 192.168.1.1 from 192.168.1.254.

I think we need to clarify requirements before proceeding. I believe Andy is concerned about the case where the wi-fi hotspot sends you to https://google.com to authenticate with your Google account. (Or, replace Google with whatever other big OAuth provider you want. Or this Boingo thing perhaps, though I've never heard of them before.) The user's expectation is that only Google can see his password, and that the hotspot will only be able to see the same basic information about his Google account (probably name and email) that Google displays will be made available to the hotspot. (Andy, can you confirm this fits with your expectation? I've never seen a legit hotspot do this before.)

If the user sees a scary IP address in the address bar, or if he sees any URL that isn't google.com, we can hope that the user will think "I should probably not enter my Google password here." At least a savvy user will have a pretty good chance. But right now the attacker can simply make the title bar say https://google.com.

> In practice, for the hotel login use case, this means that an attacker on
> the same network can steal the credentials. I don't think there is much we
> can do here, and phishing is the least of our concerns.

Like any other web page, the user should probably have an indication of (a) the URI of the page he is viewing, and (b) a little lock icon to indicate whether that page was delivered securely (TLS with valid certificate chain) or not. (I guess further discussion will decide this, since it seems you'll need to be convinced. :)

(In reply to Giovanni Campagna from comment #4)
> Note btw that TLS on its own means nothing: the first step has to be HTTP
> (otherwise the redirect will not work), and you can easily get certificates
> for verysign.co, visasecurepay.net, securepaypal.mobi, loginxfinitywifi.biz
> or who knows what.
> As a tech user, you can see that all of them are sketchy, but would a user
> know that? And what if the motel/coffee place/airport itself is sketchy, how
> do you know what's legit and what's phishing?
> 
> (Yes, all those are available, at least today, checking GoDaddy)

This argument applies equally well to web browsers, so browsers should just give up and not display any security indicators at all, right? Of course not. We display the URI and security indicator so that users have at least the possibility to detect an attack. That's all we can do. TLS makes a pretty decent guarantee that you're visiting the real verysign.co or gooogle.com, and if users don't notice the problem with that URI there's not really anything we can do: same as a normal web browser.

(In reply to Giovanni Campagna from comment #6)
> I think we are conflating two different problems here.
> 
> TLS matters for a network attacker trying to eavesdrop legitimate
> credentials by downgrading the connection.
> As mentioned, this problem is fundamentally unsolvable because the first
> step is insecure by design, and the latter steps can have TLS just fine,
> using a semantic attack.

The first step is insecure by design, yes. We load an HTTP page and expect it to be intercepted by either a legit captive portal, a malicious captive portal, or Mallory in the booth next to you pretending to be the captive portal.

I don't see how that supports your argument, though. The captive portal can choose to redirect to a secure page (TLS with a trusted certificate) or not, and if it chooses not, we should indicate that the page is insecure.

> Conversely, a phishing attacker does not need/care for TLS, because he can
> reproduce whatever positive security indicators a browser provides, even
> where there is no TLS.
>
> Therefore, let's keep TLS out of the discussion, because it doesn't really
> matter.

Woah, hold up: the attacker cannot touch our window chrome without some extra exploit.

My understanding of your argument is "the attacker can get around TLS by redirecting the user to an https page with a URI that looks like the URI of a legit site." Yes, she can, but if we display the URI in question, then savvy users will be able to detect an attack. Otherwise, users have absolutely no chance.

> I disagree that showing the address bar has any benefit - the attacker can
> have another address bar just right under, and the user would be confused
> enough to think the second one is the right one. Or the attacker could again
> go creative and choose accountsgoogle.co.

Er, well I'd like to think that a second address bar would be a pretty obvious red flag to most users... maybe that is too much to expect....

Anyway, it seems like the disagreement is over whether the _entire_ process should be treated as insecure, or if we expect that some captive portals will take the user to secure pages as part of the authentication process. I thought the former was the case and that's clearly what portal-helper was designed for, but according to Andy that's not true and captive portals that want passwords like Boingo are "all over." To handle that properly, we need to show URI + security indicator. The alternative is to just say the entire process is insecure, and display an insecure indicator always or change the window title to always say "insecure," but I see no significant value in that. For sure, the status quo is not OK.

My recommendation is to use a header bar with a static title (e.g. "Network Login") and display the actual URI of the web view in the subtitle (after passing the URI through g_uri_unescape_string() to make UTF-8 work). Next to the URI, we display the standard security indicator (channel-secure-symbolic for TLS with a trusted certificate chain, channel-insecure-symbolic otherwise). For the initial load of the test page http://www.gnome.org, if we hit LOAD_COMMITTED without a redirect, can't show any subtitle but we can change the title to, say "Insecure Network Login." If we want to go above and beyond, we can split out Epiphany's security indicator into a submodule or library so we can display the full security popover when you click on the lock.

I'm willing to implement whatever we agree on, since this should be fairly simple.

I don't think launching a web browser (least of all a third-party browser we don't control, like Firefox) would be appropriate: we don't want the user navigating to different URIs (so it would not make any sense to display an address bar), opening bookmarks, or doing anything other than getting past the captive portal.

(In reply to Michael Catanzaro from comment #8)
> I presume you're
> right that cookies won't be saved by default, but we _really_ ought to
> document that (I will file a WebKit bug).

It was documented but buried a bit. "By default, @cookie_manager doesn't store the cookies persistenly, so you need to call this method to keep cookies saved across sessions."

I'm sorry guys, I agree that the current design of the portal helper, even if copied from OS X, is sloppy and insecure, and Michael's suggestion should be implemented.
Unfortunately, I do not have time to it myself. I can review Michael's code, but I figure he knows more WebkitGtk than I do so it will be moot.

Created attachment 344020 [details] [review]
portalHelper: Don't change the main window title

The title of the window should not be in control of a potentially
hostile hotspot provider, so only set the subtitle to be that of the
page, the main title will stay the same.

The subtitle will also be set to a URI, so that the hotspot cannot be
used to control the title shown in our UI.

Helps

Which leaves the padlock and possibly related dialogues. Which makes me wonder whether the portal helper shouldn't be provided by epiphany instead, as a mode like the application mode, given that we'd likely want to also save passwords and possibly other things as well.

Review of attachment 344020 [details] [review]:

This is a big improvement and looks good as-is.

The one additional thing that needs done to close this bug is to add a security indicator like Epiphany's. It could be very very simple: just an unclickable icon would be fine, there's no reason to go extravagant here. Start it out in insecure state for HTTP pages and secure state for HTTPS pages, and degrade it from secure to insecure in a callback for the WebKitWebView::insecure-content-detected signal. I would put the icon in the subtitle to the left of the URI, like in Epiphany 3.22.

After (and only after) we have a security indicator, we should also (in a separate bug) start using WEBKIT_TLS_ERRORS_POLICY_IGNORE so that we always load the insecure resource and simply degrade the state of the security indicator instead. That's just one function call to webkit_web_context_set_tls_errors_policy(). Paul Wouters has advised that we really ought to do this in order to make the portal helper more robust. It would be easier to handle this by improving the existing portal helper than by using Epiphany since this behavior would not be acceptable for a real web browser.

(It would probably be easier to switch to Epiphany if password saving is required, but it would probably be better to reimplement that here instead. There is a big advantage to segregating captive portal passwords from normal browser passwords, after all. Another advantage of switching to Epiphany would be to grow support for displaying IDN URIs in the subtitle, but perhaps that functionality should move down into WebKit.)

Comment on attachment 344020 [details] [review]
portalHelper: Don't change the main window title

Attachment 344020 [details] pushed as 49607e1 - portalHelper: Don't change the main window title

Michael, it would be very useful if you had some test pages I could point the helper to, so I can verify that the handling is done properly.

This is the default URI used in testing:
const CONNECTIVITY_CHECK_HOST = 'nmcheck.gnome.org';
const CONNECTIVITY_CHECK_URI = 'http://' + CONNECTIVITY_CHECK_HOST;

(In reply to Bastien Nocera from comment #15)
> Michael, it would be very useful if you had some test pages I could point
> the helper to, so I can verify that the handling is done properly.

What sort of test pages...?

Ones that could show the range of different cases you mentioned in comment 13: http to https redirection, https to https with mixed content, https to http, etc.

(In reply to Bastien Nocera from comment #17)
> http to https redirection, 

http://www.gnome.org

> https to https with mixed content

https://mixed.badssl.com/

> https to http

https://planet.gnome.org

(Yes that's very dumb and should be fixed; we should just remove feeds that don't support HTTPS.)

(In reply to Michael Catanzaro from comment #13)
> Start it out in insecure state for HTTP pages and secure state for HTTPS
> pages

But don't do it by checking the URI. Instead us webkit_web_view_get_tls_info() and check the GTlsCertificateFlags *errors parameter that gets returned. If it's nonzero then display the insecure icon. (Maybe give the insecure icon a generic tooltip "Your connection has been intercepted by a Wi-Fi hotspot".)

The documentation for that function reminds me that there is one more problem: it only provides information on the security of the main resource. So when we set the TLS errors policy to ignore, we'll then not be informed if the portal hijacks just a subresource, e.g. https://google.com/security-sensitive.js, and not the main resource. Legit portals won't do that, but malicious ones will. I think it's not currently possible to get the behavior we want without adding new WebKit API for this. :/ Ignoring TLS errors will have to be put on hold until we have a way to detect TLS errors in subresource loads without blocking the subresource load. We've never had any application want this behavior before.

(In reply to Michael Catanzaro from comment #19)
> (In reply to Michael Catanzaro from comment #13)
> > Start it out in insecure state for HTTP pages and secure state for HTTPS
> > pages
> 
> But don't do it by checking the URI. Instead us
> webkit_web_view_get_tls_info() and check the GTlsCertificateFlags *errors
> parameter that gets returned. If it's nonzero then display the insecure
> icon. (Maybe give the insecure icon a generic tooltip "Your connection has
> been intercepted by a Wi-Fi hotspot".)

Sorry: if the function returns TRUE *and* errors is zero, then the main resource is secure. If it returns FALSE *or* errors is nonzero, the main resource is not secure.

(In reply to Michael Catanzaro from comment #19)
> Ignoring TLS errors will have to be put on hold until we have a way to
> detect TLS errors in subresource loads without blocking the subresource
> load. We've never had any application want this behavior before.

If you want to work on the gnome-shell component of this soon, I will try to handle the WebKit part.

(In reply to Michael Catanzaro from comment #19) 
> The documentation for that function reminds me that there is one more
> problem: it only provides information on the security of the main resource.
> So when we set the TLS errors policy to ignore, we'll then not be informed
> if the portal hijacks just a subresource, e.g.
> https://google.com/security-sensitive.js, and not the main resource. Legit
> portals won't do that, but malicious ones will.

That's all right.

> I think it's not currently
> possible to get the behavior we want without adding new WebKit API for this.
> :/ Ignoring TLS errors will have to be put on hold until we have a way to
> detect TLS errors in subresource loads without blocking the subresource
> load. We've never had any application want this behavior before.

This is totally wrong, it's easy: just connect to WebKitWebView::insecure-content-detected and degrade the security indicator the first time it fires. No problem and no need to change anything in WebKit. :)

Created attachment 345480 [details] [review]
portalHelper: Add security icon to titlebar

This adds a security icon (either secure or insecure) to the portal
helper's title bar. As soon as a part or all of the page and its content
is server insecurely, the icon shown will be a broken padlock.

Comment 18 to 22 were quite confusing. I'm going to try and sum up:
- start with the insecure icon
- set the icon to be secure when webkit_web_view_get_tls_info() returns TRUE *and* errors is zero, set to insecure otherwise
- set icon to be insecure if insecure-content-detected is fired

My questions are:
- which signal(s) do I need to update this from?
- when do I reset the state from "insecure-content-detected"?

There's probably ten lines of code left to write, and the file is opened in my editor, so would be great if you could answer quickly for review :)

(In reply to Bastien Nocera from comment #24)
> Comment 18 to 22 were quite confusing. I'm going to try and sum up:
> - start with the insecure icon
> - set the icon to be secure when webkit_web_view_get_tls_info() returns TRUE
> *and* errors is zero, set to insecure otherwise
> - set icon to be insecure if insecure-content-detected is fired

Sorry for the messy comments. I confused myself more than once while writing them!

Your summary is exactly what I said. Thinking about it more, it would make one more change: display no icon at first. Only display the security icon once the load is committed.

> My questions are:
> - which signal(s) do I need to update this from?

WebKitWebView::load-changed. Show and set to secure/insecure when the WebKitLoadEvent is WEBKIT_LOAD_COMMITTED. Hide when the WebKitLoadEvent is WEBKIT_LOAD_STARTED.

> - when do I reset the state from "insecure-content-detected"?

Hide the icon during WebKitWebView::load-changed when the WebKitLoadEvent is WEBKIT_LOAD_STARTED. Once insecure content is detected, treat the whole page as insecure until the next load starts.

*****

Note that the load events always occur in this order:

 (1) WEBKIT_LOAD_STARTED
 (2) WEBKIT_LOAD_REDIRECTED (optional, one or many times, if there are one or more redirects)
 (3) WEBKIT_LOAD_COMMITTED (after the final redirect. The main document URI can no longer change until the next load.)
 (4) WEBKIT_LOAD_FINISHED

*****

So in summary:

 (1) Start with a hidden icon. Also hide the icon during WebKitWebView::load-changed if the WebKitLoadEvent is WEBKIT_LOAD_STARTED.
 (2) Show the icon and set it to secure or insecure during WebKitWebView::load-changed if the WebKitLoadEvent is WEBKIT_LOAD_COMMITTED. Set to secure when webkit_web_view_get_tls_info() returns TRUE *and* errors is zero. Set to insecure otherwise.
 (3) Set the icon to be insecure during WebKitWebView::insecure-content-detected. No need to show it here.

Review of attachment 345480 [details] [review]:

Looking good....

::: js/portalHelper/main.js
@@ +98,3 @@
+        } else {
+            this._lockImage.set_from_icon_name("channel-insecure-symbolic", Gtk.IconSize.MENU);
+            this._lockImage.set_tooltip_text(_('Your connection has been intercepted by a Wi-Fi Hotspot'));

I would say: "Your connection to this hotspot login is not secure. Passwords or other information you enter on this page can be viewed by people nearby."

(This will probably almost always be shown, but a secure hotspot is possible if it redirects the user to a page for which it has a valid TLS certificate.)

@@ +159,3 @@
         let uri = this._webView.uri;
         if (uri)
+            this._headerBar.setSubtitle(GLib.uri_unescape_string(uri, null, false));

Hm, guess what, g_uri_unescape_string only takes two arguments in C. I doubt the JavaScript binding is different, so I bet the third argument is being silently ignored because JavaScript is great that way. Am I wrong? (I know this is a preexisting issue.)

(In reply to Michael Catanzaro from comment #25)
> Thinking about it more, it would make
> one more change:

I meant to say: "I would make one more change"

(In reply to Michael Catanzaro from comment #26)
> Review of attachment 345480 [details] [review] [review]:
> 
> Looking good....
> 
> ::: js/portalHelper/main.js
> @@ +98,3 @@
> +        } else {
> +            this._lockImage.set_from_icon_name("channel-insecure-symbolic",
> Gtk.IconSize.MENU);
> +            this._lockImage.set_tooltip_text(_('Your connection has been
> intercepted by a Wi-Fi Hotspot'));
> 
> I would say: "Your connection to this hotspot login is not secure. Passwords
> or other information you enter on this page can be viewed by people nearby."
> 
> (This will probably almost always be shown, but a secure hotspot is possible
> if it redirects the user to a page for which it has a valid TLS certificate.)

This is overly long for a tooltip. This is more appropriate for a separate popup, no?

> @@ +159,3 @@
>          let uri = this._webView.uri;
>          if (uri)
> +            this._headerBar.setSubtitle(GLib.uri_unescape_string(uri, null,
> false));
> 
> Hm, guess what, g_uri_unescape_string only takes two arguments in C. I doubt
> the JavaScript binding is different, so I bet the third argument is being
> silently ignored because JavaScript is great that way. Am I wrong? (I know
> this is a preexisting issue.)

Yeah, I've fixed it locally.

I get a warning when running:
let tlsInfo = this._webView.get_tls_info();

** (gnome-shell-portal-helper:2608): CRITICAL **: gboolean webkit_web_view_get_tls_info(WebKitWebView*, GTlsCertificate**, GTlsCertificateFlags*): assertion 'wkCertificateInfo' failed

Do you want me to file a new WebKit bug about it? The webview is loading http://nmcheck.gnome.org

Created attachment 345585 [details] [review]
portalHelper: Add security icon to titlebar

This adds a security icon (either secure or insecure) to the portal
helper's title bar. As soon as a part or all of the page and its content
is served insecurely, the icon shown will be a broken padlock.

Review of attachment 345585 [details] [review]:

OK, this looks almost good!

(In reply to Bastien Nocera from comment #29)
> I get a warning when running:
> let tlsInfo = this._webView.get_tls_info();
> 
> ** (gnome-shell-portal-helper:2608): CRITICAL **: gboolean
> webkit_web_view_get_tls_info(WebKitWebView*, GTlsCertificate**,
> GTlsCertificateFlags*): assertion 'wkCertificateInfo' failed
> 
> Do you want me to file a new WebKit bug about it? The webview is loading
> http://nmcheck.gnome.org

The problem is it's programmer error to call that function before load committed. You're calling it unnecessarily in load redirected and load finished. Don't do that!

(This used to be a real WebKit bug that regularly crashed Epiphany, caused by faking the order of load events in the WebKitGTK+ API layer to work around WebCore sometimes emitted them in an unexpected order. Carlos cleaned that up a year or two ago.)

(In reply to Bastien Nocera from comment #28)
> This is overly long for a tooltip. This is more appropriate for a separate
> popup, no?

Up to you. I guess I don't mind long tooltips. I just get annoyed when they fill the whole screen and disappear off the top edge; GTK+ doesn't handle long tooltips very well at all....

::: js/portalHelper/main.js
@@ +197,3 @@
+        if (loadEvent == WebKit.LOAD_STARTED) {
+            this._headerBar.setSecurityIcon(PortalHelperSecurityLevel.NOT_YET_DETERMINED);
+            this.insecureContent = false;

You can delete this member variable (see below).

@@ +198,3 @@
+            this._headerBar.setSecurityIcon(PortalHelperSecurityLevel.NOT_YET_DETERMINED);
+            this.insecureContent = false;
+        } else {

You really need to check if (loadEvent == WebKit.LOAD_COMMITTED) here to avoid the g_return_val_if_fail() inside WebKit.

@@ +199,3 @@
+            this.insecureContent = false;
+        } else {
+            if (this.insecureContent) {

Insecure subresources can only appear after the load is committed, so don't bother checking for insecure content here. So you should have:

if (loadEvent == WebKit.LOAD_STARTED) {
   // ...
} else if (loadEvent == WebKit.LOAD_COMMITTED) {
  let tlsInfo = this._webView.get_tls_info();
  // ...
}

@@ -123,0 +196,11 @@
+    _onLoadChanged: function(loadEvent) {
+        if (loadEvent == WebKit.LOAD_STARTED) {
+            this._headerBar.setSecurityIcon(PortalHelperSecurityLevel.NOT_YET_DETERMINED);
... 8 more ...

(I like how gjs handles the binding, getting rid of the out arguments.)

@@ -123,0 +196,22 @@
+    _onLoadChanged: function(loadEvent) {
+        if (loadEvent == WebKit.LOAD_STARTED) {
+            this._headerBar.setSecurityIcon(PortalHelperSecurityLevel.NOT_YET_DETERMINED);
... 19 more ...

Delete this member variable. All you have to do here is set the security icon.

Review of attachment 345585 [details] [review]:

::: js/portalHelper/main.js
@@ +215,3 @@
+    _onInsecureContentDetected: function (insecureContentEvent) {
+        if (!this.insecureContent) {
+            this.insecureContent = true;

Sorry, apparently I don't know how to use Splinter; it does weird things if I click on the line number instead of the line itself. My last comment was meant to be a reminder to delete this.insecureContent.

Created attachment 345595 [details] [review]
portalHelper: Add security icon to titlebar

This adds a security icon (either secure or insecure) to the portal
helper's title bar. As soon as a part or all of the page and its content
is served insecurely, the icon shown will be a broken padlock.

Review of attachment 345595 [details] [review]:

Looks good!

Bonus points if you fix bug #778253 as well. It should be a one-liner.

Attachment 345595 [details] pushed as 304b68e - portalHelper: Add security icon to titlebar