1194352 - Telemetry for percentage of forms with password fields whose form action is http/https/other

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Kate McKinley [:kmckinley, :Kate]]

Measure how often are users presented a form that submits to an https action, an http action, or a different action.

Follow on to https://bugzilla.mozilla.org/show_bug.cgi?id=1174333

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option1.patch

Building upon Kate's work in Bug 1174333, adding probes for safe/unsafe form submits.

Not sure if we should just look for form action = http vs from action = !http, or if we should also check against https.

Providing two patches here with different options for what we check the form action against:
Option 1) http vs !http
Option 2) http vs http vs (!http & !https)

If we go with option 2, we need to extend the probe to take 6 values before Bug 1174333 lands.

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option2.patch

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option1-08-18-15.patch

Rebased on top of bug 1174333.

Comment 4

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option2-08-18-15.patch

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8649405 [details] [diff] [review]
Bug1194352-option1-08-18-15.patch

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

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +144,3 @@
>      if (aForm.action.match(/^http:\/\//)) {
>        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
> +      isFormSubmitHTTP = true;

I like this simpler patch better since it aligns with when we warn developers and I'm not sure if there are non-HTTP options that we would care about as being potentially unsafe. If we cared we would be warning developers.

@@ +147,5 @@
>      }
> +
> +    // The safety of a page where we see a password field
> +    // 0=safe page, 1=safe page and unsafe form submit
> +    // 2=unsafe page, 3=unsafe page and unsafe form submit

We may want to align with the existing buckets:
0 = unsafe page and unsafe form submit
1 = safe page
…
or we can just do the date selectors on the dashboard to ignore the first few days.

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #5)
> Comment on attachment 8649405 [details] [diff] [review]
> Bug1194352-option1-08-18-15.patch
> 
> Review of attachment 8649405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +144,3 @@
> >      if (aForm.action.match(/^http:\/\//)) {
> >        this._sendWebConsoleMessage("InsecureFormActionPasswordsPresent", domDoc);
> > +      isFormSubmitHTTP = true;
> 
> I like this simpler patch better since it aligns with when we warn
> developers and I'm not sure if there are non-HTTP options that we would care
> about as being potentially unsafe. If we cared we would be warning
> developers.
>
There are non-HTTP cases we care about but we don't have a way to detect them and warn developers.  ex: form action="javascript:insert_code" or form action=javascript_method().  The javascript doesn't run until submit is hit, and by then it is too late to tell the user or developer if the destination is http.

The more complicated patch (option 2) is to categorize the unknown case.  Either we know it's safe (https), we know it's unsafe (http), or we don't know whether it is safe or not.

What do you think?



> We may want to align with the existing buckets:
> 0 = unsafe page and unsafe form submit
> 1 = safe page
> …
> or we can just do the date selectors on the dashboard to ignore the first
> few days.

I would go with the latter since it will be more clear to read and interpret the telemetry and ignoring a few days of data is not a big deal.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8649405 [details] [diff] [review]
Bug1194352-option1-08-18-15.patch

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

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +147,3 @@
>      }
> +
> +    // The safety of a page where we see a password field

Nit: s/page/form/

@@ +148,5 @@
> +
> +    // The safety of a page where we see a password field
> +    // 0=safe page, 1=safe page and unsafe form submit
> +    // 2=unsafe page, 3=unsafe page and unsafe form submit
> +    let telemetryValue;

Please name this variable something more descriptive in case we add other telemetry in this method. e.g. formSafety

Comment 8

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> What do you think?

a) It's not necessarily too late to tell the developer since console messages can be set to persist across page loads and if we don't want to rely on that we should show it on the console for the new page load.
b) We already tell the user upon submission with a modal dialog if the URL becomes http://… IIUC.
c) We probably don't want to proactively warn users on sites using javascript:… if they're on a safe page since we don't know if it's unsafe and therefore I'm not sure it's worth the added complexity to collect this.

If you really want to go with the other patch we can but I would then remove the comment describing the 6 states then since they are fairly straight-forward, defined in the json file, and takes up many lines.

