Closed Bug 1137459 Opened 5 years ago Closed 5 years ago

FxA logs contain sensitive information

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox36 --- wontfix
firefox37 --- verified
firefox38 --- verified
firefox39 --- verified
firefox-esr31 37+ affected

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file)

We are seeing in logs:

> 1424966817095	FirefoxAccounts	DEBUG	startVerifiedCheck {"email":...,"uid":...,"sessionToken":...,"sessionTokenContext":...,"unwrapBKey":...,"keyFetchToken":...,"customizeSync":...}

(but with actual values instead of '...'.  We already have a logPII global for this, but this log entry isn't using it.

Attaching a trivial patch to address this.
Attachment #8570136 - Flags: review?(ckarlof)
Flags: qe-verify+
Flags: firefox-backlog+
Comment on attachment 8570136 [details] [diff] [review]
0005-Bug-XXXXXXX-avoid-sensitive-information-in-the-FxA-l.patch

Review of attachment 8570136 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I missed this Mark.
Attachment #8570136 - Flags: review?(ckarlof) → review+
Assignee: nobody → mhammond
Iteration: --- → 39.1 - 9 Mar
https://hg.mozilla.org/mozilla-central/rev/c5335f63e7c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8570136 [details] [diff] [review]
0005-Bug-XXXXXXX-avoid-sensitive-information-in-the-FxA-l.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Users with sync errors and who paste logs may disclose sensitive information, including the all-important keys
[Describe test coverage new/current, TreeHerder]: Existing tests all pass.
[Risks and why]: Trivial and low-risk patch
[String/UUID change made/needed]: None
Attachment #8570136 - Flags: approval-mozilla-beta?
Attachment #8570136 - Flags: approval-mozilla-aurora?
QA Contact: catalin.varga
(In reply to Mark Hammond [:markh] from comment #4)
> [Feature/regressing bug #]: N/A

In what version was this issue first introduced into Firefox?
Flags: needinfo?(mhammond)
It looks like that has existed ever since the Firefox Accounts based Sync landed, in Dec 2013.  IIRC that's 29.
Flags: needinfo?(mhammond)
Comment on attachment 8570136 [details] [diff] [review]
0005-Bug-XXXXXXX-avoid-sensitive-information-in-the-FxA-l.patch

I think this is a small enough change to clean up the logging that we should take it in Beta 3 (even though it has existed for quite some time). Beta+ Aurora+
Attachment #8570136 - Flags: approval-mozilla-beta?
Attachment #8570136 - Flags: approval-mozilla-beta+
Attachment #8570136 - Flags: approval-mozilla-aurora?
Attachment #8570136 - Flags: approval-mozilla-aurora+
Given that this bug was introduced in 29 and potentially bleeds sensitive information into the logs, it may be worth taking this fix on ESR 31. (Not sure anyone would normally raise this as an issue unless they discovered it.) My hesitation is that I don't know what the usage is of FxA on ESR.
Meant to nom for ESR in the previous comment.
I've tried reproducing this issue without success, can you please provide a scenario that will determine logging of sensitive information?
Flags: needinfo?(mhammond)
To repro:
* Start with a clean profile.
* Flip the pref services.sync.logOnSuccess to true
* Setup sync by creating a new Firefox Account, and let Sync complete.
* Open about:sync-log, view all the logs.  Without this patch you should see the entry listed in comment 0, with it you should not.
Flags: needinfo?(mhammond)
Verified as fixed using the following environment:

FF 37.0b6
Build Id: 20150316202753

FF 38
Build Id: 20150316004007

FF 39
Build Id: 20150316030202

OS: Win 7 x64, Mac Os X 10.7.5, Ubuntu 14.04 x86
Mark can you check if this applies to esr31?  We can take it there if it's clean to land as it's low risk.  If it's too much to try and make it apply then we'll leave this and the issue will no longer exist in ESR 38 which ships in 8 more weeks.
Flags: needinfo?(mhammond)
I haven't tried actually applying it, but looking at mxr I'm confident it will.
Flags: needinfo?(mhammond)
Status: RESOLVED → VERIFIED
Product: Core → Firefox
Target Milestone: mozilla39 → Firefox 39
You need to log in before you can comment on or make changes to this bug.