Don't warn about insecure passwords on localhost

VERIFIED FIXED in Firefox 44

Status

()

Firefox
Security
P1
normal
VERIFIED FIXED
2 years ago
4 months ago

People

(Reporter: tanvi, Assigned: past)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

44 Branch
Firefox 45
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox44 verified, firefox45 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
Depends on: 1179961, 899099, 1162772
See Also: → bug 1217140
(Reporter)

Updated

2 years ago
Blocks: 1217142

Updated

2 years ago
Whiteboard: [fxprivacy] → [fxprivacy] [triage]

Updated

2 years ago
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(Reporter)

Comment 1

2 years ago
Created attachment 8680814 [details] [diff] [review]
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.
Attachment #8680814 - Flags: review?(paolo.mozmail)
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 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.
Attachment #8680814 - Flags: review-

Comment 4

2 years ago
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.
Attachment #8680814 - Flags: review?(paolo.mozmail)
Hi Matt, would you be able to review Paolo's Comment #4 and provide feedback on it.
Flags: needinfo?(MattN+bmo)
Created attachment 8682101 [details] [diff] [review]
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.
Attachment #8682101 - Flags: review?(paolo.mozmail)
Attachment #8682101 - Flags: review?(MattN+bmo)
Attachment #8680814 - Attachment is obsolete: true
Assignee: nobody → past
Status: NEW → ASSIGNED

Updated

2 years ago
Iteration: --- → 45.1 - Nov 16
Flags: qe-verify?
Priority: P2 → P1
(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
Flags: needinfo?(MattN+bmo)

Comment 8

2 years ago
(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?

Updated

2 years ago
Flags: needinfo?(MattN+bmo)

Comment 9

2 years ago
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.
Attachment #8682101 - Flags: review?(paolo.mozmail)
(Reporter)

Updated

2 years ago
Blocks: 1221206
(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.
Flags: needinfo?(MattN+bmo)
Depends on: 1221365
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).
Attachment #8682101 - Flags: review?(MattN+bmo) → review-
Created attachment 8683033 [details] [diff] [review]
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.
Attachment #8682101 - Attachment is obsolete: true

Comment 13

2 years ago
(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

2 years ago
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.
Attachment #8683033 - Flags: review+
Created attachment 8685412 [details] [diff] [review]
Patch v4

Switched to the ContentSecurityManager.isURIPotentiallyTrustworthy API and addressed Paolo's comments.
Attachment #8685412 - Flags: review?(MattN+bmo)
Attachment #8683033 - Attachment is obsolete: true
Comment on attachment 8685412 [details] [diff] [review]
Patch v4

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

Thanks!
Attachment #8685412 - Flags: review?(MattN+bmo) → review+

Updated

2 years ago
Blocks: 1188121
No longer blocks: 1217142
(Reporter)

Updated

2 years ago
Blocks: 1217142
(Reporter)

Comment 17

2 years ago
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().
Try run was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40099a43e6b9

Comment 19

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/7371cdd91ff5
https://hg.mozilla.org/mozilla-central/rev/7371cdd91ff5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
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
Attachment #8685412 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
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?
Flags: needinfo?(past)
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?
Flags: needinfo?(past)
(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 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.
Attachment #8685412 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 27

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e792fb316193
status-firefox44: affected → fixed
Verified fixed FF 44.0a2 (2015-11-18) Win 7
status-firefox44: fixed → verified
Depends on: 1261234

Comment 29

4 months ago
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
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
You need to log in before you can comment on or make changes to this bug.