Closed
Bug 846843
Opened 12 years ago
Closed 12 years ago
Profile directory (containing PII) may be submitted in Firefox Health Report errors
Categories
(Firefox Health Report Graveyard :: Client: Desktop, defect)
Firefox Health Report Graveyard
Client: Desktop
Tracking
(firefox21 verified)
VERIFIED
FIXED
Firefox 22
Tracking | Status | |
---|---|---|
firefox21 | --- | verified |
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(1 file, 1 obsolete file)
4.20 KB,
patch
|
rnewman
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 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...
Comment 2•12 years ago
|
||
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•12 years ago
|
||
This should remove traces of profile directories in the error strings.
Comment 4•12 years ago
|
||
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•12 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.
Comment 6•12 years ago
|
||
(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•12 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.
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #721305 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 10•12 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
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 12•12 years ago
|
||
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•12 years ago
|
Attachment #721305 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•12 years ago
|
||
status-firefox21:
--- → fixed
Comment 14•12 years ago
|
||
Where can I get a look at the data in my FHR so I can confirm if this is fixed?
Assignee | ||
Comment 15•12 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.
Comment 16•12 years ago
|
||
Okay, thank you Gregory. Marking this verified fixed.
Status: RESOLVED → VERIFIED
Updated•12 years ago
|
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Updated•6 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•