Closed Bug 1194352 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: kmckinley, Assigned: tanvi)

References

Details

Attachments

(1 file, 5 obsolete files)

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
Depends on: 1174333
Attached patch Bug1194352-option1.patch (obsolete) — Splinter Review
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.
Attached patch Bug1194352-option2.patch (obsolete) — Splinter Review
Rebased on top of bug 1174333.
Attachment #8648147 - Attachment is obsolete: true
Attachment #8649405 - Flags: review?(MattN+bmo)
Attachment #8648148 - Attachment is obsolete: true
Attachment #8649406 - Flags: review?(MattN+bmo)
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.
Attachment #8649405 - Flags: review?(MattN+bmo) → review+
Assignee: nobody → tanvi
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(MattN+bmo)
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
(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.
Flags: needinfo?(MattN+bmo)
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.
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.
Attachment #8649405 - Attachment is obsolete: true
Attachment #8649406 - Attachment is obsolete: true
Attachment #8650750 - Flags: review?(MattN+bmo)
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.
Attachment #8650750 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8650750 - Attachment is obsolete: true
Attachment #8651267 - Flags: review?(ally)
Comment on attachment 8651267 [details] [diff] [review]
Bug1194352-option2-08-21-15.patch

Carrying over Matt's r+.
Attachment #8651267 - Flags: review+
Attachment #8651267 - Flags: review?(ally) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/84f2f517bd8bb3295b74b2fcb33d2162d4005571
changeset:  84f2f517bd8bb3295b74b2fcb33d2162d4005571
user:       Tanvi Vyas <tanvi@mozilla.com>
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
https://hg.mozilla.org/mozilla-central/rev/84f2f517bd8b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.