summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJeff Sharkey <jsharkey@android.com>2016-01-07 14:15:59 -0700
committerJeff Sharkey <jsharkey@android.com>2016-01-14 14:24:09 -0700
commit5accb135178325878840c6e36fc3e640ae582dea (patch)
treeaa0af740555f5791a96c42195d8447b80db7b786
parent06d3867b1d96c3ad73d677e7c04a8d94b9f93c8b (diff)
downloadandroid_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
-rw-r--r--src/com/android/providers/downloads/DownloadProvider.java12
-rw-r--r--src/com/android/providers/downloads/Helpers.java62
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) {