Closed Bug 1249092 Opened 6 years ago Closed 6 years ago
Profile leaks profile path
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
6 years ago
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.
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+
https://hg.mozilla.org/integration/fx-team/rev/ac846629e1df216445858e18057d0ef4e912fad2 Bug 1249092 - Log profile path in debug mode. r=rnewman
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
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
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.
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
Remove the qe-verify flag based on comment 16.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.