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

Status: VERIFIED FIXED

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

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

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...

Comment 2

[Daniel Einspanjer [:dre] [:deinspanjer]]

I'd be most happy to see something like a basename function applied to any possible area that paths could show up.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Scrub profile directory from error strings, v1

This should remove traces of profile directories in the error strings.

Comment 4

[Richard Newman [: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!

Comment 5

[Gregory Szorc [:gps]]

(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.

Comment 6

[Richard Newman [:rnewman]]

(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

[Benjamin Smedberg]

> 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.

Comment 8

[Richard Newman [:rnewman]]

(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.

Comment 9

[Gregory Szorc [:gps]]

Attachment: 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 :)

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fa67fd3bbb44

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 721305 [details] [diff] [review]
Scrub profile directory from error strings, v2

Scrubs the patch from Bug 845431; important privacy win.

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2baf49a3b9cc

Comment 14

[u279076]

Where can I get a look at the data in my FHR so I can confirm if this is fixed?

Comment 15

[Gregory Szorc [:gps]]

(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.

Comment 16

[u279076]

Okay, thank you Gregory. Marking this verified fixed.