1364208 - Insecure password warning appears for insecure form submit actions on local IP address hosts

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

We whitelist local IP addresses for showing the insecure password warning but we still flag forms on those pages that have submit actions we consider insecure, i.e everything that is not https://

We should still flag http:// form actions, but forms that just direct to other local pages are probably fine.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1364208 - Consider local IP address form actions secure for the insecure password warning.

We whitelist local IP addresses for the in-content insecure password warning,
but many of them will have forms that point to e.g., /login.php. That should
not show the insecure password warning either.

Review commit: https://reviewboard.mozilla.org/r/142564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142564/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8870994 [details]
Bug 1364208 - Consider local IP address form actions secure for the insecure password warning.

https://reviewboard.mozilla.org/r/142564/#review165006

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:91
(Diff revision 1)
>        let principal = gScriptSecurityManager.getCodebasePrincipal(uri);
>  
>        if (uri.schemeIs("http")) {
>          isFormSubmitHTTP = true;
> -        if (gContentSecurityManager.isOriginPotentiallyTrustworthy(principal)) {
> +        if (gContentSecurityManager.isOriginPotentiallyTrustworthy(principal) ||
> +            this._isPrincipalForLocalIPAddress(principal)) {

Ideally this condition should also check the form is on a local IP otherwise it would be easy for a regular website to prevent our warning by setting their @action to a local IP address and then actually submit the form to a different address using JS to change @action before submission or simply using an XHR instead: 
```js
(this._isPrincipalForLocalIPAddress(aForm.rootElement.nodePrincipal) && this._isPrincipalForLocalIPAddress(principal))
```

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8870994 [details]
Bug 1364208 - Consider local IP address form actions secure for the insecure password warning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142564/diff/1-2/

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8870994 [details]
Bug 1364208 - Consider local IP address form actions secure for the insecure password warning.

https://reviewboard.mozilla.org/r/142564/#review210298

Thanks. I wish we had tests for this but this is overdue on getting fixed to address routers so don't block on it. We were not able to use 127.0.0.1 for a mochitest?

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #4)
> Comment on attachment 8870994 [details]
> Bug 1364208 - Consider local IP address form actions secure for the insecure
> password warning.
> 
> https://reviewboard.mozilla.org/r/142564/#review210298
> 
> Thanks. I wish we had tests for this but this is overdue on getting fixed to
> address routers so don't block on it. We were not able to use 127.0.0.1 for
> a mochitest?

Thanks for the review!

I can't really remember what it was, but there was definitely an issue in retrieving the local address of the mochitest server (I had a whole patch that tried to get it using WebRTC). I asked around back then and nobody was able to help me. So unfortunately that doesn't sound possible. We could open a new bug in the appropriate component to find a way to expose this to us, it could be helpful in the future.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b53c7903818a
Consider local IP address form actions secure for the insecure password warning. r=MattN

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b53c7903818a