1261234 - Insecure form action password form warning appears for trustworthy action URLs using HTTP

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Matthew N. [:MattN]]

Attachment: Test case that shouldn't have a warning about the form action

Bug 1217133 fixed some insecure password warnings so they don't affect "trustworthy" origins like localhost/127.0.0.1/etc. but the form action check still uses a regex on the scheme alone meaning that any HTTP (not HTTPS) origin that is secure will still cause the warning. This only affects sites that use the full URL in the action attribute.

The inaccurate code at https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm?rev=5ed6d4e80064#91 should resolve the action relative to the form base URI and then use isURIPotentiallyTrustworthy

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Hey Matthew, I would like to take this follow-up bug for Bug 1191092. Thanks! :)

Comment 2

[Matthew N. [:MattN]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> Hey Matthew, I would like to take this follow-up bug for Bug 1191092.

Great, let me know if you need more information to begin.

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Attachment: 0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

Hi Matthew,

Could you help to give a feedback?

I only use isURIPotentiallyTrustworthy for replacing the regext of HTTP.
For HTTPS, I suppose that the variable 'isFormSubmitHTTPS' is for checking HTTPS only.
Based on isURIPotentiallyTrustworthy test case [1], it is possible to be true for a non-https URI. So I leave the original design there.

Thank you!

(I will rebase the patch once the final patch of Bug 1191092 is confirmed.)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/unit/test_isURIPotentiallyTrustworthy.js#24-25

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8738383 [details] [diff] [review]
0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

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

Could you please include 8 lines of context in your patch files (I didn't have enough context to review hunk 3 from splinter)?

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +98,4 @@
>  
>      let isFormSubmitHTTP = false, isFormSubmitHTTPS = false;
>      if (aForm.rootElement instanceof Ci.nsIDOMHTMLFormElement) {
> +      let uri = NetUtil.newURI(aForm.action);

I think you need to use the 3rd argument to newURI to "resolve the action relative to the form base URI" (aForm.baseURI) (as I said in comment 0) otherwise you're going to have a problem with action="" and maybe action="/login". Can you please add a test for examples like these?

If you rebase this patch on m-c you will see I added a comment a few weeks ago about the relative paths possible.

It also mentions another change that this will cause which is that after this patch we will warn about an HTTP action even if we already warned that the form itself being on a HTTP page. I think we should try to preserve the old behaviour to:
1) Not have double-warnings for forms on HTTP that also submit to HTTP (e.g. relative action URL)
2) Not skew the telemetry meaning.

Comment 5

[Tanvi Vyas[:tanvi]]

This patch may affect the telemetry collected here:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#98

Comment 6

[Tanvi Vyas[:tanvi]]

Comment on attachment 8738383 [details] [diff] [review]
0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

Form action is tricky.  Just because it is !isURIPotentiallyTrustworthy, doesn't mean that it is insecure.  What if the form action is javascript:method() where the method ends up posting to https?  Then it will fail the isURIPotentiallyTrustworthy check even though it is still secure.  I think you need something like the following:

    let isFormSubmitHTTP = false, isFormSubmitSecure = false;
    // Note that aForm.action can be a relative path (e.g. "", "/login", "//example.com", etc.)
    // but we don't warn about those since we would have already warned about the form's document
    // not being safe above.
    let uri = NetUtil.newURI(aForm.action);
    if (aForm.action.match(/^http:\/\//)) {
      if (host.Equals("127.0.0.1") || host.Equals("localhost") || host.Equals("::1")) {
        isFormSubmitSecure = true;
      } else {
        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
        isFormSubmitHTTP = true;
      }
    } else if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri) {
      isFormSubmitSecure = true;
    }

    // The safety of a password field determined by the form action and the page protocol
    let passwordSafety;
    if (isSafePage) {
      if (isFormSubmitSecure) {
        passwordSafety = 0;
      } else if (isFormSubmitHTTP) {
        passwordSafety = 1;
      } else {
        passwordSafety = 2;
      }
    } else {
      if (isFormSubmitHTTPS) {
        passwordSafety = 3;
      } else if (isFormSubmitHTTP) {
        passwordSafety = 4;
      } else {
        passwordSafety = 5;
      }
    }

    Services.telemetry.getHistogramById("PWMGR_LOGIN_PAGE_SAFETY").add(passwordSafety);

And then you may also need to change the description of the telemetry probe.  Or maybe we need to break out the localhost part of IsURIPotentiallyTrustworthy and call that directly here.

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Attachment: 0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

Hi Matthew,

For "resolve the action relative to the form base URI", using aForm.action get a full URL includes host part even it's a relative path in action.

If I understand your comments correctly, the two form actions have the same host should popup warning message once. aForm.action will get the host part only, so only one warning message will be shown even there are two forms.

The full action URL can be got from aForm.rootElement.action.

Could you give a feedback for my above comment and patch?
Thank you.


Hi Tanvi,

I've rebased your suggestion to the latest code since Bug 1191092 is landed. Could you give a feedback here? Thank you.

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8742227 [details] [diff] [review]
0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

Thanks Sean!  Looks like you incorporated my suggestions in comment 6.  This looks good to me.  I leave it up to Matt to see if he is okay with this or if he prefers to pull out the localhost checks into a separate method.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8742227 [details] [diff] [review]
0001-Bug-1261234-Use-isURIPotentiallyTrustworthy-to-verif.patch

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

See my new patch coming up that addresses the issues below.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +100,5 @@
>      if (aForm.rootElement instanceof Ci.nsIDOMHTMLFormElement) {
>        // Note that aForm.action can be a relative path (e.g. "", "/login", "//example.com", etc.)
>        // but we don't warn about those since we would have already warned about the form's document
>        // not being safe above.
> +      let uri = NetUtil.newURI(aForm.action);

This was a mistake in Tanvi's code as it's still not resolving relation form actions IIUC.

@@ +106,2 @@
>        if (aForm.action.match(/^http:\/\//)) {
> +        if (host === "127.0.0.1" || host === "localhost" || host === "::1") {

I don't think we should duplicate these checks from isURIPotentiallyTrustworthy, we can instead just use that method here

Comment 10

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy when checking for insecure form actions.

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

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8746262 [details]
MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy when checking for insecure form actions.

Please check that this still makes sense (as I did it quickly). Tanvi, could you please review the JSM. The test still needs work.
Sean, could you address an issues you or Tanvi find and address the test TODOs?

Thanks

Comment 12

[Tanvi Vyas[:tanvi]]

Comment on attachment 8746262 [details]
MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy when checking for insecure form actions.

https://reviewboard.mozilla.org/r/49345/#review47087

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:110
(Diff revision 1)
> +      if (uri.schemeIs("http")) {
> +        isFormSubmitHTTP = true;
> +        if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri)) {
> +          isFormSubmitSecure = true;
> +        } else if (isSafePage) {
> +          // Only warn about the action if we didn't already warn about the form being insecure.

We should warn about both.  No point in a developer fixing one issue, only to find another.  So you can remove the "if (isSafePage)" from the else.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:114
(Diff revision 1)
> -        this._formRootsWarned.set(aForm.rootElement, true);
> +          this._formRootsWarned.set(aForm.rootElement, true);
> -        isFormSubmitHTTP = true;
> -      } else if (aForm.action.match(/^https:\/\//)) {
> -        isFormSubmitHTTPS = true;
> -      }
> +        }
> +      } else {

This should be an:

else if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri)) {
  isFormSubmitSecure=true;
}

Comment 13

[Sean Lee [:seanlee][:weilonge]]

gContentSecurityManager.isURIPotentiallyTrustworthy is changed at bug 1267509, and I am rebasing that to the new one.
HG commit: https://hg.mozilla.org/mozilla-central/rev/915ddad13087

Comment 14

[Sean Lee [:seanlee][:weilonge]]

https://reviewboard.mozilla.org/r/49345/#review47199

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:104
(Diff revision 1)
>      if (aForm.rootElement instanceof Ci.nsIDOMHTMLFormElement) {
>        // Note that aForm.action can be a relative path (e.g. "", "/login", "//example.com", etc.)
>        // but we don't warn about those since we would have already warned about the form's document
>        // not being safe above.
> -      if (aForm.action.match(/^http:\/\//)) {
> +      let baseURI = Services.io.newURI(aForm.rootElement.baseURI, null, null);
> +      let uri = Services.io.newURI(aForm.action, null, baseURI);

Is there any difference between Services.io.newURI and NetUtil.newURI for this case?

aForm.rootElement.action provides a full link like this:
```
http://10.247.100.200:8000/toolkit/components/passwordmgr/test/browser/hello.html
```
which is for the form (```<form id="form-basic" action="./hello.html">```)

So we can use this line to simplify it:
```
let uri = NetUtil.newURI(aForm.rootElement.action);
```

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:107
(Diff revision 1)
>        // not being safe above.
> -      if (aForm.action.match(/^http:\/\//)) {
> +      let baseURI = Services.io.newURI(aForm.rootElement.baseURI, null, null);
> +      let uri = Services.io.newURI(aForm.action, null, baseURI);
> +      if (uri.schemeIs("http")) {
> +        isFormSubmitHTTP = true;
> +        if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri)) {

Is isOriginPotentiallyTrustworthy a good alternative since isURIPotentiallyTrustworthy is removed?

I would like to use [LoginManagerContent.isDocumentSecure](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1128) as an example to rewrite it.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8746262 [details]
MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy when checking for insecure form actions.

Hi Matthew,

I will prepare a patch based on my previous comment in your patch. I found there are some codes could be removed like bug 1269568 mentioned. Do you think we can do that in this patch?

Comment 16

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49345/#review47087

> We should warn about both.  No point in a developer fixing one issue, only to find another.  So you can remove the "if (isSafePage)" from the else.

I'm partly concerned that developers won't realize the errors are different and fix only the action without testing again.

In order for your suggestion to matter then the page would have to be HTTP and the action would have to be absolute with the HTTP scheme. For pages using an action without the scheme (which I would suspect is most common), fixing the warning about the page being HTTP will automatically fix the action. I think we shouldn't warn twice about the same problem which is what we would do with your suggestion if the page is using a non-absolute URL. Before my patch we didn't do the double-warning either.

> This should be an:
> 
> else if (gContentSecurityManager.isURIPotentiallyTrustworthy(uri)) {
>   isFormSubmitSecure=true;
> }

Why? Are there other schemes we want to warn for? I think that will cause problems for "javascript:…". I think this can be handled in a separate bug since it's orthogonal to what this bug is about.

Comment 17

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49345/#review47199

> Is there any difference between Services.io.newURI and NetUtil.newURI for this case?
> 
> aForm.rootElement.action provides a full link like this:
> ```
> http://10.247.100.200:8000/toolkit/components/passwordmgr/test/browser/hello.html
> ```
> which is for the form (```<form id="form-basic" action="./hello.html">```)
> 
> So we can use this line to simplify it:
> ```
> let uri = NetUtil.newURI(aForm.rootElement.action);
> ```

> Is there any difference between Services.io.newURI and NetUtil.newURI for this case?

No, so I switched to Services.io to avoid the import.

> aForm.rootElement.action provides a full link like this…

Oh, hmm… you're right… I know there was some code that was getting the action without resolving it before so I was confused. Maybe I was thinking of old code that did getAttribute("action") which wouldn't be resolved. Sorry about that.

> Is isOriginPotentiallyTrustworthy a good alternative since isURIPotentiallyTrustworthy is removed?
> 
> I would like to use [LoginManagerContent.isDocumentSecure](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#1128) as an example to rewrite it.

> Is isOriginPotentiallyTrustworthy a good alternative since isURIPotentiallyTrustworthy is removed?

Yes, that's what we need to use now for the action.

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Attachment: MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy to verify the form action.; r?MattN

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

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8749089 [details]
MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy to verify the form action.; r?MattN

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

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Hi Matthew,

Based on your patch, my patch is added more cases to verify the different type of form actions.
Please help to review it! Thanks. :)

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8749089 [details]
MozReview Request: Bug 1261234 - Use isURIPotentiallyTrustworthy to verify the form action.; r?MattN

https://reviewboard.mozilla.org/r/50747/#review48899

Thanks for the additional tests and apologies for the delay again.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/854bceba6286

Comment 23

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/50747/#review48903

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:103
(Diff revision 2)
>        isSafePage = false;
>      }
>  
> -    let isFormSubmitHTTP = false, isFormSubmitHTTPS = false;
> +    let isFormSubmitHTTP = false, isFormSubmitSecure = false;
>      if (aForm.rootElement instanceof Ci.nsIDOMHTMLFormElement) {
> -      // Note that aForm.action can be a relative path (e.g. "", "/login", "//example.com", etc.)
> +      let uri = Services.io.newURI(aForm.rootElement.action, null, null);

Actually, I ran the test and noticed it was spewing errors because of the issue I mentioned but the problem is only for when the action atrribute is missing like in form_basic.html. I'll push a follow-up

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6581bbe179b9

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/854bceba6286
https://hg.mozilla.org/mozilla-central/rev/6581bbe179b9