diff options
author | Jeff Sharkey <jsharkey@android.com> | 2016-01-07 14:15:59 -0700 |
---|---|---|
committer | Jeff Sharkey <jsharkey@android.com> | 2016-01-14 14:24:09 -0700 |
commit | 5accb135178325878840c6e36fc3e640ae582dea (patch) | |
tree | aa0af740555f5791a96c42195d8447b80db7b786 /src | |
parent | 06d3867b1d96c3ad73d677e7c04a8d94b9f93c8b (diff) | |
download | android_packages_providers_DownloadProvider-5accb135178325878840c6e36fc3e640ae582dea.tar.gz android_packages_providers_DownloadProvider-5accb135178325878840c6e36fc3e640ae582dea.tar.bz2 android_packages_providers_DownloadProvider-5accb135178325878840c6e36fc3e640ae582dea.zip |
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
Diffstat (limited to 'src')
-rw-r--r-- | src/com/android/providers/downloads/DownloadProvider.java | 12 | ||||
-rw-r--r-- | 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; } } @@ -367,6 +368,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) { |