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)

All
Android
defect
Not set
normal

Tracking

(firefox47 fixed, firefox48 fixed, firefox49 fixed, fennec47+)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed
fennec 47+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Keywords: csectype-disclosure, sec-low, Whiteboard: [adv-main47-] local log leakage)

Attachments

(1 file)

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.
MozReview-Commit-ID: Gzb2qdm3TuT
Attachment #8751524 - Flags: review?(margaret.leibovic)
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.
Attachment #8751524 - Flags: review?(margaret.leibovic) → review+
(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?
(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
(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
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)
(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)
(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...
https://hg.mozilla.org/mozilla-central/rev/44dc9457053b
Status: NEW → RESOLVED
Closed: 8 years ago
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+
tracking-fennec: ? → 47+
Group: firefox-core-security → core-security-release
Whiteboard: local log leakage
Whiteboard: local log leakage → [adv-main47-] local log leakage
Group: core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.