Closed
Bug 1272166
Opened 8 years ago
Closed 8 years ago
Telemetry logging can potentially leak user information
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox47 fixed, firefox48 fixed, firefox49 fixed, fennec47+)
RESOLVED
FIXED
Firefox 49
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
(Keywords: csectype-disclosure, sec-low, Whiteboard: [adv-main47-] local log leakage)
Attachments
(1 file)
1.79 KB,
patch
|
Margaret
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
I was writing a patch for bug 1261617, to add UI telemetry for search events, where the extra was the search engine identifier. This can reveal the type of content a user is searching for (e.g. diamond ring search engines). While this is one example, it's unclear what the extras field is being used for in other probes. I propose only logging the extras in local builds and always logging everything else. fwiw, depending on the probes we decide to add in the future, how the user interacts with the UI could also be considered a privacy leak – but this bug is not to contest that.
Assignee | ||
Comment 1•8 years ago
|
||
MozReview-Commit-ID: Gzb2qdm3TuT
Assignee | ||
Updated•8 years ago
|
Attachment #8751524 -
Flags: review?(margaret.leibovic)
Comment 2•8 years ago
|
||
I'll also need to address this in bug 1268104, since I'm changing this logging there. It might not be worth landing this, if we're can land my patch and uplift it.
Updated•8 years ago
|
Attachment #8751524 -
Flags: review?(margaret.leibovic) → review+
Comment 3•8 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #0) > I was writing a patch for bug 1261617, to add UI telemetry for search > events, where the extra was the search engine identifier. This can reveal > the type of content a user is searching for (e.g. diamond ring search > engines). > > While this is one example, it's unclear what the extras field is being used > for in other probes. > > I propose only logging the extras in local builds and always logging > everything else. > > fwiw, depending on the probes we decide to add in the future, how the user > interacts with the UI could also be considered a privacy leak – but this bug > is not to contest that. I'm not sure what the problem you are fixing actually is. There can be a privacy leak when logging the identifier of a user-installed plugin that has an obvious name, but logging the identifier of search plugins that are shipped with the browser shouldn't be a problem, right?
Comment 4•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #3) > There can be a > privacy leak when logging the identifier of a user-installed plugin that has > an obvious name, but logging the identifier of search plugins that are > shipped with the browser shouldn't be a problem, right? Actually, the identifier (if we are talking about nsISearchEngine.identifier) is null for user installed engines: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=ce2501a5f268#2154
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #3) > I'm not sure what the problem you are fixing actually is. There can be a > privacy leak when logging the identifier of a user-installed plugin that has > an obvious name, but logging the identifier of search plugins that are > shipped with the browser shouldn't be a problem, right? The problem I'm trying to solve is general – UI telemetry extras can store arbitrary Strings as extras and developers don't realize that these will be logged publicly into the phone's logs. Sometimes these can contain user data (e.g. my task below). In my specific observation (when writing a patch for bug 1261617), we set the extra as SearchEngine.getEngineIdentifier which apparently uses the name if the identifier is null [1] (as is the case with the custom search engines), revealing the name of the search engine (e.g. in the test I just did, it'd be "other-Kotaku - The Gamer's Guide"). The patch in this bug prevents us from logging the extra publicly in the logs on device, not from sending it up to the remote telemetry servers. [1]: https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/mobile/android/base/java/org/mozilla/gecko/home/SearchEngine.java#66
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8751524 [details] [diff] [review] Update UI telemetry logging @Margaret: I think security usually has us land on all branches at once so you might need to build your patch on top of this draft. Approval Request Comment [Feature/regressing bug #]: Unknown [User impact if declined]: Users can leak personal data through UI telemetry logging [Describe test coverage new/current, TreeHerder]: Tested locally. Note that this requires the MOZILLA_OFFICIAL flag, expected to be used in automation, to fully test – we're trying to prevent logging certain values in production builds [Risks and why]: Low – we change how we log with branching and we're rejiggering a small amount of existing code. [String/UUID change made/needed]: None
Attachment #8751524 -
Flags: approval-mozilla-beta?
Attachment #8751524 -
Flags: approval-mozilla-aurora?
Hi Mike, I will wait for this to land on Nightly before approving uplift to Beta. Are we waiting on something?
Flags: needinfo?(michael.l.comella)
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #7) > Hi Mike, I will wait for this to land on Nightly before approving uplift to > Beta. Are we waiting on something? It's my understanding that for some security bugs we try to land it all branches at once, close to merge date, to avoid releasing publicizing the issue. However, release users are already affected and we don't log any personal data that I'm aware of so I'll just land it.
Flags: needinfo?(michael.l.comella)
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/44dc9457053b0f6ecacbc01c7d11a8b457c3cb55 Bug 1272166 - Update UI telemetry logging. r=margaret
Comment 10•8 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #8) > (In reply to Ritu Kothari (:ritu) from comment #7) > > Hi Mike, I will wait for this to land on Nightly before approving uplift to > > Beta. Are we waiting on something? > > It's my understanding that for some security bugs we try to land it all > branches at once, close to merge date, to avoid releasing publicizing the > issue. This depends on the severity of the issue. See https://wiki.mozilla.org/Security/Bug_Approval_Process . I am not a regular security-rating person (for that, try Al Billings or Dan Veditz), but it seems this is likely non-sec-critical/sec-high (more like sec-low), so you don't need approval to land - though, to be fair, it still feels like I'm not really sure what the threat model / attack vector is. Third-party apps on the same device can know when and using which search engine you search (but not what you search) ? Or something else? How did we rate similar disclosure-through-logs issues? Anyway, it would have been a good idea to get it sec-rated before landing, but it now landed, so...
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44dc9457053b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8751524 [details] [diff] [review] Update UI telemetry logging Let's uplift to Beta47 and Aurora48.
Attachment #8751524 -
Flags: approval-mozilla-beta?
Attachment #8751524 -
Flags: approval-mozilla-beta+
Attachment #8751524 -
Flags: approval-mozilla-aurora?
Attachment #8751524 -
Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd43bc66da8a https://hg.mozilla.org/releases/mozilla-beta/rev/7d1f3450acc4
Updated•8 years ago
|
tracking-fennec: ? → 47+
Updated•8 years ago
|
Group: firefox-core-security → core-security-release
Updated•8 years ago
|
Keywords: csectype-disclosure,
sec-low
Whiteboard: local log leakage
Updated•8 years ago
|
Whiteboard: local log leakage → [adv-main47-] local log leakage
Updated•7 years ago
|
Group: core-security-release
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•