GNOME Bugzilla – Bug 744064
Insecure content warning should be displayed on all insecure pages
Last modified: 2015-12-09 11:45:18 UTC
Right now, we have the following security UI, shared by all major browsers: * HTTP pages -> no security indicator * HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon This makes very little sense, since a page with some security is treated better than a page with no security at all. Let's do this instead: * HTTP pages and HTTPS pages with mixed content (or, in the future, connected via unsafe protocol downgrade, a weaker cipher that we don't want to block completely, etc.) -> insecure warning icon * HTTPS pages with no mixed content (or, in the future, blocked mixed content) -> secure lock icon This presents a very clear, binary security distinction to the user: the page is either secure, or it's not secure. Note that the warning icon is mostly meant to be ignored, so I'm not concerned about warning fatigue with respect to this indicator; we'll continue to use an interstitial for unacceptable TLS certificates. Chrome is planning to do this too [1], but let's not wait for them. A couple more things to change: * The current text of the popover on pages with mixed content is "Part of this page is insecure." This doesn't really reflect the level of risk: an insecure script cedes complete control of the page to an attacker, and any insecure content on the same domain gives up the user's cookies (bad when the session cookie is not marked as secure). Let's not make this distinction at all, and just say "This page is insecure." * Let's strip away the protocol http:// https:// file:// etc. from the URI when displaying it in the subtitle of the title box (but not when displaying it in the location entry) so that nobody thinks "https means secure!". [1] https://www.chromium.org/Home/chromium-security/marking-http-as-non-secure
Created attachment 296251 [details] [review] Display insecure content warning on all insecure pages We used to do this: * HTTP pages -> no security indicator * HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon Now we do this: * HTTP pages and HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon
Created attachment 296252 [details] [review] Trim the protocol version from the title box subtitle So we display www.gnome.org instead of http://www.gnome.org in the title box. The location entry still has the protocol version. I *think* I like this. Am I crazy?
Review of attachment 296251 [details] [review]: I guess we are not used to seeing an insecure icon for non HTTPS pages, but I agree non HTTPS are indeed insecure. ::: embed/ephy-web-view.c @@ +985,3 @@ "The view's security level", EPHY_TYPE_SECURITY_LEVEL, + EPHY_SECURITY_LEVEL_TO_BE_DETERMINED, EPHY_SECURITY_LEVEL_UNKNOWN? @@ +1568,3 @@ +static char * +uri_to_uri_scheme (const char *uri) This sounds like it converts a uri to a scheme, while it's getting the scheme of a uri. @@ +1574,3 @@ + result = strncpy (result, uri, colon); + result[colon] = '\0'; + return result; I think we don't need this. Use soup_uri_new() + soup_uri->scheme @@ +1601,3 @@ + if (webkit_security_manager_uri_scheme_is_local (security_manager, uri_scheme)) { + g_assert (!priv->certificate); + security_level = EPHY_SECURITY_LEVEL_LOCAL_PAGE; So, this is the only case when we don't show any icon now, right? (Well, and in case of unknown) @@ +1606,3 @@ + security_level = priv->tls_errors == 0 ? + EPHY_SECURITY_LEVEL_STRONG_SECURITY : EPHY_SECURITY_LEVEL_UNACCEPTABLE_CERTIFICATE; + } else if (priv->loading_placeholder) { I don't think we need this, we could probably use ephy_embed_has_load_pending() @@ +1709,2 @@ ephy_web_view_thaw_history (view); + priv->loading_placeholder = FALSE; Ditto, we don't need to keep track of this, I think, EphyEmbed already does it. @@ +1744,3 @@ + /* Could reuse freeze_history for this, but that would be confusing. */ + priv->loading_placeholder = TRUE; Let's try avoid it instead :-) ::: lib/ephy-security-levels.h @@ +38,3 @@ EPHY_SECURITY_LEVEL_MIXED_CONTENT, + /* EPHY_SECURITY_LEVEL_WEAK_SECURITY should be implemented for e.g. SHA1 certs + RC4, protocol version fallback, no safe renegotiation extension, etc. etc. */ Don't use a comment for this, use #if 0 and add a comment starting with FIXME or TODO ::: lib/widgets/ephy-certificate-popover.c @@ +89,3 @@ + if (certificate) + priv->certificate = g_object_ref (certificate); A certificate popover that might not have a certifciate sounds weird to me. Maybe we should rename this as EphySecurityInfoPopover or simply EphySecurityPopover. @@ +164,3 @@ + + if (!priv->certificate) + gtk_widget_hide (priv->certificate_button); Why don't we save the grid in the priv struct, and create the certificate button here conditionally instead of hiding it? ::: src/ephy-title-box.c @@ +238,3 @@ ephy_title_box_add_title_bar (title_box); + gtk_widget_set_visible (priv->lock_image, TRUE); Shouldn't this be hidden for local page and unknown?
Review of attachment 296252 [details] [review]: If we show the secure icon always in the title, this might not be a problem and we have more space for the url. ::: src/ephy-title-box.c @@ +680,3 @@ + if (result) + return result + 3; + return uri; Use SoupURI for this.
(In reply to comment #3) > Shouldn't this be hidden for local page and unknown? I'm not sure if it matters, since the icon would be invisible, but it surely can't hurt to hide it. (In reply to comment #4) > ::: src/ephy-title-box.c > @@ +680,3 @@ > + if (result) > + return result + 3; > + return uri; > > Use SoupURI for this. This is actually harder to do with SoupURI. The nicest I could come up with would be to pass uri->scheme to strstr instead of :// and then adjust by strlen(uri->scheme) instead of 3.
Created attachment 296328 [details] [review] Display insecure content warning on all insecure pages We used to do this: * HTTP pages -> no security indicator * HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon Now we do this: * HTTP pages and HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon
Created attachment 296329 [details] [review] Trim the protocol version from the title box subtitle So we display www.gnome.org instead of http://www.gnome.org in the title box. The location entry still has the protocol version.
Created attachment 296330 [details] [review] ephy-title-box: rename uri to subtitle This was already not a URI because of the triangle character.
Created attachment 296331 [details] [review] Rename EphyCertificatePopover to EphySecurityPopover Since it is now used for HTTP as well.
(In reply to comment #5) > This is actually harder to do with SoupURI. The nicest I could come up with > would be to pass uri->scheme to strstr instead of :// and then adjust by > strlen(uri->scheme) instead of 3. FYI I didn't change anything in this patch. I implemented all your other comments though.
Created attachment 296386 [details] [review] Display insecure content warning on all insecure pages
Created attachment 296387 [details] [review] Trim the protocol from the title box subtitle So we display www.gnome.org instead of http://www.gnome.org in the title box. The location entry still has the protocol.
Created attachment 296388 [details] [review] ephy-title-box: rename uri to subtitle This was already not a URI because of the triangle character.
Created attachment 296389 [details] [review] Rename EphyCertificatePopover to EphySecurityPopover Since it is now used for HTTP as well.
Carlos, Claudio, any more thoughts on these? This will be our major showpoint for 3.18, I think.
Ping reviewers. Sergio, Carlos mentioned you might have had an objection to this change...?
Created attachment 316959 [details] [review] Display insecure content warning on all insecure pages We used to do this: * HTTP pages -> no security indicator * HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon Now we do this: * HTTP pages and HTTPS pages with mixed content -> insecure warning icon * HTTPS pages with no mixed content -> secure lock icon
Created attachment 316960 [details] [review] Trim the protocol from the title box subtitle So we display www.gnome.org instead of http://www.gnome.org in the title box. The location entry still has the protocol.
Created attachment 316961 [details] [review] ephy-title-box: rename uri to subtitle This was already not a URI because of the triangle character.
Created attachment 316962 [details] [review] Rename EphyCertificatePopover to EphySecurityPopover Since it is now used for HTTP as well.
Review of attachment 316959 [details] [review]: ::: embed/ephy-web-view.c @@ +1563,3 @@ + + if (webkit_security_manager_uri_scheme_is_local (security_manager, soup_uri->scheme)) { + g_assert (!view->certificate); I don't think we need this, we have just done g_clear_object (&view->certificate); ::: lib/ephy-security-levels.h @@ +29,3 @@ G_BEGIN_DECLS +#if 0 Use a comment instead of ifdefed code, please.
Review of attachment 316961 [details] [review]: Makes sense, yes.
Review of attachment 316962 [details] [review]: I'm assuming this is just a rename, there are no actual changes.
Attachment 316959 [details] pushed as 759a59a - Display insecure content warning on all insecure pages Attachment 316961 [details] pushed as bc32937 - ephy-title-box: rename uri to subtitle Attachment 316962 [details] pushed as 383cd99 - Rename EphyCertificatePopover to EphySecurityPopover
I will fix these in a follow-up patch, since yet again I have pushed before reading review comments <_< (In reply to Carlos Garcia Campos from comment #21) > Review of attachment 316959 [details] [review] [review]: > > ::: embed/ephy-web-view.c > @@ +1563,3 @@ > + > + if (webkit_security_manager_uri_scheme_is_local (security_manager, > soup_uri->scheme)) { > + g_assert (!view->certificate); > > I don't think we need this, we have just done g_clear_object > (&view->certificate); Yeah, OK. > ::: lib/ephy-security-levels.h > @@ +29,3 @@ > G_BEGIN_DECLS > > +#if 0 > > Use a comment instead of ifdefed code, please. You told me to use an ifdef instead of a comment, up above! I agree, a comment is much better.
(In reply to Michael Catanzaro from comment #25) > I will fix these in a follow-up patch, since yet again I have pushed before > reading review comments <_< > > (In reply to Carlos Garcia Campos from comment #21) > > Review of attachment 316959 [details] [review] [review] [review]: > > > > ::: embed/ephy-web-view.c > > @@ +1563,3 @@ > > + > > + if (webkit_security_manager_uri_scheme_is_local (security_manager, > > soup_uri->scheme)) { > > + g_assert (!view->certificate); > > > > I don't think we need this, we have just done g_clear_object > > (&view->certificate); > > Yeah, OK. > > > ::: lib/ephy-security-levels.h > > @@ +29,3 @@ > > G_BEGIN_DECLS > > > > +#if 0 > > > > Use a comment instead of ifdefed code, please. > > You told me to use an ifdef instead of a comment, up above! I agree, a > comment is much better. It' different thing. In previous patch you had commented code (the new level), but here is just a comment. So, code that we can add yet should be ifdefef and with a comment explaining why it's ifdefed. Comments explaining whatever should always be comments