Merge conflict--DO NOT MERGE. Use resolved path for both checking and opening.

This avoids a race condition where someone can change a symlink
target after the security checks have passed.

Bug: 26211054
Change-Id: I0dcc41c94dfede2d5dc75031191605944be2e595

3 files changed, 82 insertions, 20 deletions

diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java
index 25d59014..5995a083 100644
--- a/src/com/android/providers/downloads/DownloadProvider.java
+++ b/src/com/android/providers/downloads/DownloadProvider.java
@@ -875,7 +875,7 @@ public final class DownloadProvider extends ContentProvider {
             if (projection == null) {
                 projection = sAppReadableColumnsArray.clone();
             } else {
-                // check the validity of the columns in projection 
+                // check the validity of the columns in projection
                 for (int i = 0; i < projection.length; ++i) {
                     if (!sAppReadableColumnsSet.contains(projection[i]) &&
                             !downloadManagerColumnsList.contains(projection[i])) {
@@ -1221,11 +1221,17 @@ public final class DownloadProvider extends ContentProvider {
         if (path == null) {
             throw new FileNotFoundException("No filename found.");
         }
-        if (!Helpers.isFilenameValid(getContext(), path, mDownloadsDataDir)) {
-            throw new FileNotFoundException("Invalid filename: " + path);
+
+        final File file;
+        try {
+            file = new File(path).getCanonicalFile();
+        } catch (IOException e) {
+            throw new FileNotFoundException(e.getMessage());
         }
 
-        final File file = new File(path);
+        if (!Helpers.isFilenameValid(getContext(), file)) {
+            throw new FileNotFoundException("Invalid file path: " + file);
+        }
         if ("r".equals(mode)) {
             return ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY);
         } else {
diff --git a/src/com/android/providers/downloads/DownloadThread.java b/src/com/android/providers/downloads/DownloadThread.java
index 88cd9162..48de0aa2 100755
--- a/src/com/android/providers/downloads/DownloadThread.java
+++ b/src/com/android/providers/downloads/DownloadThread.java
@@ -769,14 +769,20 @@ public class DownloadThread implements Runnable {
                 Log.i(Constants.TAG, "have run thread before for id: " + mInfo.mId +
                         ", and state.mFilename: " + state.mFilename);
             }
-            if (!Helpers.isFilenameValid(mContext, state.mFilename,
-                    mStorageManager.getDownloadDataDirectory())) {
+
+            File f;
+            try {
+                f = new File(state.mFilename).getCanonicalFile();
+            } catch (IOException e) {
+                throw new StopRequestException(Downloads.Impl.STATUS_FILE_ERROR,
+                        e.getMessage());
+            }
+            if (!Helpers.isFilenameValid(mContext, f)) {
                 // this should never happen
                 throw new StopRequestException(Downloads.Impl.STATUS_FILE_ERROR,
                         "found invalid internal destination filename");
             }
             // We're resuming a download that got interrupted
-            File f = new File(state.mFilename);
             if (f.exists()) {
                 if (Constants.LOGV) {
                     Log.i(Constants.TAG, "resuming download for id: " + mInfo.mId +
diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java
index 40cf5e30..716109c5 100644
--- a/src/com/android/providers/downloads/Helpers.java
+++ b/src/com/android/providers/downloads/Helpers.java
@@ -350,32 +350,41 @@ public class Helpers {
     }
 
     /**
-     * Checks whether the filename looks legitimate
+     * Checks whether the filename looks legitimate for security purposes. This
+     * prevents us from opening files that aren't actually downloads.
      */
-    static boolean isFilenameValid(Context context, String filename, File downloadsDataDir) {
-        final String[] whitelist;
+    static boolean isFilenameValid(Context context, File file) {
+        final File[] whitelist;
+
         final List<String> secondaryStoragePaths = StorageManager.getSecondaryStoragePaths(context);
+
         try {
-            filename = new File(filename).getCanonicalPath();
-            whitelist = new String[] {
-                    downloadsDataDir.getCanonicalPath(),
-                    Environment.getDownloadCacheDirectory().getCanonicalPath(),
-                    Environment.getExternalStorageDirectory().getCanonicalPath(),
+            whitelist = new File[] {
+                    context.getFilesDir().getCanonicalFile(),
+                    context.getCacheDir().getCanonicalFile(),
+                    Environment.getDownloadCacheDirectory().getCanonicalFile(),
+                    Environment.getExternalStorageDirectory().getCanonicalFile(),
             };
         } catch (IOException e) {
             Log.w(TAG, "Failed to resolve canonical path: " + e);
             return false;
         }
 
-        for (String test : whitelist) {
-            if (filename.startsWith(test)) {
-                return true;
+        for (File testDir : whitelist) {
+            if (contains(testDir, file)) {
+                  return true;
             }
         }
 
         for (String test : secondaryStoragePaths) {
-            if (filename.startsWith(test)) {
-                return true;
+            try {
+                File testDir = new File(test).getCanonicalFile();
+                if (contains(testDir, file)) {
+                    return true;
+                }
+            } catch (IOException e) {
+                Log.w(TAG, "Failed to resolve canonical path for sec. storage path: " + e);
+                return false;
             }
         }
 
@@ -383,6 +392,47 @@ public class Helpers {
     }
 
     /**
+     * Test if a file lives under the given directory, either as a direct child
+     * or a distant grandchild.
+     * <p>
+     * Both files <em>must</em> have been resolved using
+     * {@link File#getCanonicalFile()} to avoid symlink or path traversal
+     * attacks.
+     */
+    public static boolean contains(File[] dirs, File file) {
+        for (File dir : dirs) {
+            if (contains(dir, file)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Test if a file lives under the given directory, either as a direct child
+     * or a distant grandchild.
+     * <p>
+     * Both files <em>must</em> have been resolved using
+     * {@link File#getCanonicalFile()} to avoid symlink or path traversal
+     * attacks.
+     */
+    public static boolean contains(File dir, File file) {
+        if (dir == null || file == null) return false;
+
+        String dirPath = dir.getAbsolutePath();
+        String filePath = file.getAbsolutePath();
+
+        if (dirPath.equals(filePath)) {
+            return true;
+        }
+
+        if (!dirPath.endsWith("/")) {
+            dirPath += "/";
+        }
+        return filePath.startsWith(dirPath);
+    }
+
+    /**
      * Checks whether this looks like a legitimate selection parameter
      */
     public static void validateSelection(String selection, Set<String> allowedColumns) {