diff options
Diffstat (limited to 'src/com')
-rw-r--r-- | src/com/android/providers/downloads/DownloadProvider.java | 14 | ||||
-rwxr-xr-x | src/com/android/providers/downloads/DownloadThread.java | 12 | ||||
-rw-r--r-- | src/com/android/providers/downloads/Helpers.java | 76 |
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) { |