From 366af2ee1f841615d44ab770b537112d769eed05 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++++++++-- src/com/android/providers/downloads/Helpers.java | 4 ---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 94e5a997..620085fc 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1260,9 +1260,15 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - final File file = new File(path); + final File file; + try { + file = new File(path).getCanonicalFile(); + } catch (IOException e) { + throw new FileNotFoundException(e.getMessage()); + } + if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file: " + file); + throw new FileNotFoundException("Invalid file path: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index d1cc5450..d01cbff2 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -357,8 +357,6 @@ public class Helpers { static boolean isFilenameValidInExternalPackage(Context context, File file, String packageName) { try { - file = file.getCanonicalFile(); - if (containsCanonical(buildExternalStorageAppFilesDirs(packageName), file) || containsCanonical(buildExternalStorageAppObbDirs(packageName), file) || containsCanonical(buildExternalStorageAppCacheDirs(packageName), file) || @@ -380,8 +378,6 @@ public class Helpers { */ static boolean isFilenameValid(Context context, File file, boolean allowInternal) { try { - file = file.getCanonicalFile(); - if (allowInternal) { if (containsCanonical(context.getFilesDir(), file) || containsCanonical(context.getCacheDir(), file) -- cgit v1.2.3 From 6608eb8feedafab93b840c208a40aa6d6c755584 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++++++++-- src/com/android/providers/downloads/Helpers.java | 4 ---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 94e5a997..620085fc 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1260,9 +1260,15 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - final File file = new File(path); + final File file; + try { + file = new File(path).getCanonicalFile(); + } catch (IOException e) { + throw new FileNotFoundException(e.getMessage()); + } + if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file: " + file); + throw new FileNotFoundException("Invalid file path: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index d1cc5450..d01cbff2 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -357,8 +357,6 @@ public class Helpers { static boolean isFilenameValidInExternalPackage(Context context, File file, String packageName) { try { - file = file.getCanonicalFile(); - if (containsCanonical(buildExternalStorageAppFilesDirs(packageName), file) || containsCanonical(buildExternalStorageAppObbDirs(packageName), file) || containsCanonical(buildExternalStorageAppCacheDirs(packageName), file) || @@ -380,8 +378,6 @@ public class Helpers { */ static boolean isFilenameValid(Context context, File file, boolean allowInternal) { try { - file = file.getCanonicalFile(); - if (allowInternal) { if (containsCanonical(context.getFilesDir(), file) || containsCanonical(context.getCacheDir(), file) -- cgit v1.2.3 From 8fbf1207a69b123493a8c6ca7e0295c385d195dc Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++++++++-- src/com/android/providers/downloads/Helpers.java | 4 ---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 94e5a997..620085fc 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1260,9 +1260,15 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - final File file = new File(path); + final File file; + try { + file = new File(path).getCanonicalFile(); + } catch (IOException e) { + throw new FileNotFoundException(e.getMessage()); + } + if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file: " + file); + throw new FileNotFoundException("Invalid file path: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index d1cc5450..d01cbff2 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -357,8 +357,6 @@ public class Helpers { static boolean isFilenameValidInExternalPackage(Context context, File file, String packageName) { try { - file = file.getCanonicalFile(); - if (containsCanonical(buildExternalStorageAppFilesDirs(packageName), file) || containsCanonical(buildExternalStorageAppObbDirs(packageName), file) || containsCanonical(buildExternalStorageAppCacheDirs(packageName), file) || @@ -380,8 +378,6 @@ public class Helpers { */ static boolean isFilenameValid(Context context, File file, boolean allowInternal) { try { - file = file.getCanonicalFile(); - if (allowInternal) { if (containsCanonical(context.getFilesDir(), file) || containsCanonical(context.getCacheDir(), file) -- cgit v1.2.3 From bdc831357e7a116bc561d51bf2ddc85ff11c01a9 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++++++++-- src/com/android/providers/downloads/Helpers.java | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 4b23024f..2d914c41 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1230,9 +1230,15 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - final File file = new File(path); + final File file; + try { + file = new File(path).getCanonicalFile(); + } catch (IOException e) { + throw new FileNotFoundException(e.getMessage()); + } + if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file: " + file); + throw new FileNotFoundException("Invalid file path: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index 0aa49c0a..1b4c911e 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -341,7 +341,6 @@ public class Helpers { static boolean isFilenameValid(Context context, File file) { final File[] whitelist; try { - file = file.getCanonicalFile(); whitelist = new File[] { context.getFilesDir().getCanonicalFile(), context.getCacheDir().getCanonicalFile(), -- cgit v1.2.3 From 5c08fb8cbeb045b9ce447443208e87f42604d168 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++++++++-- src/com/android/providers/downloads/Helpers.java | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 4b23024f..2d914c41 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1230,9 +1230,15 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - final File file = new File(path); + final File file; + try { + file = new File(path).getCanonicalFile(); + } catch (IOException e) { + throw new FileNotFoundException(e.getMessage()); + } + if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file: " + file); + throw new FileNotFoundException("Invalid file path: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index eb071395..7ca50bb1 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -341,7 +341,6 @@ public class Helpers { static boolean isFilenameValid(Context context, File file) { final File[] whitelist; try { - file = file.getCanonicalFile(); whitelist = new File[] { context.getFilesDir().getCanonicalFile(), context.getCacheDir().getCanonicalFile(), -- cgit v1.2.3 From 5accb135178325878840c6e36fc3e640ae582dea Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 7 Jan 2016 14:15:59 -0700 Subject: 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: I5842aaecc7b7d417a3b1902957b59b8a1f3c1ccb --- .../providers/downloads/DownloadProvider.java | 12 ++++- src/com/android/providers/downloads/Helpers.java | 62 ++++++++++++++++++---- 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index ad3cf7ac..1595dfa9 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1205,8 +1205,16 @@ public final class DownloadProvider extends ContentProvider { if (path == null) { throw new FileNotFoundException("No filename found."); } - if (!Helpers.isFilenameValid(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()); + } + + if (!Helpers.isFilenameValid(getContext(), file)) { + throw new FileNotFoundException("Invalid file path: " + file); } final File file = new File(path); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index 013faf27..61a49a2a 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -341,24 +341,25 @@ 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(String filename, File downloadsDataDir) { - final String[] whitelist; + static boolean isFilenameValid(Context context, File file) { + final File[] whitelist; 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)) { + for (File testDir : whitelist) { + if (contains(testDir, file)) { return true; } } @@ -366,6 +367,47 @@ public class Helpers { return false; } + /** + * Test if a file lives under the given directory, either as a direct child + * or a distant grandchild. + *

+ * Both files must 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. + *

+ * Both files must 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 */ -- cgit v1.2.3 From 6947a091f1d9d2b62f5d4a6429df2679ffe5434b Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 15 Jan 2016 18:51:24 +0000 Subject: Revert "Use resolved path for both checking and opening." This reverts commit 5accb135178325878840c6e36fc3e640ae582dea. Change-Id: I5ec1719b28feafb5b0850ec7c17cf23571ab0bba --- .../providers/downloads/DownloadProvider.java | 12 +---- src/com/android/providers/downloads/Helpers.java | 62 ++++------------------ 2 files changed, 12 insertions(+), 62 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 1595dfa9..ad3cf7ac 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1205,16 +1205,8 @@ public final class DownloadProvider extends ContentProvider { if (path == null) { throw new FileNotFoundException("No filename found."); } - - final File file; - try { - file = new File(path).getCanonicalFile(); - } catch (IOException e) { - throw new FileNotFoundException(e.getMessage()); - } - - if (!Helpers.isFilenameValid(getContext(), file)) { - throw new FileNotFoundException("Invalid file path: " + file); + if (!Helpers.isFilenameValid(path, mDownloadsDataDir)) { + throw new FileNotFoundException("Invalid filename: " + path); } final File file = new File(path); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index 61a49a2a..013faf27 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -341,25 +341,24 @@ public class Helpers { } /** - * Checks whether the filename looks legitimate for security purposes. This - * prevents us from opening files that aren't actually downloads. + * Checks whether the filename looks legitimate */ - static boolean isFilenameValid(Context context, File file) { - final File[] whitelist; + static boolean isFilenameValid(String filename, File downloadsDataDir) { + final String[] whitelist; try { - whitelist = new File[] { - context.getFilesDir().getCanonicalFile(), - context.getCacheDir().getCanonicalFile(), - Environment.getDownloadCacheDirectory().getCanonicalFile(), - Environment.getExternalStorageDirectory().getCanonicalFile(), + filename = new File(filename).getCanonicalPath(); + whitelist = new String[] { + downloadsDataDir.getCanonicalPath(), + Environment.getDownloadCacheDirectory().getCanonicalPath(), + Environment.getExternalStorageDirectory().getCanonicalPath(), }; } catch (IOException e) { Log.w(TAG, "Failed to resolve canonical path: " + e); return false; } - for (File testDir : whitelist) { - if (contains(testDir, file)) { + for (String test : whitelist) { + if (filename.startsWith(test)) { return true; } } @@ -367,47 +366,6 @@ public class Helpers { return false; } - /** - * Test if a file lives under the given directory, either as a direct child - * or a distant grandchild. - *

- * Both files must 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. - *

- * Both files must 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 */ -- cgit v1.2.3 From 616188b0a0c96e88926e84cfe156908f0da9ebe2 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 22 Jan 2016 22:22:25 +0000 Subject: Revert "Use resolved path for both checking and opening." This reverts commit 366af2ee1f841615d44ab770b537112d769eed05. Change-Id: Id1155425ebcae23be8ce3916f19dda82eee992c4 --- src/com/android/providers/downloads/DownloadProvider.java | 10 ++-------- src/com/android/providers/downloads/Helpers.java | 4 ++++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java index 620085fc..94e5a997 100644 --- a/src/com/android/providers/downloads/DownloadProvider.java +++ b/src/com/android/providers/downloads/DownloadProvider.java @@ -1260,15 +1260,9 @@ public final class DownloadProvider extends ContentProvider { throw new FileNotFoundException("No filename found."); } - 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); + throw new FileNotFoundException("Invalid file: " + file); } final int pfdMode = ParcelFileDescriptor.parseMode(mode); diff --git a/src/com/android/providers/downloads/Helpers.java b/src/com/android/providers/downloads/Helpers.java index d01cbff2..d1cc5450 100644 --- a/src/com/android/providers/downloads/Helpers.java +++ b/src/com/android/providers/downloads/Helpers.java @@ -357,6 +357,8 @@ public class Helpers { static boolean isFilenameValidInExternalPackage(Context context, File file, String packageName) { try { + file = file.getCanonicalFile(); + if (containsCanonical(buildExternalStorageAppFilesDirs(packageName), file) || containsCanonical(buildExternalStorageAppObbDirs(packageName), file) || containsCanonical(buildExternalStorageAppCacheDirs(packageName), file) || @@ -378,6 +380,8 @@ public class Helpers { */ static boolean isFilenameValid(Context context, File file, boolean allowInternal) { try { + file = file.getCanonicalFile(); + if (allowInternal) { if (containsCanonical(context.getFilesDir(), file) || containsCanonical(context.getCacheDir(), file) -- cgit v1.2.3