Closed Bug 1249092 Opened 8 years ago Closed 8 years ago

GeckoProfile leaks profile path

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox46+ verified, firefox47 verified, fennec46+)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox46 + verified
firefox47 --- verified
fennec 46+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Keywords: sec-low, Whiteboard: [post-critsmash-triage][adv-main46-])

Attachments

(1 file)

via https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java#268
  Log.v(LOGTAG, "Fetching profile: '" + profileName + "', '" + profileDir + "'");

dveditz discovered it was added in FF42, bug 1145192 and mentioned, "the problem is that on older Android all apps can read that log, and if we store the profile on the SD card then many apps have permissions to read the profile".

dveditz did some research:
* We stopped moving the profile to the SD card in bug 721084 (FF14)
* We fixed a bug to stop leaking profile paths in bug 953993 (FF27). Was this necessary?

Richard, you fixed bug 953993 so maybe you have more context – do we still need to obscure the profile path?

Some further facts/speculation on irc:

11:55 <dveditz> the problem is that on older Android all apps can read that log, and if we store the profile on the SD card then many apps have permissions to read the profile
11:55 <mcomella> I see
11:55 <mcomella> I was under the impression we didn't push the profile off to the SD card for that reason
11:55 <mcomella> But maybe that's changed?
11:56 <dveditz> or maybe it changed so that we don't push it to the SD card now, when we used to allow that as an option
11:56 <mcomella> I thought we kept the profile on internal memory but move the application itself to the sd card
11:56 <mcomella> In any case, I'll file a bug to investigate
11:57 <dveditz> even not on the SD card though, if an app knows the location they could send an intent to open a local file: in the browser
11:57 <mcomella> But if it's within the application's storage area, the permissions should be locked down so only the application can open it, right?
11:57 <dveditz> I heard we're dropping support for old android though. not sure what versions this is really an issue for
11:58 <mcomella> We are planning on dropping GB soonish, unclear when that is
11:58 <dveditz> you can make the browser download a file, in which case the browser owns it. 
11:58 <dveditz> and the browser can open files in the browser profile, of course
Flags: needinfo?(rnewman)
Assignee: nobody → michael.l.comella
FYI, we are planning to drop support for GB in 48 (47 will be the last update).
Some more history:
12:08 <dveditz> it was actually added in 1077590, only moved in 1145192. The reviewed attachment differs from the commit :-(
12:08 <dveditz> looks like that bit was supposed to be removed -- https://bugzilla.mozilla.org/show_bug.cgi?id=1077590#c24
Obligatory history:

Bug 56002
Bug 97180
Bug 944373

and pointers from those.


Yes, Android 4.1 or 4.2 and higher prevents other apps from reading your logs (… apart from on rooted devices, and apart from system apps, that is).

Notably, 4.1 is API 16, and so even if we kill GB, we'll still support API 15. That means we still have a justified policy of squashing this kind of logging.
Flags: needinfo?(rnewman)
I wonder if we can't write a test that will parse our logs and warn us if anything like a profile directory pops out...
tracking-fennec: --- → ?
Comment on attachment 8725366 [details] [diff] [review]
Log profile path in debug mode

I'm unclear that we want to log this ever, but logging in debug mode seemed reasonable to me.
Attachment #8725366 - Flags: review?(rnewman)
Attachment #8725366 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ac846629e1df
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8725366 [details] [diff] [review]
Log profile path in debug mode

Approval Request Comment
[Feature/regressing bug #]: bug 1145192
[User impact if declined]: Users profile path will be leaked into logcat output
[Describe test coverage new/current, TreeHerder]: Tested locally
[Risks and why]: Extremely low – we're adding a condition to our else branch and the variable is already existed/tested
[String/UUID change made/needed]: None
Attachment #8725366 - Flags: approval-mozilla-beta?
Attachment #8725366 - Flags: approval-mozilla-aurora?
Comment on attachment 8725366 [details] [diff] [review]
Log profile path in debug mode

too late for 45. We release next tuesday
Attachment #8725366 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
[Tracking Requested - why for this release]: low-risk security fix
tracking-fennec: ? → 46+
Regression since 42, I don't think we need to track.
Comment on attachment 8725366 [details] [diff] [review]
Log profile path in debug mode

Fix to avoid data leak, please uplift to aurora.
Attachment #8725366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It might be good to verify this fix in 46.
Flags: qe-verify+
Group: firefox-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46-]
I was able to reproduce this on Firefox 45.0.2.
Verified as fixed on HTC Desire S (Android 2.3.3) on Firefox 46 Beta 13 and on latest Nightly
Status: RESOLVED → VERIFIED
Group: core-security-release
Remove the qe-verify flag based on comment 16.
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: