770912 - Excessive logging could violate user privacy

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Right now we're logging every URL the user visits to logcat, which is A Bad Idea(TM). (see http://www.defcon.org/images/defcon-18/dc-18-presentations/Lineberry/DEFCON-18-Lineberry-Not-The-Permissions-You-Are-Looking-For.pdf)

The attached patch removes (or disables) the ones I could find by looking in logcat and the code. There might be more.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Review ping. This has bitrotted a lot but if it's worth landing this then I can un-bitrot it.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 639113 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java

>+    private static final boolean VIOLATE_PRIVACY = false;

Let's not even do this. It's a compile time thing so specific logging could be added just as easily.

>         private BitmapDrawable downloadFavicon(URL faviconUrl) {

>             try {
>                 uri = faviconUrl.toURI();
>             } catch (URISyntaxException e) {
>-                Log.d(LOGTAG, "Could not get URI for favicon URL: " + mFaviconUrl);

Let's just log "Could not get URI for favicon", which will let people know the exception was hit.

>             } catch (MalformedURLException e) {
>-                Log.d(LOGTAG, "The provided favicon URL is not valid: " + e);

"The provided favicon URL is not valid"


>     void handleDoorHanger(JSONObject geckoObject) throws JSONException {

>-        Log.i(LOGTAG, "DoorHanger received for tab " + tabId + ", msg:" + message);
>+        Log.i(LOGTAG, "DoorHanger received for tab " + tabId);

Completely remove this

>         else if (Intent.ACTION_VIEW.equals(action)) {
>             String uri = intent.getDataString();
>             GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(uri));
>             Log.i(LOGTAG,"onNewIntent: " + uri);

Remove this one too?

>         else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {
>             String uri = getURIFromIntent(intent);
>             GeckoAppShell.sendEventToGecko(GeckoEvent.createWebappLoadEvent(uri));
>-            Log.i(LOGTAG,"Intent : WEBAPP (" + action + ") - " + uri);
>+            Log.i(LOGTAG,"Intent : WEBAPP (" + action + ")");

Let's just remove it

>         else if (ACTION_BOOKMARK.equals(action)) {
>             String uri = getURIFromIntent(intent);
>             GeckoAppShell.sendEventToGecko(GeckoEvent.createBookmarkLoadEvent(uri));
>-            Log.i(LOGTAG,"Intent : BOOKMARK - " + uri);
>+            Log.i(LOGTAG,"Intent : BOOKMARK");

Remove too

r+, I agree we need to stop putting private info in the logs. Brian just mentioned this to me today regarding Private Browsing mode. Thanks for re-pinging.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch (unbitrotted)

Unbitrotted and addressed review comments. Enough has changed that I'd like re-review. In particular I found a few more places were printing out the URLs so I removed those as well.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5557d37be66

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5557d37be66

Comment 6

[Cristian Nicolae (:xti)]

Changes are applied on the latest Nightly build. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-11)
Device: Galaxy Note
OS: Android 4.0.4