Fix path traversal vulnerabilities in MediaProvider
Canonicalize filepath provided by the caller when hanling SCAN_FILE_CALL
method call in MediaProvider.
Additionally, make sure to check access permission in SCAN_FILE_CALL
(using enforceCallingPermissionInternal()).
Preemptively canonicalize Files provided as an arguments to the public
API methods in ModernMediaScanner (scanFile(), scanDirectory() and
onDirectoryDirty()) to prevent path traversal attacks.
Bug: 262244882
Test: atest MediaProviderTests
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:5bb32d7fba00b9e53c7e20ae8acaf6f84a8b2e8d)
Merged-In: I61e77d69ae857984b819fa0ea27bec5c26a34842
Change-Id: I61e77d69ae857984b819fa0ea27bec5c26a34842
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 9ae9c6f..87a5fa5 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -1691,11 +1691,7 @@
}
public Uri scanFile(File file, int reason) {
- return scanFile(file, reason, null);
- }
-
- public Uri scanFile(File file, int reason, String ownerPackage) {
- return mMediaScanner.scanFile(file, reason, ownerPackage);
+ return mMediaScanner.scanFile(file, reason);
}
private Uri scanFileAsMediaProvider(File file, int reason) {
@@ -6162,38 +6158,51 @@
}
return null;
}
- case MediaStore.SCAN_FILE_CALL:
+ case MediaStore.SCAN_FILE_CALL: {
+ final LocalCallingIdentity token = clearLocalCallingIdentity();
+ final CallingIdentity providerToken = clearCallingIdentity();
+
+ final String filePath = arg;
+ final Uri uri;
+ try {
+ File file;
+ try {
+ file = FileUtils.getCanonicalFile(filePath);
+ } catch (IOException e) {
+ file = null;
+ }
+
+ uri = file != null ? scanFile(file, REASON_DEMAND) : null;
+ } finally {
+ restoreCallingIdentity(providerToken);
+ restoreLocalCallingIdentity(token);
+ }
+
+ // TODO(b/262244882): maybe enforceCallingPermissionInternal(uri, ...)
+
+ final Bundle res = new Bundle();
+ res.putParcelable(Intent.EXTRA_STREAM, uri);
+ return res;
+ }
case MediaStore.SCAN_VOLUME_CALL: {
final int userId = uidToUserId(Binder.getCallingUid());
final LocalCallingIdentity token = clearLocalCallingIdentity();
final CallingIdentity providerToken = clearCallingIdentity();
+
+ final String volumeName = arg;
try {
- final Bundle res = new Bundle();
- switch (method) {
- case MediaStore.SCAN_FILE_CALL: {
- final File file = new File(arg);
- res.putParcelable(Intent.EXTRA_STREAM, scanFile(file, REASON_DEMAND));
- break;
- }
- case MediaStore.SCAN_VOLUME_CALL: {
- final String volumeName = arg;
- try {
- MediaVolume volume = mVolumeCache.findVolume(volumeName,
- UserHandle.of(userId));
- MediaService.onScanVolume(getContext(), volume, REASON_DEMAND);
- } catch (FileNotFoundException e) {
- Log.w(TAG, "Failed to find volume " + volumeName, e);
- }
- break;
- }
- }
- return res;
+ final MediaVolume volume = mVolumeCache.findVolume(volumeName,
+ UserHandle.of(userId));
+ MediaService.onScanVolume(getContext(), volume, REASON_DEMAND);
+ } catch (FileNotFoundException e) {
+ Log.w(TAG, "Failed to find volume " + volumeName, e);
} catch (IOException e) {
throw new RuntimeException(e);
} finally {
restoreCallingIdentity(providerToken);
restoreLocalCallingIdentity(token);
}
+ return Bundle.EMPTY;
}
case MediaStore.GET_VERSION_CALL: {
final String volumeName = extras.getString(Intent.EXTRA_TEXT);
diff --git a/src/com/android/providers/media/scan/LegacyMediaScanner.java b/src/com/android/providers/media/scan/LegacyMediaScanner.java
index d8d3bed..d73dda5 100644
--- a/src/com/android/providers/media/scan/LegacyMediaScanner.java
+++ b/src/com/android/providers/media/scan/LegacyMediaScanner.java
@@ -19,8 +19,6 @@
import android.content.Context;
import android.net.Uri;
-import androidx.annotation.Nullable;
-
import com.android.providers.media.MediaVolume;
import java.io.File;
@@ -49,11 +47,6 @@
}
@Override
- public Uri scanFile(File file, int reason, @Nullable String ownerPackage) {
- throw new UnsupportedOperationException();
- }
-
- @Override
public void onDetachVolume(MediaVolume volume) {
throw new UnsupportedOperationException();
}
diff --git a/src/com/android/providers/media/scan/MediaScanner.java b/src/com/android/providers/media/scan/MediaScanner.java
index 45d2a24..eb5e4d6 100644
--- a/src/com/android/providers/media/scan/MediaScanner.java
+++ b/src/com/android/providers/media/scan/MediaScanner.java
@@ -24,23 +24,20 @@
import android.content.Context;
import android.net.Uri;
-import androidx.annotation.Nullable;
-
import com.android.providers.media.MediaVolume;
import java.io.File;
public interface MediaScanner {
- public static final int REASON_UNKNOWN = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__UNKNOWN;
- public static final int REASON_MOUNTED = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__MOUNTED;
- public static final int REASON_DEMAND = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__DEMAND;
- public static final int REASON_IDLE = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__IDLE;
+ int REASON_UNKNOWN = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__UNKNOWN;
+ int REASON_MOUNTED = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__MOUNTED;
+ int REASON_DEMAND = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__DEMAND;
+ int REASON_IDLE = MEDIA_PROVIDER_SCAN_OCCURRED__REASON__IDLE;
- public Context getContext();
- public void scanDirectory(File file, int reason);
- public Uri scanFile(File file, int reason);
- public Uri scanFile(File file, int reason, @Nullable String ownerPackage);
- public void onDetachVolume(MediaVolume volume);
- public void onIdleScanStopped();
- public void onDirectoryDirty(File file);
+ Context getContext();
+ void scanDirectory(File file, int reason);
+ Uri scanFile(File file, int reason);
+ void onDetachVolume(MediaVolume volume);
+ void onIdleScanStopped();
+ void onDirectoryDirty(File file);
}
diff --git a/src/com/android/providers/media/scan/ModernMediaScanner.java b/src/com/android/providers/media/scan/ModernMediaScanner.java
index 41f53d5..81133f7 100644
--- a/src/com/android/providers/media/scan/ModernMediaScanner.java
+++ b/src/com/android/providers/media/scan/ModernMediaScanner.java
@@ -50,6 +50,8 @@
import static com.android.providers.media.util.Metrics.translateReason;
+import static java.util.Objects.requireNonNull;
+
import android.content.ContentProviderClient;
import android.content.ContentProviderOperation;
import android.content.ContentProviderResult;
@@ -236,23 +238,36 @@
}
@Override
- public void scanDirectory(File file, int reason) {
- try (Scan scan = new Scan(file, reason, /*ownerPackage*/ null)) {
+ public void scanDirectory(@NonNull File file, int reason) {
+ requireNonNull(file);
+ try {
+ file = file.getCanonicalFile();
+ } catch (IOException e) {
+ Log.e(TAG, "Couldn't canonicalize directory to scan" + file, e);
+ return;
+ }
+
+ try (Scan scan = new Scan(file, reason)) {
scan.run();
- } catch (OperationCanceledException ignored) {
} catch (FileNotFoundException e) {
- Log.e(TAG, "Couldn't find directory to scan", e) ;
+ Log.e(TAG, "Couldn't find directory to scan", e);
+ } catch (OperationCanceledException ignored) {
+ // No-op.
}
}
@Override
- public Uri scanFile(File file, int reason) {
- return scanFile(file, reason, /*ownerPackage*/ null);
- }
+ @Nullable
+ public Uri scanFile(@NonNull File file, int reason) {
+ requireNonNull(file);
+ try {
+ file = file.getCanonicalFile();
+ } catch (IOException e) {
+ Log.e(TAG, "Couldn't canonicalize file to scan" + file, e);
+ return null;
+ }
- @Override
- public Uri scanFile(File file, int reason, @Nullable String ownerPackage) {
- try (Scan scan = new Scan(file, reason, ownerPackage)) {
+ try (Scan scan = new Scan(file, reason)) {
scan.run();
return scan.getFirstResult();
} catch (OperationCanceledException ignored) {
@@ -286,10 +301,18 @@
}
@Override
- public void onDirectoryDirty(File dir) {
+ public void onDirectoryDirty(@NonNull File dir) {
+ requireNonNull(dir);
+ try {
+ dir = dir.getCanonicalFile();
+ } catch (IOException e) {
+ Log.e(TAG, "Couldn't canonicalize directory" + dir, e);
+ return;
+ }
+
synchronized (mPendingCleanDirectories) {
mPendingCleanDirectories.remove(dir.getPath());
- FileUtils.setDirectoryDirty(dir, /*isDirty*/ true);
+ FileUtils.setDirectoryDirty(dir, /* isDirty */ true);
}
}
@@ -320,7 +343,6 @@
private final String mVolumeName;
private final Uri mFilesUri;
private final CancellationSignal mSignal;
- private final String mOwnerPackage;
private final List<String> mExcludeDirs;
private final long mStartGeneration;
@@ -349,7 +371,7 @@
*/
private boolean mIsDirectoryTreeDirty;
- public Scan(File root, int reason, @Nullable String ownerPackage)
+ public Scan(File root, int reason)
throws FileNotFoundException {
Trace.beginSection("ctor");
@@ -371,7 +393,6 @@
mStartGeneration = MediaStore.getGeneration(mResolver, mVolumeName);
mSingleFile = mRoot.isFile();
- mOwnerPackage = ownerPackage;
mExcludeDirs = new ArrayList<>();
Trace.endSection();
@@ -800,10 +821,7 @@
}
if (op != null) {
op.withValue(FileColumns._MODIFIER, FileColumns._MODIFIER_MEDIA_SCAN);
- // Add owner package name to new insertions when package name is provided.
- if (op.build().isInsert() && !attrs.isDirectory() && mOwnerPackage != null) {
- op.withValue(MediaColumns.OWNER_PACKAGE_NAME, mOwnerPackage);
- }
+
// Force DRM files to be marked as DRM, since the lower level
// stack may not set this correctly
if (isDrm) {
diff --git a/src/com/android/providers/media/scan/NullMediaScanner.java b/src/com/android/providers/media/scan/NullMediaScanner.java
index 7a1a396..e53f964 100644
--- a/src/com/android/providers/media/scan/NullMediaScanner.java
+++ b/src/com/android/providers/media/scan/NullMediaScanner.java
@@ -57,12 +57,6 @@
}
@Override
- public Uri scanFile(File file, int reason, @Nullable String ownerPackage) {
- Log.w(TAG, "Ignoring scan request for " + file);
- return null;
- }
-
- @Override
public void onDetachVolume(MediaVolume volume) {
// Ignored
}
diff --git a/src/com/android/providers/media/util/FileUtils.java b/src/com/android/providers/media/util/FileUtils.java
index 097eca8..6c2695c 100644
--- a/src/com/android/providers/media/util/FileUtils.java
+++ b/src/com/android/providers/media/util/FileUtils.java
@@ -1110,18 +1110,25 @@
}
public static @Nullable String extractRelativePath(@Nullable String data) {
- data = getCanonicalPath(data);
if (data == null) return null;
- final Matcher matcher = PATTERN_RELATIVE_PATH.matcher(data);
+ final String path;
+ try {
+ path = getCanonicalPath(data);
+ } catch (IOException e) {
+ Log.d(TAG, "Unable to get canonical path from invalid data path: " + data, e);
+ return null;
+ }
+
+ final Matcher matcher = PATTERN_RELATIVE_PATH.matcher(path);
if (matcher.find()) {
- final int lastSlash = data.lastIndexOf('/');
+ final int lastSlash = path.lastIndexOf('/');
if (lastSlash == -1 || lastSlash < matcher.end()) {
// This is a file in the top-level directory, so relative path is "/"
// which is different than null, which means unknown path
return "/";
} else {
- return data.substring(matcher.end(), lastSlash + 1);
+ return path.substring(matcher.end(), lastSlash + 1);
}
} else {
return null;
@@ -1769,15 +1776,29 @@
return new File(file.getPath().replaceFirst(FUSE_FS_PREFIX, LOWER_FS_PREFIX));
}
- @Nullable
- private static String getCanonicalPath(@Nullable String path) {
- if (path == null) return null;
+ /**
+ * Returns the canonical {@link File} for the provided abstract pathname.
+ *
+ * @return The canonical pathname string denoting the same file or directory as this abstract
+ * pathname
+ * @see File#getCanonicalFile()
+ */
+ @NonNull
+ public static File getCanonicalFile(@NonNull String path) throws IOException {
+ Objects.requireNonNull(path);
+ return new File(path).getCanonicalFile();
+ }
- try {
- return new File(path).getCanonicalPath();
- } catch (IOException e) {
- Log.d(TAG, "Unable to get canonical path from invalid data path: " + path, e);
- return null;
- }
+ /**
+ * Returns the canonical pathname string of the provided abstract pathname.
+ *
+ * @return The canonical pathname string denoting the same file or directory as this abstract
+ * pathname.
+ * @see File#getCanonicalPath()
+ */
+ @NonNull
+ public static String getCanonicalPath(@NonNull String path) throws IOException {
+ Objects.requireNonNull(path);
+ return new File(path).getCanonicalPath();
}
}
diff --git a/tests/src/com/android/providers/media/scan/LegacyMediaScannerTest.java b/tests/src/com/android/providers/media/scan/LegacyMediaScannerTest.java
index cf9cb39..2831e96 100644
--- a/tests/src/com/android/providers/media/scan/LegacyMediaScannerTest.java
+++ b/tests/src/com/android/providers/media/scan/LegacyMediaScannerTest.java
@@ -48,12 +48,6 @@
} catch (UnsupportedOperationException expected) {
}
try {
- scanner.scanFile(new File("/dev/null"), MediaScanner.REASON_UNKNOWN,
- InstrumentationRegistry.getContext().getPackageName());
- fail();
- } catch (UnsupportedOperationException expected) {
- }
- try {
scanner.onDetachVolume(null);
fail();
} catch (UnsupportedOperationException expected) {
diff --git a/tests/src/com/android/providers/media/scan/ModernMediaScannerTest.java b/tests/src/com/android/providers/media/scan/ModernMediaScannerTest.java
index 0450c1c..06f10ed 100644
--- a/tests/src/com/android/providers/media/scan/ModernMediaScannerTest.java
+++ b/tests/src/com/android/providers/media/scan/ModernMediaScannerTest.java
@@ -779,24 +779,6 @@
assertThat(mModern.scanFile(image, REASON_UNKNOWN)).isNull();
}
- @Test
- public void testScanFileAndUpdateOwnerPackageName() throws Exception {
- final File image = new File(mDir, "image.jpg");
- final String thisPackageName = InstrumentationRegistry.getContext().getPackageName();
- stage(R.raw.test_image, image);
-
- assertQueryCount(0, MediaStore.Images.Media.EXTERNAL_CONTENT_URI);
- // scanning the image file inserts new database entry with OWNER_PACKAGE_NAME as
- // thisPackageName.
- assertNotNull(mModern.scanFile(image, REASON_UNKNOWN, thisPackageName));
- try (Cursor cursor = mIsolatedResolver.query(MediaStore.Images.Media.EXTERNAL_CONTENT_URI,
- new String[] {MediaColumns.OWNER_PACKAGE_NAME}, null, null, null)) {
- assertEquals(1, cursor.getCount());
- cursor.moveToNext();
- assertEquals(thisPackageName, cursor.getString(0));
- }
- }
-
/**
* Verify fix for obscure bug which would cause us to delete files outside a
* directory that share a common prefix.
diff --git a/tests/src/com/android/providers/media/scan/NullMediaScannerTest.java b/tests/src/com/android/providers/media/scan/NullMediaScannerTest.java
index 063f1b7..265d1a9 100644
--- a/tests/src/com/android/providers/media/scan/NullMediaScannerTest.java
+++ b/tests/src/com/android/providers/media/scan/NullMediaScannerTest.java
@@ -38,8 +38,6 @@
scanner.scanDirectory(new File("/dev/null"), MediaScanner.REASON_UNKNOWN);
scanner.scanFile(new File("/dev/null"), MediaScanner.REASON_UNKNOWN);
- scanner.scanFile(new File("/dev/null"), MediaScanner.REASON_UNKNOWN,
- InstrumentationRegistry.getContext().getPackageName());
scanner.onDetachVolume(null);
}