Comment 9

[Tanvi Vyas[:tanvi]]

I definitely don't want to warn form javascript:.

It would be nice to know for sure how many pages are using http with https submits (protects against eavesdroppers but not MITM) vs http with potentially unsecure submits.  Hence I'd like to go with that patch.

I'll revise the option2/6 states patch with your comments and reflag you.

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option2-08-20-15.patch

Removed 6 lines of comments that contained the different states in InsecurePasswordUtils.jsm

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #7)
> Comment on attachment 8649405 [details] [diff] [review]
> Bug1194352-option1-08-18-15.patch
> 
> Review of attachment 8649405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +147,3 @@
> >      }
> > +
> > +    // The safety of a page where we see a password field
> 
> Nit: s/page/form/
> 
Changed to:
// The safety of a password field determined by the form action and the page protocol


> @@ +148,5 @@
> > +
> > +    // The safety of a page where we see a password field
> > +    // 0=safe page, 1=safe page and unsafe form submit
> > +    // 2=unsafe page, 3=unsafe page and unsafe form submit
> > +    let telemetryValue;
> 
> Please name this variable something more descriptive in case we add other
> telemetry in this method. e.g. formSafety
Changed to passwordSafety.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8650750 [details] [diff] [review]
Bug1194352-option2-08-20-15.patch

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

Please flag for data privacy review.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +139,5 @@
>        this._sendWebConsoleMessage("InsecurePasswordsPresentOnIframe", domDoc);
>        isSafePage = false;
>      }
>  
> +    bool isFormSubmitHTTP = false, isFormSubmitHTTPS = false;

I think you meant s/bool/let/

@@ +161,5 @@
> +    } else {
> +      if (isFormSubmitHTTPS) {
> +        passwordSafety = 3;
> +      } else if (isFormSubmitHTTP) {
> +        passwordSafety = 4;

You could cut this code approximately in half by using addition.

E.g. use passwordSafety += 0/1/2; and add 3 if the page is not safe though I guess maybe that is less clear without the comment.

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Bug1194352-option2-08-21-15.patch

Flagging ally for a privacy review.

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #11)
> Comment on attachment 8650750 [details] [diff] [review]
> Bug1194352-option2-08-20-15.patch
> 
> Review of attachment 8650750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please flag for data privacy review.
> 
> ::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
> @@ +139,5 @@
> >        this._sendWebConsoleMessage("InsecurePasswordsPresentOnIframe", domDoc);
> >        isSafePage = false;
> >      }
> >  
> > +    bool isFormSubmitHTTP = false, isFormSubmitHTTPS = false;
> 
> I think you meant s/bool/let/
> 
Yes, thanks! Fixed.

> @@ +161,5 @@
> > +    } else {
> > +      if (isFormSubmitHTTPS) {
> > +        passwordSafety = 3;
> > +      } else if (isFormSubmitHTTP) {
> > +        passwordSafety = 4;
> 
> You could cut this code approximately in half by using addition.
> 
> E.g. use passwordSafety += 0/1/2; and add 3 if the page is not safe though I
> guess maybe that is less clear without the comment.
This is a neat idea!  But it does make the code more complicated to read (and hence requires we add back in the 6 line comment), and also takes the same number of operations, so I'm going to leave it as is.

Comment 13

[Tanvi Vyas[:tanvi]]

Comment on attachment 8651267 [details] [diff] [review]
Bug1194352-option2-08-21-15.patch

Carrying over Matt's r+.

Comment 14

[Tanvi Vyas[:tanvi]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/84f2f517bd8bb3295b74b2fcb33d2162d4005571
changeset:  84f2f517bd8bb3295b74b2fcb33d2162d4005571
user:       Tanvi Vyas <[e-mail]>
date:       Fri Aug 28 15:51:26 2015 -0700
description:
Bug 1194352 - Add a check for insecure forms to the PWMGR_LOGIN_PAGE_SAFETY telemetry probe. r=MattN, p=ally

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84f2f517bd8b