Closed Bug 846843 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox21 verified)

VERIFIED FIXED
Firefox 22
Tracking Status
firefox21 --- verified

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
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.
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)
(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").
> 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.
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+
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
Closed: 7 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?
Attachment #721305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Where can I get a look at the data in my FHR so I can confirm if this is fixed?
(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
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.