[tor-commits] [tor-browser/tor-browser-60.4.0esr-8.5-1] Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko. r=jchen

[gk at torproject.org]

commit ad9b3c0d704dceab75c2b3f6246740c78d8f7c04
Author: Jan Henning <jh+bugzilla at buttercookie.de>
Date:   Sun May 13 00:07:48 2018 +0200

    Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko. r=jchen
    
    This is a case where I disagree with Google's stance about content:// URIs.
    They're perfect for granting access to files that might not even be present on
    the file system, e.g. virtual files generated on the spot or retrieved from some
    database, a cloud storage provider's app granting access to files stored in the
    cloud, etc., as well as for being able to selectively grant access to files
    conceptually "owned" by a certain app, especially files within the app's private
    internal storage.
    
    However when considering files that don't actually "belong" to any specific app
    in particular and that are already being stored in a publicly accessible (modulo
    the READ_EXTERNAL_STORAGE permission, respectively the user granting access
    through the Storage Access Framework) directory somewhere within the external
    storage, they also have a number of drawbacks:
    - While in practice a number of FileProviders will "leak" the true file system
      path through the content:// URI they generate, the problem remains that
      there's no way to know for sure whether two content:// URIs received from
      different apps are in fact referring to the same file or not.
      In case of our downloads for example, content:// URIs all referring to the
      same file could in principle be generated
      * by Firefox itself
      * by the system Downloads app
      * by the system file browser app
      * by any other third-party file browser or similar app that the user might
        have installed
      which e.g. will needlessly clutter up any LRU lists other apps might keep.
    - content:// URIs obviously depend on the generating app still being installed.
      So even if we fixed bug 1280184, so that uninstalling Firefox would no longer
      remove the user's downloads, all content:// URIs generated by Firefox re-
      ferring to those files would become invalid anyway.
    - Even if the actual file is already sitting in a public directory, when
      accessing it through the content:// URI the receiving app still needs to
      explicitly persist the permissions granted for that URI, and there are some
      signs that you can only persist permissions for a limited number of files. For
      file:// URIs on the other hand the only limitation on the number of file://
      URIS you can remember is the available storage space for storing those URIs,
      i.e. for practical purposes more or less unlimited.
    - content:// URIs only grant access to a specific file. If we (or possibly an
      add-on) started implementing saving of websites as on desktop (i.e. HTML +
      associated support files instead of a PDF "copy"), then receiving apps
      couldn't properly open those additional support files (images, style sheets,
      etc.) when getting a content:// URI to the main HTML file
      (see https://issuetracker.google.com/issues/77406791).
    
    Since we do store downloads in the public Downloads folder on the external
    storage by default and I believe that conceptually, those files belong to the
    user and not Firefox specifically, I propose to continue launching downloaded
    files directly through their file:// URI.
    To that end, we temporarily disable the corresponding StrictMode restrictions
    when required and restore them afterwards.
    
    MozReview-Commit-ID: LuIYIA5FSGf
    
    --HG--
    extra : rebase_source : a690b3097fdb03591f25f05a944c9ca3c05ddd04
---
 .../org/mozilla/gecko/notifications/NotificationHelper.java    |  7 +++++++
 .../src/main/java/org/mozilla/gecko/GeckoAppShell.java         | 10 +++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java b/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
index 35366609da49..713e9f9d4b3c 100644
--- a/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
@@ -32,6 +32,7 @@ import android.content.pm.PackageManager;
 import android.content.pm.ResolveInfo;
 import android.graphics.Bitmap;
 import android.net.Uri;
+import android.os.StrictMode;
 import android.support.v4.util.SimpleArrayMap;
 import android.util.Log;
 
@@ -295,8 +296,14 @@ public final class NotificationHelper implements BundleEventListener {
         // scheme to prevent Fennec from popping up.
         final Intent viewFileIntent = createIntentIfDownloadCompleted(message);
         if (builder != null && viewFileIntent != null && mContext != null) {
+            // Bug 1450449 - Downloaded files already are already in a public directory and aren't
+            // really owned exclusively by Firefox, so there's no real benefit to using
+            // content:// URIs here.
+            StrictMode.VmPolicy prevPolicy = StrictMode.getVmPolicy();
+            StrictMode.setVmPolicy(StrictMode.VmPolicy.LAX);
             final PendingIntent pIntent = PendingIntent.getActivity(
                     mContext, 0, viewFileIntent, PendingIntent.FLAG_UPDATE_CURRENT);
+            StrictMode.setVmPolicy(prevPolicy);
             builder.setAutoCancel(true);
             builder.setContentIntent(pIntent);
 
diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
index 34ba3315f295..3d21d7bb2f56 100644
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ -86,6 +86,7 @@ import android.os.Environment;
 import android.os.Looper;
 import android.os.ParcelFileDescriptor;
 import android.os.PowerManager;
+import android.os.StrictMode;
 import android.os.SystemClock;
 import android.os.Vibrator;
 import android.provider.Settings;
@@ -956,7 +957,14 @@ public class GeckoAppShell
         if (geckoInterface == null) {
             return false;
         }
-        return geckoInterface.openUriExternal(targetURI, mimeType, packageName, className, action, title);
+        // Bug 1450449 - Downloaded files already are already in a public directory and aren't
+        // really owned exclusively by Firefox, so there's no real benefit to using
+        // content:// URIs here.
+        StrictMode.VmPolicy prevPolicy = StrictMode.getVmPolicy();
+        StrictMode.setVmPolicy(StrictMode.VmPolicy.LAX);
+        boolean success = geckoInterface.openUriExternal(targetURI, mimeType, packageName, className, action, title);
+        StrictMode.setVmPolicy(prevPolicy);
+        return success;
     }
 
     @WrapForJNI(dispatchTo = "gecko")