Merge branch 'dev/11/fp3/security-aosp-rvc-release' into int/11/fp3
* dev/11/fp3/security-aosp-rvc-release:
Fix path traversal vulnerabilities in MediaProvider
Canonicalize file path for insertion by legacy apps
Remove invalid surrogates during bindSelection
Change-Id: I9581c7e5c1889e93f4fc068a2eff6bc5d43c4d0a
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 7db1e6d..689ce26 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -4601,31 +4601,44 @@
BackgroundThread.waitForIdle();
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);
+ }
+
+ final Bundle res = new Bundle();
+ res.putParcelable(Intent.EXTRA_STREAM, uri);
+ return res;
+ }
case MediaStore.SCAN_VOLUME_CALL: {
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;
- MediaService.onScanVolume(getContext(), volumeName, REASON_DEMAND);
- break;
- }
- }
- return res;
+ MediaService.onScanVolume(getContext(), volumeName, REASON_DEMAND);
} 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/ModernMediaScanner.java b/src/com/android/providers/media/scan/ModernMediaScanner.java
index 9658827..33a611b 100644
--- a/src/com/android/providers/media/scan/ModernMediaScanner.java
+++ b/src/com/android/providers/media/scan/ModernMediaScanner.java
@@ -47,6 +47,8 @@
import static android.text.format.DateUtils.HOUR_IN_MILLIS;
import static android.text.format.DateUtils.MINUTE_IN_MILLIS;
+import static java.util.Objects.requireNonNull;
+
import android.content.ContentProviderClient;
import android.content.ContentProviderOperation;
import android.content.ContentProviderResult;
@@ -221,7 +223,15 @@
}
@Override
- public void scanDirectory(File file, int reason) {
+ 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, /*ownerPackage*/ null)) {
scan.run();
} catch (OperationCanceledException ignored) {
@@ -229,12 +239,21 @@
}
@Override
- public Uri scanFile(File file, int reason) {
+ @Nullable
+ public Uri scanFile(@NonNull File file, int reason) {
return scanFile(file, reason, /*ownerPackage*/ null);
}
@Override
public Uri scanFile(File file, int reason, @Nullable String ownerPackage) {
+ requireNonNull(file);
+ try {
+ file = file.getCanonicalFile();
+ } catch (IOException e) {
+ Log.e(TAG, "Couldn't canonicalize file to scan" + file, e);
+ return null;
+ }
+
try (Scan scan = new Scan(file, reason, ownerPackage)) {
scan.run();
return scan.getFirstResult();
diff --git a/src/com/android/providers/media/util/DatabaseUtils.java b/src/com/android/providers/media/util/DatabaseUtils.java
index 66b7751..e6e7ffa 100644
--- a/src/com/android/providers/media/util/DatabaseUtils.java
+++ b/src/com/android/providers/media/util/DatabaseUtils.java
@@ -127,8 +127,9 @@
res.append(((Boolean) arg).booleanValue() ? 1 : 0);
} else {
res.append('\'');
- // Escape single quote character while appending the string.
- res.append(arg.toString().replace("'", "''"));
+ // Escape single quote character while appending the string and reject
+ // invalid unicode.
+ res.append(escapeSingleQuoteAndRejectInvalidUnicode(arg.toString()));
res.append('\'');
}
break;
@@ -142,6 +143,37 @@
return res.toString();
}
+ private static String escapeSingleQuoteAndRejectInvalidUnicode(@NonNull String target) {
+ final int len = target.length();
+ final StringBuilder res = new StringBuilder(len);
+ boolean lastHigh = false;
+
+ for (int i = 0; i < len; ) {
+ final char c = target.charAt(i++);
+
+ if (lastHigh != Character.isLowSurrogate(c)) {
+ Log.e(TAG, "Invalid surrogate in string " + target);
+ throw new IllegalArgumentException("Invalid surrogate in string " + target);
+ }
+
+ lastHigh = Character.isHighSurrogate(c);
+
+ // Escape the single quotes by duplicating them
+ if (c == '\'') {
+ res.append(c);
+ }
+
+ res.append(c);
+ }
+
+ if (lastHigh) {
+ Log.e(TAG, "Invalid surrogate in string " + target);
+ throw new IllegalArgumentException("Invalid surrogate in string " + target);
+ }
+
+ return res.toString();
+ }
+
/**
* Returns data type of the given object's value.
*<p>
diff --git a/src/com/android/providers/media/util/FileUtils.java b/src/com/android/providers/media/util/FileUtils.java
index 7bd1e4e..d100e57 100644
--- a/src/com/android/providers/media/util/FileUtils.java
+++ b/src/com/android/providers/media/util/FileUtils.java
@@ -975,18 +975,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;
@@ -1138,9 +1145,17 @@
values.remove(MediaColumns.BUCKET_ID);
values.remove(MediaColumns.BUCKET_DISPLAY_NAME);
- final String data = values.getAsString(MediaColumns.DATA);
+ String data = values.getAsString(MediaColumns.DATA);
if (TextUtils.isEmpty(data)) return;
+ try {
+ data = new File(data).getCanonicalPath();
+ values.put(MediaColumns.DATA, data);
+ } catch (IOException e) {
+ throw new IllegalArgumentException(
+ String.format(Locale.ROOT, "Invalid file path:%s in request.", data));
+ }
+
final File file = new File(data);
final File fileLower = new File(data.toLowerCase(Locale.ROOT));
@@ -1381,15 +1396,29 @@
return status;
}
- @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/util/DatabaseUtilsTest.java b/tests/src/com/android/providers/media/util/DatabaseUtilsTest.java
index 685d897..c547f54 100644
--- a/tests/src/com/android/providers/media/util/DatabaseUtilsTest.java
+++ b/tests/src/com/android/providers/media/util/DatabaseUtilsTest.java
@@ -127,6 +127,29 @@
}
@Test
+ public void testBindSelection_RejectInvalidUnicode() {
+ try {
+ bindSelection("DATA=?", "Fo\uD83Do");
+ fail();
+ } catch (IllegalArgumentException ignored) {
+ }
+
+ try {
+ bindSelection("DATA=?", "Fo\uDE00o");
+ fail();
+ } catch (IllegalArgumentException ignored) {
+ }
+
+ assertEquals("DATA='Fo\uD83D\uDE00o'", bindSelection("DATA=?", "Fo\uD83D\uDE00o"));
+
+ try {
+ bindSelection("DATA=?", "Fo\uDE00\uD83Do");
+ fail();
+ } catch (IllegalArgumentException ignored) {
+ }
+ }
+
+ @Test
public void testResolveQueryArgs_GroupBy() throws Exception {
args.putStringArray(QUERY_ARG_GROUP_COLUMNS, new String[] { "foo", "bar" });
args.putString(QUERY_ARG_SQL_GROUP_BY, "raw");