Open Bug 1310842 Opened 3 years ago Updated 4 months ago

[meta] Show a negative indicator for HTTP and no indicator for HTTPS

Categories

(Firefox :: Security, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: tanvi, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(4 files, 1 obsolete file)

Research has shown that people don't notice the absence of security indicators.  On HTTPS pages, we show a lock icon to show users that the connection is secure.  But when the user visits an HTTP page, do they notice that the lock is missing and their connection is not secure?

Instead, we should show a negative indicator over HTTP and potentially show nothing at all when the user is visiting an HTTPS page.

This will require a multi-step process and the timeline will likely be influenced by our telemetry data around the percentage of pages that are HTTPS and world events.
Some steps we can take (not necessarily in order):

* Add a preference to about:config that enables the lock with the strikethrough on HTTP pages. - bug 1310447

* Turn on the feature for a pre-release channel for a specified about of time.

* Turn on that feature for some portion of release or beta users to conduct user research

* Turn on the feature for all users.

* Remove the green lock icon from HTTPS pages.
Severity: normal → enhancement
Priority: -- → P3
Are we going to ship improvements here, timed to match the upcoming Chrome changes?

https://security.googleblog.com/2018/02/a-secure-web-is-here-to-stay.html
Flags: needinfo?(tanvi)
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #2)
> Are we going to ship improvements here, timed to match the upcoming Chrome
> changes?
> 
> https://security.googleblog.com/2018/02/a-secure-web-is-here-to-stay.html

Jonathan has added some prefs to show "Not Secure" and the lock with the strikethrough on HTTP pages.

https://bugzilla.mozilla.org/show_bug.cgi?id=1335970
https://bugzilla.mozilla.org/show_bug.cgi?id=1310447

In Nightly Private Browsing Mode, the lock with the strikethrough is shown on HTTP pages: https://bugzilla.mozilla.org/show_bug.cgi?id=1434626
Flags: needinfo?(tanvi)
Assignee: nobody → jkt
Attached image Screenshot from 2018-02-20 04-10-11.png (obsolete) —
Comment on attachment 8950613 [details]
Bug 1310842 - Pref with insecure only connection icons

https://reviewboard.mozilla.org/r/219884/#review228182

I think I'm generally on board with this, but see comments.

Thanks!

::: browser/app/profile/firefox.js:1339
(Diff revision 3)
>  pref("security.insecure_password.ui.enabled", true);
>  
>  // Show in-content login form warning UI for insecure login fields
>  pref("security.insecure_field_warning.contextual.enabled", true);
>  
> +// Show positive UI for https pages; disabling forces insecure_connection_* true

Hm, I can see why you want to do this, but I'd prefer not to have prefs toggle other pref's behavior without setting their values explicitly. What if you want to experiment with showing only text or only the icon while not showing secure indicators? Then you would need another patch...

::: browser/base/content/browser.js:7452
(Diff revision 3)
>        return "verifiedDomain";
>      }
>      return "unknownIdentity";
>    },
>  
> +  get secureIconEnabled() {

Please add some JSDoc

::: browser/base/content/browser.js:7453
(Diff revision 3)
>      }
>      return "unknownIdentity";
>    },
>  
> +  get secureIconEnabled() {
> +    return Services.prefs.getBoolPref("security.secure_connection_icon.enabled");

This is a good opportunity to introduce XPCOMUtils.defineLazyPreferenceGetter for all these prefs.

(We'll enable a perf test in bug 1425613 which whitelists this soon)

::: browser/base/content/browser.js:7456
(Diff revision 3)
>  
> +  get secureIconEnabled() {
> +    return Services.prefs.getBoolPref("security.secure_connection_icon.enabled");
> +  },
> +
> +  get warnOnInsecure() {

JSDoc

::: browser/base/content/browser.js:7463
(Diff revision 3)
> +           !this.secureIconEnabled ||
> +           (Services.prefs.getBoolPref("security.insecure_connection_icon.pbmode.enabled") &&
> +           PrivateBrowsingUtils.isWindowPrivate(window));
> +  },
> +
> +  get warnTextOnInsecure() {

JSDoc

::: browser/base/content/browser.js:7470
(Diff revision 3)
> +           !this.secureIconEnabled ||
> +           (Services.prefs.getBoolPref("security.insecure_connection_text.pbmode.enabled") &&
> +           PrivateBrowsingUtils.isWindowPrivate(window));
> +  },
> +
> +  treatAsInsecurePage() {

I wonder if there's a better function name for this, such as getInsecureIconLabel?

Also please document what this function does.

::: browser/base/content/browser.js:7521
(Diff revision 3)
>        this._identityBox.className = "chromeUI";
>        let brandBundle = document.getElementById("bundle_brand");
>        icon_label = brandBundle.getString("brandShorterName");
> -    } else if (this._uriHasHost && this._isEV) {
> +    } else if (this._uriHasHost && this._isEV && this.secureIconEnabled) {
>        this._identityBox.className = "verifiedIdentity";
>        if (this._isMixedActiveContentBlocked) {

You need to make this depend on this.secureIconEnabled as well.

::: browser/base/content/browser.js:7550
(Diff revision 3)
>        let extensionName = this._pageExtensionPolicy.name;
>        icon_label = gNavigatorBundle.getFormattedString(
>          "identity.extension.label", [extensionName]);
>      } else if (this._uriHasHost && this._isSecure) {
> -      this._identityBox.className = "verifiedDomain";
> +      this._identityBox.className = this.secureIconEnabled ? "verifiedDomain" : "unknownIdentity";
>        if (this._isMixedActiveContentBlocked) {

You need to make this depend on secureIconEnabled as well.

::: browser/base/content/browser.js:7574
(Diff revision 3)
> -        // pages, either already insecure or with mixed active content loaded.
> -        this._identityBox.classList.add("insecureLoginForms");
> -      }
>      }
>  
>      if (this._isCertUserOverridden) {

Not making this depend on secureIconEnabled means that overridden cert pages get a lock icon, is that intended?
Attachment #8950613 - Flags: review?(jhofmann) → review-
> I can see why you want to do this, but I'd prefer not to have prefs toggle other pref's behavior without setting their values explicitly. What if you want to experiment with showing only text or only the icon while not showing secure indicators? Then you would need another patch...

I agree with this, my other idea was to set a default if there was no insecure indicator. So on an insecure page and insecure_connection_* = false and secure_connection_icon.enabled=false then enable either the padlock or text.

My rationale for not doing this was complexity for code and users who are messing with this pref, although I don't think we ever care about the usability of our prefs.

Basically the browser should never ever mark secure and not secure the same. I don't think our prefs should allow for that, especially as we could tell users to "just flip x, to see the future"

All the other comments seem reasonable, I'll check over the "_isMixedActiveContentBlocked" content checks.

> This is a good opportunity to introduce XPCOMUtils.defineLazyPreferenceGetter for all these prefs.

Agreed, I was lazy. Thanks for slapping me for this :)
Flags: needinfo?(jhofmann)
(In reply to Jonathan Kingston [:jkt] from comment #9)
> > I can see why you want to do this, but I'd prefer not to have prefs toggle other pref's behavior without setting their values explicitly. What if you want to experiment with showing only text or only the icon while not showing secure indicators? Then you would need another patch...
> 
> I agree with this, my other idea was to set a default if there was no
> insecure indicator. So on an insecure page and insecure_connection_* = false
> and secure_connection_icon.enabled=false then enable either the padlock or
> text.
> 
> My rationale for not doing this was complexity for code and users who are
> messing with this pref, although I don't think we ever care about the
> usability of our prefs.
> 
> Basically the browser should never ever mark secure and not secure the same.
> I don't think our prefs should allow for that, especially as we could tell
> users to "just flip x, to see the future"

I think the question is: Who are we targeting with this pref? Is it meant as an easy dogfood feature for advertising to people (though I'm a bit skeptical about the concept of advertising flipping prefs in general) or is it supposed to be used for internal testing?
Flags: needinfo?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #10)
> or is it supposed to be used for internal testing?

By which I meant shield studies etc., code flipping prefs not humans.
>  think the question is: Who are we targeting with this pref?

Both, more internal studies than other users though. Tor might also use it too.

I'm still pretty reluctant to ever allow http looking the same as https in Firefox, I have seen a few people would prefer that as an option and I would rather not have pref.js files circling with this as an option. Quite frankly letting this happens removes  all security merit of the browser this could then include developers skirting around intranet issues etc and deploying it to people. (I might be being paranoid here though)
What about modifying bug 1310447 to take integer input instead, therefore:

0 - http as normal, https as secure
1 - http as insecure, https as secure
2 - http as insecure, https as normal
We have 4 prefs to control state already which makes this much more complex than the 3 states:

security.insecure_connection_icon.enabled
security.insecure_connection_icon.pbmode.enabled
security.insecure_connection_text.enabled
security.insecure_connection_text.pbmode.enabled

The non pbmode variants already override pbmode such that security.insecure_connection_icon.enabled=true and security.insecure_connection_icon.enabled=false would still make the icon insecure in private mode http. (this was the same behaviour "privacy.trackingprotection.pbmode.enable" used so I followed it).
In that case, why not just lose the pbmode preferences in order to implement them by default, therefore also gaining feature parity with Chrome (https://blog.chromium.org/2017/04/next-steps-toward-more-connection.html).

Similar case with normal preferences, they could be lost on Firefox 63 which comes almost in July (https://security.googleblog.com/2018/02/a-secure-web-is-here-to-stay.html).

Therefore eventually it could be just a simple boolean preference that defines whether https is normal or secure :)
(In reply to Madis from comment #15)
> In that case, why not just lose the pbmode preferences in order to implement
> them by default, therefore also gaining feature parity with Chrome
> (https://blog.chromium.org/2017/04/next-steps-toward-more-connection.html).
> 
> Similar case with normal preferences, they could be lost on Firefox 63 which
> comes almost in July
> (https://security.googleblog.com/2018/02/a-secure-web-is-here-to-stay.html).
> 
> Therefore eventually it could be just a simple boolean preference that
> defines whether https is normal or secure :)

To be clear, this pref we're adding is not for turning it on by default yet, this pref is for experimenting. It can be important to be flexible when testing things. We're not talking about parity or consistency with anyone here.

A safe way to treat these bugs is to assume that this pref could be removed again tomorrow without further notice just because we feel like it.
Hiding the green padlock without a draconic http/ftp warning would make http:// more valuable again. And comment 12 isn't that paranoid, I'm afraid.

bug 1067293 comment 9 looks like a/the plan.
* Firefox 61/62 (June/August; parity-Chrome): security.insecure_connection_text.enabled;true
* browser.urlbar.trimURLs;false (while doing bug 1379247) + marking http:// and ftp:// red (I haven't found a bug about that.)
* security.insecure_connection_icon.enabled;true
* (Nice to have: mark mixed content as broken and maybe use its current icon for ancient encryption, bug 942136)
* this bug: security.secure_connection_icon.enabled;false
* browser.urlbar.trimURLs;true

https://twitter.com/ericlaw/status/964631422391681025
(In reply to Jonathan Kingston [:jkt] from comment #12)
> >  think the question is: Who are we targeting with this pref?
> 
> Both, more internal studies than other users though. Tor might also use it
> too.
> 
> I'm still pretty reluctant to ever allow http looking the same as https in
> Firefox, I have seen a few people would prefer that as an option and I would
> rather not have pref.js files circling with this as an option. Quite frankly
> letting this happens removes  all security merit of the browser this could
> then include developers skirting around intranet issues etc and deploying it
> to people. (I might be being paranoid here though)

Yeah, I agree. I'd like to live in a world where we could ignore users messing with their prefs but being paranoid might not be a bad thing. I wonder if there's a more accurate name for the pref, but I can't think of any right now and I'm not blocking review on it.
The latest update replaces any insecure padlock with insecure. So no padlocks should ever be seen other than the strike through. I think this is fair in that the mixed padlock is a state that looks more secure than secure.
I didn't explicitly change anything really just reordered some of the checks in the insecure function.
Attachment #8952284 - Attachment is obsolete: true
:johannh still some work to do, just pushing a backup :)
Comment on attachment 8950613 [details]
Bug 1310842 - Pref with insecure only connection icons

https://reviewboard.mozilla.org/r/219884/#review228870


Code analysis found 9 defects in this patch:
 - 9 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/browser.js:7080
(Diff revision 4)
> +    observed: new Map([]),
> +    types: new Map([]),
> +    observe(i, j, prefName) {
> +      switch (this.types.get(prefName)) {
> +        case Ci.nsIPrefBranch.PREF_STRING:
> +          value = Services.pref.getCharPref(prefName);

Error: 'value' is not defined. [eslint: no-undef]

::: browser/base/content/browser.js:7082
(Diff revision 4)
> +    observe(i, j, prefName) {
> +      switch (this.types.get(prefName)) {
> +        case Ci.nsIPrefBranch.PREF_STRING:
> +          value = Services.pref.getCharPref(prefName);
> +        case Ci.nsIPrefBranch.PREF_INT:
> +          value = Services.prefs.getIntPref(prefName);

Error: 'value' is not defined. [eslint: no-undef]

::: browser/base/content/browser.js:7084
(Diff revision 4)
> +        case Ci.nsIPrefBranch.PREF_STRING:
> +          value = Services.pref.getCharPref(prefName);
> +        case Ci.nsIPrefBranch.PREF_INT:
> +          value = Services.prefs.getIntPref(prefName);
> +        case Ci.nsIPrefBranch.PREF_BOOL:
> +          value = Services.prefs.getBoolPref(prefName);

Error: 'value' is not defined. [eslint: no-undef]

::: browser/base/content/browser.js:7087
(Diff revision 4)
> +          value = Services.prefs.getIntPref(prefName);
> +        case Ci.nsIPrefBranch.PREF_BOOL:
> +          value = Services.prefs.getBoolPref(prefName);
> +          break;
> +        default:
> +          value = null;

Error: 'value' is not defined. [eslint: no-undef]

::: browser/base/content/browser.js:7089
(Diff revision 4)
> +          value = Services.prefs.getBoolPref(prefName);
> +          break;
> +        default:
> +          value = null;
> +      }
> +      this.observed.set(prefName, value);

Error: 'value' is not defined. [eslint: no-undef]

::: browser/base/content/browser.js:7092
(Diff revision 4)
> +          value = null;
> +      }
> +      this.observed.set(prefName, value);
> +    },
> +    uninit() {
> +      for (let [pref, val] of this.observed) {

Error: 'val' is assigned a value but never used. allowed unused vars must match /^cc|ci|cu|cr|exported_symbols/. [eslint: no-unused-vars]

::: browser/base/content/browser.js:7106
(Diff revision 4)
> +
> +    Object.defineProperty(prefObserver, propName, {
> +      get() {
> +        if (this.observed.has(prefName)) {
> +          return this.observed.get(prefName);
> +        } else {

Error: Unnecessary 'else' after 'return'. [eslint: no-else-return]

::: browser/base/content/browser.js:7645
(Diff revision 4)
> -        // It's a normal cert, verifier is the CA Org.
> +          // It's a normal cert, verifier is the CA Org.
> -        tooltip = gNavigatorBundle.getFormattedString("identity.identified.verifier",
> +          tooltip = gNavigatorBundle.getFormattedString("identity.identified.verifier",
> -                                                      [this.getIdentityData().caOrg]);
> +                                                        [this.getIdentityData().caOrg]);
> -      }
> +        }
> +      } else {
> +			  this._identityBox.className = "unknownDomain";

Error: Unexpected tab character. [eslint: no-tabs]

::: browser/base/content/browser.js:7645
(Diff revision 4)
> -        // It's a normal cert, verifier is the CA Org.
> +          // It's a normal cert, verifier is the CA Org.
> -        tooltip = gNavigatorBundle.getFormattedString("identity.identified.verifier",
> +          tooltip = gNavigatorBundle.getFormattedString("identity.identified.verifier",
> -                                                      [this.getIdentityData().caOrg]);
> +                                                        [this.getIdentityData().caOrg]);
> -      }
> +        }
> +      } else {
> +			  this._identityBox.className = "unknownDomain";

Error: Mixed spaces and tabs. [eslint: no-mixed-spaces-and-tabs]
Johann, good to go on the review whenever you have spare time :)

Try run fails all seem unrelated:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0e847d20298a74639f52823b0a6611c04a78c2a&selectedJob=164261255
Comment on attachment 8950613 [details]
Bug 1310842 - Pref with insecure only connection icons

https://reviewboard.mozilla.org/r/219884/#review229574

This looks mostly fine to me but I'm a bit unsure about the about:certerror/about:neterror changes. Maybe I'm just missing something here.

::: browser/base/content/browser.js:7154
(Diff revision 6)
> +    secureIcon: "security.secure_connection_icon.enabled",
> +    insecureIcon: "security.insecure_connection_icon.enabled",
> +    insecureIconPbmode: "security.insecure_connection_icon.pbmode.enabled",
> +    insecureText: "security.insecure_connection_text.enabled",
> +    insecureTextPbmode: "security.insecure_connection_text.pbmode.enabled",
> +    insecurePassword: "security.insecure_password.ui.enabled"

I'm hoping that once this is all over we can remove some of these prefs (at the very least the pdmode ones).

::: browser/base/content/browser.js:7530
(Diff revision 6)
> +  /**
> +   * Should show icon (strike through lock) warning for insecure pages.
> +   *
> +   * @return boolean
> +   */
> +  get warnOnInsecure() {

The name duplication with your helper function is a little irritating, so maybe call this _shouldShowInsecureIcon (with an underscore since I don't think this should be exposed to anyone else)? :)

::: browser/base/content/browser.js:7541
(Diff revision 6)
> +  /**
> +   * Should show text warning for insecure pages.
> +   *
> +   * @return boolean
> +   */
> +  get warnTextOnInsecure() {

Maybe _shouldShowInsecureText or _shouldShowTextOnInsecure?

::: browser/base/content/browser.js:7556
(Diff revision 6)
> +   * This should represent pages that should only ever be considered insecure:
> +   *   HTTP Pages, Cert overrides and net/cert errors
> +   *
> +   * @return string of icon_label
> +   */
> +  showInsecureIdentityBlock() {

Please prefix an underscore. This function is definitely not practicable without refreshIdentityBlock.

::: browser/base/content/browser.js:7654
(Diff revision 6)
>                 (gBrowser.selectedBrowser.documentURI.scheme == "about" ||
>                 gBrowser.selectedBrowser.documentURI.scheme == "chrome")) {
> -        // For net errors we should not show notSecure as it's likely confusing
> +      // For net errors we should not show notSecure as it's likely confusing
> -      this._identityBox.className = "unknownIdentity";
> +      if (gBrowser.selectedBrowser.documentURI.spec.startsWith("about:neterror") ||
> +          gBrowser.selectedBrowser.documentURI.spec.startsWith("about:certerror")) {
> +        icon_label = this.showInsecureIdentityBlock();

What's the idea here? This will mark all neterror and certerror pages as insecure. It seems like you're also asserting this in your tests, what's the reasoning behind this change?

Also, the comment needs updating.
Attachment #8950613 - Flags: review?(jhofmann)
Attached image security-indicators.png
from https://github.com/paulrouget/servoshell/issues/11#issuecomment-379511082.

I would like to propose to make security.secure_connection_icon.enabled;false directly look like a desired target state. 
The price of hiding the padlock should be to make https:// look tidy (no trimmed and beautiful http:// anymore) and to make it self-explaining regarding insecure protocols.
Comment on attachment 8950613 [details]
Bug 1310842 - Pref with insecure only connection icons

https://reviewboard.mozilla.org/r/219884/#review245800

A couple of notes, enough that I'd like to see another version of this patch.

Also note that IMO the icon state in the identity popup is pretty confusing with this patch applied in some cases , e.g. overridden certificate pages show you a broken lock on the identity block and a yellow triangle lock on the identity popup, or mixed passive content which shows a broken lock on the identity block and a regular (non-green?) lock in the identity popup...

::: browser/base/content/browser-siteIdentity.js:22
(Diff revision 7)
> + *
> + * @return object which has getters for the prefs
> + */
> +function setupPrefObserver(prefNames) {
> +  let prefObserver = {
> +  };

Nit: please make this one line

::: browser/base/content/browser-siteIdentity.js:457
(Diff revision 7)
> +   * @return boolean
> +   */
> +  _warnOnInsecure(prefPrefix) {
> +    return this._prefs[prefPrefix] ||
> +           (this._prefs[`${prefPrefix}Pbmode`] &&
> +           PrivateBrowsingUtils.isWindowPrivate(window));

nit: please indent this line one space more

::: browser/base/content/browser-siteIdentity.js:567
(Diff revision 7)
>        this._identityBox.className = "extensionPage";
>        let extensionName = this._pageExtensionPolicy.name;
>        icon_label = gNavigatorBundle.getFormattedString(
>          "identity.extension.label", [extensionName]);
>      } else if (this._uriHasHost && this._isSecure) {
> +      this._identityBox.className = this._prefs.secureIcon ? "verifiedDomain" : "unknownIdentity";

This is just duplicating what you do in the if-condition below, you can probably just remove this line...

::: browser/base/content/browser-siteIdentity.js:599
(Diff revision 7)
>      }
>  
>      if (this._isCertUserOverridden) {
> +      if (!this._prefs.secureIcon) {
> +        icon_label = this._showInsecureIdentityBlock();
> +      } else {

I don't really understand this, why is this case not handled in the huge if chain above? AFAIU this case should be handled in line 566 and below (you should probably just move the entire if-condition there).

::: browser/base/content/browser-siteIdentity.js:779
(Diff revision 7)
>      // Fill in organization information if we have a valid EV certificate.
>      if (this._isEV) {
>        let iData = this.getIdentityData();
>        host = owner = iData.subjectOrg;
> +      if (iData.country && !this._prefs.secureIcon)
> +        host += " (" + iData.country + ")";

Do you think it makes sense to do this independent of whether the secure icon pref is set?
Attachment #8950613 - Flags: review?(jhofmann) → review-

This is how I feel the security indicators should look. Top shows indicator for all HTTPS sites including EV. I feel EV text should be removed from the address bar as it conveys trust and I can list off a number of companies with terrible business practises who have EV certs. Middle shows mixed content indicator, and bottom shows negative insecure http indicator.

I think this is better solution than removing the https padlock entirely, and actually the use of colour in the three icons makes identification of the connection type pretty easy and intuitive. Also with a security indication icon showing on all sites, the information icon seems to me unnecessary (but it could still appear on FF's internal pages). This has the added benefit of simplifying the information area of the address bar nicely so that it looks neater and tidier.

(In reply to Daniel Baxter from comment #30)

Created attachment 9059775 [details]
Simple address bar security indicators

This is how I feel the security indicators should look. Top shows indicator for all HTTPS sites including EV. I feel EV text should be removed from the address bar as it conveys trust and I can list off a number of companies with terrible business practises who have EV certs. Middle shows mixed content indicator, and bottom shows negative insecure http indicator.

I think this is better solution than removing the https padlock entirely, and actually the use of colour in the three icons makes identification of the connection type pretty easy and intuitive. Also with a security indication icon showing on all sites, the information icon seems to me unnecessary (but it could still appear on FF's internal pages). This has the added benefit of simplifying the information area of the address bar nicely so that it looks neater and tidier.

Info icon will be removed per bug 1562881, so your suggestion will be partially implemented soon.

In terms of feature parity with Chrome, it is interesting to note that they still haven't implemented or announced the date for the warning symbol in all HTTP (though there is a flag for it) and no flag/exact plans for removing the lock icon. https://www.chromium.org/Home/chromium-security/marking-http-as-non-secure

You need to log in before you can comment on or make changes to this bug.