DO NOT MERGE Avoid path traversal in MediaProvider delete call
Added changes to ensure invalid paths are not inserted into MP database
and cannonical path is used during file deletion.
Test: atest MediaProviderTest
Bug: 221855295
Change-Id: Ibeadf4425ae4ecc39613b8c1fb190319dc98b2f6
(cherry picked from commit 18939f61c18602131116ac12eabaf5016f7b5180)
Merged-In: Ibeadf4425ae4ecc39613b8c1fb190319dc98b2f6
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 680cde0..9ae9c6f 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -8408,7 +8408,7 @@
private void deleteIfAllowed(Uri uri, Bundle extras, String path) {
try {
- final File file = new File(path);
+ final File file = new File(path).getCanonicalFile();
checkAccess(uri, extras, file, true);
deleteAndInvalidate(file);
} catch (Exception e) {
diff --git a/src/com/android/providers/media/util/FileUtils.java b/src/com/android/providers/media/util/FileUtils.java
index ee421c9..1c4edb3 100644
--- a/src/com/android/providers/media/util/FileUtils.java
+++ b/src/com/android/providers/media/util/FileUtils.java
@@ -49,8 +49,8 @@
import android.net.Uri;
import android.os.Environment;
import android.os.ParcelFileDescriptor;
-import android.os.UserHandle;
import android.os.SystemProperties;
+import android.os.UserHandle;
import android.os.storage.StorageManager;
import android.os.storage.StorageVolume;
import android.provider.MediaStore;
@@ -1399,9 +1399,18 @@
resolvedDisplayName = displayName;
}
- final File filePath = buildPath(volumePath,
- values.getAsString(MediaColumns.RELATIVE_PATH), resolvedDisplayName);
- values.put(MediaColumns.DATA, filePath.getAbsolutePath());
+ String relativePath = values.getAsString(MediaColumns.RELATIVE_PATH);
+ if (relativePath == null) {
+ relativePath = "";
+ }
+ try {
+ final File filePath = buildPath(volumePath, relativePath, resolvedDisplayName);
+ values.put(MediaColumns.DATA, filePath.getCanonicalPath());
+ } catch (IOException e) {
+ throw new IllegalArgumentException(
+ String.format("Failure in conversion to canonical file path. Failure path: %s.",
+ relativePath.concat(resolvedDisplayName)), e);
+ }
}
public static void sanitizeValues(@NonNull ContentValues values,
diff --git a/tests/src/com/android/providers/media/MediaProviderTest.java b/tests/src/com/android/providers/media/MediaProviderTest.java
index a04f57b..fc30292 100644
--- a/tests/src/com/android/providers/media/MediaProviderTest.java
+++ b/tests/src/com/android/providers/media/MediaProviderTest.java
@@ -78,6 +78,7 @@
import com.android.providers.media.util.SQLiteQueryBuilder;
import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.Assume;
import org.junit.BeforeClass;
import org.junit.Ignore;
@@ -375,6 +376,42 @@
}
}
+ @Test
+ public void testInsertionWithInvalidFilePath_throwsIllegalArgumentException() {
+ final ContentValues values = new ContentValues();
+ values.put(MediaStore.MediaColumns.RELATIVE_PATH, "Android/media/com.example");
+ values.put(MediaStore.Images.Media.DISPLAY_NAME,
+ "./../../../../../../../../../../../data/media/test.txt");
+
+ IllegalArgumentException illegalArgumentException = Assert.assertThrows(
+ IllegalArgumentException.class, () -> sIsolatedResolver.insert(
+ MediaStore.Files.getContentUri(MediaStore.VOLUME_EXTERNAL_PRIMARY),
+ values));
+
+ assertThat(illegalArgumentException).hasMessageThat().contains(
+ "Primary directory Android not allowed for content://media/external_primary/file;"
+ + " allowed directories are [Download, Documents]");
+ }
+
+ @Test
+ public void testUpdationWithInvalidFilePath_throwsIllegalArgumentException() {
+ final ContentValues values = new ContentValues();
+ values.put(MediaStore.MediaColumns.RELATIVE_PATH, "Download");
+ values.put(MediaStore.Images.Media.DISPLAY_NAME, "test.txt");
+ Uri uri = sIsolatedResolver.insert(
+ MediaStore.Files.getContentUri(MediaStore.VOLUME_EXTERNAL_PRIMARY),
+ values);
+
+ final ContentValues newValues = new ContentValues();
+ newValues.put(MediaStore.MediaColumns.DATA, "/storage/emulated/0/../../../data/media/");
+ IllegalArgumentException illegalArgumentException = Assert.assertThrows(
+ IllegalArgumentException.class,
+ () -> sIsolatedResolver.update(uri, newValues, null));
+
+ assertThat(illegalArgumentException).hasMessageThat().contains(
+ "Requested path /data/media doesn't appear under [/storage/emulated/0]");
+ }
+
/**
* We already have solid coverage of this logic in
* {@code CtsProviderTestCases}, but the coverage system currently doesn't
diff --git a/tests/src/com/android/providers/media/util/FileUtilsTest.java b/tests/src/com/android/providers/media/util/FileUtilsTest.java
index 266c144..4704aa1 100644
--- a/tests/src/com/android/providers/media/util/FileUtilsTest.java
+++ b/tests/src/com/android/providers/media/util/FileUtilsTest.java
@@ -63,6 +63,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -1200,4 +1201,27 @@
assertTrue(values.containsKey(MediaColumns.BUCKET_DISPLAY_NAME));
assertNull(values.get(MediaColumns.BUCKET_DISPLAY_NAME));
}
+
+ @Test
+ public void testComputeDataFromValuesForValidPath_success() {
+ final ContentValues values = new ContentValues();
+ values.put(MediaColumns.RELATIVE_PATH, "Android/media/com.example");
+ values.put(MediaColumns.DISPLAY_NAME, "./../../abc.txt");
+
+ FileUtils.computeDataFromValues(values, new File("/storage/emulated/0"), false);
+
+ assertThat(values.getAsString(MediaColumns.DATA)).isEqualTo(
+ "/storage/emulated/0/Android/abc.txt");
+ }
+
+ @Test
+ public void testComputeDataFromValuesForInvalidPath_throwsIllegalArgumentException() {
+ final ContentValues values = new ContentValues();
+ values.put(MediaColumns.RELATIVE_PATH, "\0");
+ values.put(MediaColumns.DISPLAY_NAME, "./../../abc.txt");
+
+ assertThrows(IllegalArgumentException.class,
+ () -> FileUtils.computeDataFromValues(values, new File("/storage/emulated/0"),
+ false));
+ }
}
