Profile directory (containing PII) may be submitted in Firefox Health Report errors

VERIFIED FIXED in Firefox 21

Status

Firefox Health Report
Client: Desktop
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 22
Points:
---

Firefox Tracking Flags

(firefox21 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
FHR has the ability to self-report errors in its payload. It's a very handy feature!

We recently expanded it a little bit in bug 845431 to send more errors because those it was sending weren't very useful. We thought there would be no private data leakage. However, one slipped through.

Preliminary data obtained in bug 846604 reveals that the CrashesProvider is leaking the profile directory. The profile directory contains the username, which is PII. I'm pretty sure we can't have that in FHR payloads without user consent.

Currently, the known scope of the problem is limited to Nightly builds starting with 2013-02-28 that have FHR enabled and that encounter a specific error (bug 846839) resulting from the profile directory being on a network share.

Please advise on a course of action.
Flags: needinfo?(mconnor)
(Assignee)

Comment 1

5 years ago
As explained in bug 845431, the only user data we thought could be in the errors would be things already in FHR's world and thus already consented to submission.

Obviously the profile directory isn't part of this.

How do people feel about a search and replace of the profile directory string as a way to work around this discovered exception? I'm trying to think if there are any other paths that may creep in. I don't think there are...
I'd be most happy to see something like a basename function applied to any possible area that paths could show up.
(Assignee)

Comment 3

5 years ago
Created attachment 720926 [details] [diff] [review]
Scrub profile directory from error strings, v1

This should remove traces of profile directories in the error strings.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #720926 - Flags: review?(rnewman)
Comment on attachment 720926 [details] [diff] [review]
Scrub profile directory from error strings, v1

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

::: services/healthreport/healthreporter.jsm
@@ +596,5 @@
>  
> +    // Scrub out potentially identifying information from strings that could
> +    // make the payload.
> +    let appData = Services.dirsvc.get("UAppData", Ci.nsIFile).path;
> +    let profile = Services.dirsvc.get("ProfD", Ci.nsIFile).path;

Doesn't this rely on the same path string format (UNC? Unix? file:///?) for the input data?

Perhaps we should be looking for "abc123487.default" in the input, and killing from preceding unescaped whitespace up to and including that region... or instead computing any of the various kinds of paths and replacing them all. 

Hard to be thorough here.

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +550,5 @@
> +add_task(function test_error_message_scrubbing() {
> +  let reporter = yield getReporter("error_message_scrubbing");
> +
> +  let profile = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
> +  reporter._recordError("Foo " + profile);

I don't believe this test adequately covers the real-world issue, for the aforementioned reason!
Attachment #720926 - Flags: review?(rnewman)
(Assignee)

Comment 5

5 years ago
(In reply to Richard Newman [:rnewman] from comment #4)
> ::: services/healthreport/healthreporter.jsm
> @@ +596,5 @@
> >  
> > +    // Scrub out potentially identifying information from strings that could
> > +    // make the payload.
> > +    let appData = Services.dirsvc.get("UAppData", Ci.nsIFile).path;
> > +    let profile = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
> 
> Doesn't this rely on the same path string format (UNC? Unix? file:///?) for
> the input data?

Yes. Your point?

> Perhaps we should be looking for "abc123487.default" in the input, and
> killing from preceding unescaped whitespace up to and including that
> region... or instead computing any of the various kinds of paths and
> replacing them all. 

Paths with spaces! Are you suggesting I also perform substitutions on the .replace("/", "\\", "g") variations?
 
> Hard to be thorough here.

Indeed. I'm pretty sure this will catch the only offender we know of because the UNC \\ paths are coming from "UAppData" and are fed directly into OS.File (which is throwing the error).

> ::: services/healthreport/tests/xpcshell/test_healthreporter.js
> @@ +550,5 @@
> > +add_task(function test_error_message_scrubbing() {
> > +  let reporter = yield getReporter("error_message_scrubbing");
> > +
> > +  let profile = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
> > +  reporter._recordError("Foo " + profile);
> 
> I don't believe this test adequately covers the real-world issue, for the
> aforementioned reason!

What other tests would you like? Let's design the tests first then implement to pass them.
(In reply to Gregory Szorc [:gps] from comment #5)

> > Doesn't this rely on the same path string format (UNC? Unix? file:///?) for
> > the input data?
> 
> Yes. Your point?

My concern is stuff like:

    let dir = FileUtils.getDir("ProfD", ["weave", "logs"], true);
    let uri = Services.io.newFileURI(dir);
    let channel = Services.io.newChannelFromURI(uri);

-- there are places in our code where directories are turned into URIs. If you're only removing 

  \\foo\bar\baz

and the exception contains

  file:///foo/bar/baz

then your code isn't complete.

> Indeed. I'm pretty sure this will catch the only offender we know of because
> the UNC \\ paths are coming from "UAppData" and are fed directly into
> OS.File (which is throwing the error).

Yeah, I just don't want to play whack-a-mole here by skimming through user reports.
 

> What other tests would you like? Let's design the tests first then implement
> to pass them.

Let's start with a file URI variant, and also just the bare profile directory name ("couldn't open profile directory abcdef.12345").

Comment 7

5 years ago
> Let's start with a file URI variant, and also just the bare profile
> directory name ("couldn't open profile directory abcdef.12345").

I don't understand this comment. The random profile salting of that name is an important part of our security system, and we should not be including the salt directory name in FHR reports.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> > Let's start with a file URI variant, and also just the bare profile
> > directory name ("couldn't open profile directory abcdef.12345").
> 
> I don't understand this comment. The random profile salting of that name is
> an important part of our security system, and we should not be including the
> salt directory name in FHR reports.

That is exactly this bug: we *currently* include exception strings that can include the profile path.

Greg's first iteration removed all instances of UAppData and ProfD as native paths from the exception strings, so an exception like

  Couldn't open file c:\Users\rnewman\…\foo.default\bar.json

would be sanitized to

  Couldn't open file <profile>\bar.json

I'm suggesting he also remove file URI versions and the bare profile name of those directories from exception strings, because some parts of the codebase don't operate on native paths, and thus would still leak through.
(Assignee)

Comment 9

5 years ago
Created attachment 721305 [details] [diff] [review]
Scrub profile directory from error strings, v2

Handles URI variants and the special case where UAppDir is beneath ProfD (which happens in our test code because UAppDir is created as part of running the individual xpcshell test).

I /think/ this should be good enough. Surely this is better than nothing :)
Attachment #720926 - Attachment is obsolete: true
Attachment #721305 - Flags: review?(rnewman)
Attachment #721305 - Flags: review?(rnewman) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa67fd3bbb44

I'm also opening up this bug because we didn't post confidential information like I thought we might. I see no reason for it to remain hidden from public view.
Group: mozilla-corporation-confidential
Flags: needinfo?(mconnor)
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/fa67fd3bbb44
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 721305 [details] [diff] [review]
Scrub profile directory from error strings, v2

Scrubs the patch from Bug 845431; important privacy win.
Attachment #721305 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #721305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2baf49a3b9cc
status-firefox21: --- → fixed
Where can I get a look at the data in my FHR so I can confirm if this is fixed?
(Assignee)

Comment 15

5 years ago
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #14)
> Where can I get a look at the data in my FHR so I can confirm if this is
> fixed?

The data is stored in Hadoop. I pulled the payloads 2 days ago and verified this fix is good.
Okay, thank you Gregory. Marking this verified fixed.
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified

Updated

5 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
You need to log in before you can comment on or make changes to this bug.