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
diff --git a/src/com/android/providers/downloads/DownloadProvider.java b/src/com/android/providers/downloads/DownloadProvider.java
index ad3cf7a..1595dfa 100644
--- a/src/com/android/providers/downloads/DownloadProvider.java
+++ b/src/com/android/providers/downloads/DownloadProvider.java
@@ -1205,8 +1205,16 @@
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 013faf2..61a49a2 100644
--- a/src/com/android/providers/downloads/Helpers.java
+++ b/src/com/android/providers/downloads/Helpers.java
@@ -341,24 +341,25 @@
}
/**
- * 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 @@
}
/**
+ * 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) {
