1217133 - Don't warn about insecure passwords on localhost

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

When password fields appear over localhost, 127.0.0.1, ipv6 ::1, we shouldn't show the lock with the strikethrough.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1133

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1825

(Note that there is also ongoing work to use a more generic helper function for this, instead of having this isSecure check all over the codebase with differing implementations - https://bugzilla.mozilla.org/show_bug.cgi?id=1162772, https://bugzilla.mozilla.org/show_bug.cgi?id=899099)

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Bug1217144-10-29-15.patch

Checks for localhost in LoginManagerContent.checkIfURIisSecure()

I don't want developers to become habituated to this warning on localhost.  Hence I'd like to land this and uplift to FF 44 asap (i.e. before Tuesday's release to dev edition).

Not that is doesn't use a generic isSecure function.  That is still being implemented in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1162772 and needs to follow the secure context spec.  Once that is done, we can change our implementation to use it, but we shouldn't be blocked on that.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8680814 [details] [diff] [review]
Bug1217144-10-29-15.patch

Review of attachment 8680814 [details] [diff] [review]:
-----------------------------------------------------------------

What about adding a comment pointing to Bug 1162772 and mentioning that this should be updated to that implementation once its available?

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8680814 [details] [diff] [review]
Bug1217144-10-29-15.patch

Review of attachment 8680814 [details] [diff] [review]:
-----------------------------------------------------------------

IMO we should fix bug 1162772 since it's pretty straightforward instead of putting in another workaround.

Comment 4

[:Paolo Amadini]

Comment on attachment 8680814 [details] [diff] [review]
Bug1217144-10-29-15.patch

Tanvi, uri.host can throw, so you should wrap in a try-catch block. And, as Brian suggested, refer to the bug that will make this API available to JavaScript.

Matt, I don't see how it could hurt to make this function more similar to the C++ version in the meantime. As far as I can see from a quick scan of the patch in bug 1162772, the API in isn't exposed to JS yet, so fixing that would definitely require more time than this implementation.

I don't think it's mandatory to fix this bug before the insecure passwords warning is released in the Developer Edition on Tuesday, but I agree we should try to have this fix in one of the first builds.

Clearing the review request, since nobody can review the patch now unless Matt's r- is lifted.

Comment 5

[Marco Mucci [:MarcoM]]

Hi Matt, would you be able to review Paolo's Comment #4 and provide feedback on it.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

I've addressed Brian's and Paolo's comments and incorporated a test that Tanvi wrote, with small tweaks to get it to pass.

Comment 7

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #4)
> Matt, I don't see how it could hurt to make this function more similar to
> the C++ version in the meantime. As far as I can see from a quick scan of
> the patch in bug 1162772, the API in isn't exposed to JS yet, so fixing that
> would definitely require more time than this implementation.

We already have the code in the tree[1] it's just a matter of moving it and using that here. I think we could easily put it somewhere like nsIDOMWindowUtils.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp?rev=b18e03d64493#1390

Comment 8

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #7)
> We already have the code in the tree[1] it's just a matter of moving it and
> using that here. I think we could easily put it somewhere like
> nsIDOMWindowUtils.

There are additional complexities involved, if the code is moved rather than copied from ServiceWorkerManager.cpp. The function you pointed to performs a full scan of the document tree, which is something different from what we do here, only asking about the properties of a single URI.

Also, this API currently doesn't give us the flexibility of choosing which URI to use for the checks, it's always the documentURI property, which could match the specification but may or may not be appropriate based on the specific security check (for example, I'm currently investigating a bug with PDF.js where we might need to check if we're using a custom content viewer and treat it specially).

Reconciling all those aspects into a single API is no trivial amount of work. And at a minimum, we would need an additional regression test for the platform bits. We may differ in goals here, but it seems to me that waiting on a full solution would delay the activation of the Insecure Login Forms Warning feature on the Developer Edition channel, for which we determined this bug is the last blocker.

Given this information, would you be opposed to landing Panos's patch, which now includes a regression test for the insecure passwords side?

Comment 9

[:Paolo Amadini]

Comment on attachment 8682101 [details] [diff] [review]
Patch v2

Review of attachment 8682101 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_insecureLoginForms.js
@@ +4,5 @@
>  // Load directly from the browser-chrome support files of login tests.
>  const testUrlPath =
>        "://example.com/browser/toolkit/components/passwordmgr/test/browser/";
> +const testUrlPath2 =
> +      "://127.0.0.1/browser/toolkit/components/passwordmgr/test/browser/";

Let's just remove "://example.com" from the test string and let each test add what's needed.

@@ +78,5 @@
> + * Checks that the insecure login forms don't warn on localhost pages
> + */
> +add_task(function* test_localhost() {
> +  // Load the a localhost page with a password field
> +  let tab = gBrowser.addTab("http" + testUrlPath2 + "form_basic.html");

No need to duplicate code here, we can have one test case that does:

for (let host of ["example.com", "127.0.0.1", "localhost"] {
  let tab =
  ...
}

And the we can have the test expect the warning only for the "example.com" case.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +1137,5 @@
>  
> +    // Is the connection to localhost? Consider localhost safe for passwords
> +    // TODO: replace the following checks with the secure context API helper
> +    // from bug 1162772 once it is fixed.
> +    let isLocalhost;

let isLocalhost = false;

@@ +1143,5 @@
> +      isLocalhost = (uri.host == "localhost" ||
> +                     uri.host == "127.0.0.1" ||
> +                     uri.host == "::1");
> +    } catch (e) {
> +      // Ignore URI schemes that throw, like about:blank.

// URI schemes that throw, like about:blank, are not considered local.

Comment 10

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #8)
> There are additional complexities involved, if the code is moved rather than
> copied from ServiceWorkerManager.cpp. The function you pointed to performs a
> full scan of the document tree, which is something different from what we do
> here, only asking about the properties of a single URI.

I'm very much aware of the current API and needs for this bug. This is trivial to resolve by changing the internal function to take an nsIURI.

> Also, this API currently doesn't give us the flexibility of choosing which
> URI to use for the checks, it's always the documentURI property, which could
> match the specification but may or may not be appropriate based on the
> specific security check (for example, I'm currently investigating a bug with
> PDF.js where we might need to check if we're using a custom content viewer
> and treat it specially).

Solved above (pdf.js can still do whatever workaround but if it's using a chrome URI can also be done easier with my approach).

> Reconciling all those aspects into a single API is no trivial amount of
> work. And at a minimum, we would need an additional regression test for the
> platform bits. We may differ in goals here, but it seems to me that waiting
> on a full solution would delay the activation of the Insecure Login Forms
> Warning feature on the Developer Edition channel, for which we determined
> this bug is the last blocker.

I never said it has to be one function call that makes the whole decision I just think that the logic related to https vs. http vs. file etc. should be centralized. We can do pre or post processing of that result as necessary.

I'm just going to implement bug 899099 myself as I guess that's the only way it will get done.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8682101 [details] [diff] [review]
Patch v2

Review of attachment 8682101 [details] [diff] [review]:
-----------------------------------------------------------------

I ended up doing this in bug 1221365 to not interfere with jwatt's WIP on the greater secure context work.

Unless bug 1221365 gets opposition I think this patch should use that new API (wherever it ends up).

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

Addressed Paolo's comments, now  waiting for bug 1221365 to use that API.

(In reply to :Paolo Amadini from comment #9)
> No need to duplicate code here, we can have one test case that does:
> 
> for (let host of ["example.com", "127.0.0.1", "localhost"] {
>   let tab =
>   ...
> }
> 
> And the we can have the test expect the warning only for the "example.com"
> case.

IMO clarity should trump efficiency in tests, since people who often have to debug one aren't necessarily experienced in that area and having a straightforward test will make their lives easier. Having said that, I did follow your suggestion with the exception of "localhost", which doesn't seem to be configured in the server used by mochitests.

Comment 13

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #11)
> I ended up doing this in bug 1221365 to not interfere with jwatt's WIP on
> the greater secure context work.

Okay, this change is more incremental than what I thought you were originally asking for, which I understood was to fix bug 1162772, including figuring out all the logic and getting rid of the checkIfURIisSecure function in this patch. With bug 1221365 we still need the checkIfURIisSecure function here but we should add the call to isURIPotentiallyTrustworthy as one of the "or" checks inside it.

I still think the initial patch by Tanvi and Panos was a valid incremental step in this direction, but bug 1221365 is a step further that doesn't indeed involve a much larger amount of work.

Comment 14

[:Paolo Amadini]

Comment on attachment 8683033 [details] [diff] [review]
Patch v3

Review of attachment 8683033 [details] [diff] [review]:
-----------------------------------------------------------------

Pre-emptive r+ with the changes below, now that bug 1221365 is fixed. Matt might want to take a look at the new patch as well.

::: browser/base/content/test/general/browser_insecureLoginForms.js
@@ +23,5 @@
>  
> +  for (let host of ["example.com", "127.0.0.1"]) {
> +    for (let scheme of ["http", "https"]) {
> +      if (host == "127.0.0.1" && scheme == "https") {
> +        continue;

Can you specify in a comment that this saves some test time, but the case should work anyways?

Other option, maybe better:

for (let [origin, expectWarning] of [
  ["http://example.com", true],
  ["http://127.0.0.1", false],
  ["https://example.com", false],
]) {

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +1146,5 @@
> +    } catch (e) {
> +      // URI schemes that throw, like about:blank, are not considered local.
> +    }
> +
> +    if (isLocalhost ||

Define a lazy service getter at the top of the module, and call the new helper function in the "if" statement, like in the test case:

https://reviewboard.mozilla.org/r/24243/diff/1#0

Note that I think the lazy service getter will not incur in the performance issues that the lazy module getter had, because it shouldn't cause I/O.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

Switched to the ContentSecurityManager.isURIPotentiallyTrustworthy API and addressed Paolo's comments.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8685412 [details] [diff] [review]
Patch v4

Review of attachment 8685412 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 17

[Tanvi Vyas[:tanvi]]

Comment on attachment 8685412 [details] [diff] [review]
Patch v4

>@@ -1125,17 +1129,19 @@ var LoginManagerContent = {
>-    if (netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
>+    // Is the connection to localhost? Consider localhost safe for passwords.
>+    if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri) ||
>+        netutil.URIChainHasFlags(uri, ph.URI_IS_LOCAL_RESOURCE) ||
>         netutil.URIChainHasFlags(uri, ph.URI_DOES_NOT_RETURN_DATA) ||
>         netutil.URIChainHasFlags(uri, ph.URI_INHERITS_SECURITY_CONTEXT) ||
>         netutil.URIChainHasFlags(uri, ph.URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT)) {
> 
>       isSafe = true;
>     }
> 
As a followup, we should revisit the URI flags used here and update checkIfURIisSecure().

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Try run was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40099a43e6b9

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7371cdd91ff5

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7371cdd91ff5

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8685412 [details] [diff] [review]
Patch v4

Approval Request Comment
[Feature/regressing bug #]: bug 1179961
[User impact if declined]: we won't be able to enable insecure Http password warnings in 44, because localhost will behave unexpectedly
[Describe test coverage new/current, TreeHerder]: mochitest included
[Risks and why]: minor risk, feature is currently preffed off
[String/UUID change made/needed]: none

Comment 22

[Paul Silaghi, QA [:pauly]]

I managed to setup over my ipv4 an Apache server which listens on ipv6 and then reproduced the problem.
Verified fixed on FF 45.0a1 (2015-11-15) Win 7.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Panos, when reviewing the patch, I noticed that there isn't really a noticeable change in the idl file but it still shows up in the patch. Is that expected?

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

It's just a typo fix in a comment, it doesn't affect anything. Do you want me to remove that part from the aurora patch?

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Panos Astithas [:past] from comment #24)
> It's just a typo fix in a comment, it doesn't affect anything. Do you want
> me to remove that part from the aurora patch?

Nope, it's fine. I could not spot the typo earlier which I do now so might as well take it. :)

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685412 [details] [diff] [review]
Patch v4

Adding logic to handle localhost pages when looking for insecure passwords. Seems like a good idea. Let's uplift to Aurora44.

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e792fb316193

Comment 28

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 44.0a2 (2015-11-18) Win 7

Comment 29

[Dimas]

What's considered a localhost address? Since version 52 I see that in our localhost forms: https://bugzilla.mozilla.org/attachment.cgi?id=8830183
It happens on a localhost address: http://zeus.cpd1.grupics.intranet/glpi/index.php

Comment 30

[Matthew N. [:MattN]]

See https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/dom/security/nsContentSecurityManager.cpp#716-721

This is defined in the Secure Contexts spec: https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin