Sanitize display names, keep extensions intact.
When creating or renaming files on external storage, sanitize the
requested display names to be valid FAT filenames. This also fixes
a handful of directory traversal bugs.
Also relax logic around generating display names to allow any
extension which maps to the requested MIME type. Tests to verify.
Bug: 18512473, 18504132
Change-Id: I89e632019ee145f53d9d9d2050932f8939a756af
diff --git a/core/java/android/os/FileUtils.java b/core/java/android/os/FileUtils.java
index fe47f5b..0a724a1 100644
--- a/core/java/android/os/FileUtils.java
+++ b/core/java/android/os/FileUtils.java
@@ -17,9 +17,8 @@
package android.os;
import android.system.ErrnoException;
-import android.text.TextUtils;
import android.system.Os;
-import android.system.OsConstants;
+import android.text.TextUtils;
import android.util.Log;
import android.util.Slog;
@@ -403,20 +402,89 @@
return success;
}
+ private static boolean isValidExtFilenameChar(char c) {
+ switch (c) {
+ case '\0':
+ case '/':
+ return false;
+ default:
+ return true;
+ }
+ }
+
/**
- * Assert that given filename is valid on ext4.
+ * Check if given filename is valid for an ext4 filesystem.
*/
public static boolean isValidExtFilename(String name) {
+ return (name != null) && name.equals(buildValidExtFilename(name));
+ }
+
+ /**
+ * Mutate the given filename to make it valid for an ext4 filesystem,
+ * replacing any invalid characters with "_".
+ */
+ public static String buildValidExtFilename(String name) {
if (TextUtils.isEmpty(name) || ".".equals(name) || "..".equals(name)) {
- return false;
+ return "(invalid)";
}
+ final StringBuilder res = new StringBuilder(name.length());
for (int i = 0; i < name.length(); i++) {
final char c = name.charAt(i);
- if (c == '\0' || c == '/') {
- return false;
+ if (isValidExtFilenameChar(c)) {
+ res.append(c);
+ } else {
+ res.append('_');
}
}
- return true;
+ return res.toString();
+ }
+
+ private static boolean isValidFatFilenameChar(char c) {
+ if ((0x00 <= c && c <= 0x1f)) {
+ return false;
+ }
+ switch (c) {
+ case '"':
+ case '*':
+ case '/':
+ case ':':
+ case '<':
+ case '>':
+ case '?':
+ case '\\':
+ case '|':
+ case 0x7F:
+ return false;
+ default:
+ return true;
+ }
+ }
+
+ /**
+ * Check if given filename is valid for a FAT filesystem.
+ */
+ public static boolean isValidFatFilename(String name) {
+ return (name != null) && name.equals(buildValidFatFilename(name));
+ }
+
+ /**
+ * Mutate the given filename to make it valid for a FAT filesystem,
+ * replacing any invalid characters with "_".
+ */
+ public static String buildValidFatFilename(String name) {
+ if (TextUtils.isEmpty(name) || ".".equals(name) || "..".equals(name)) {
+ return "(invalid)";
+ }
+ final StringBuilder res = new StringBuilder(name.length());
+ for (int i = 0; i < name.length(); i++) {
+ final char c = name.charAt(i);
+ if (isValidFatFilenameChar(c)) {
+ res.append(c);
+ } else {
+ res.append('_');
+ }
+ }
+ return res.toString();
}
public static String rewriteAfterRename(File beforeDir, File afterDir, String path) {
diff --git a/core/tests/coretests/src/android/os/FileUtilsTest.java b/core/tests/coretests/src/android/os/FileUtilsTest.java
index 93e68eb..5c9e813 100644
--- a/core/tests/coretests/src/android/os/FileUtilsTest.java
+++ b/core/tests/coretests/src/android/os/FileUtilsTest.java
@@ -180,6 +180,51 @@
assertDirContents("file1", "file2");
}
+ public void testValidExtFilename() throws Exception {
+ assertTrue(FileUtils.isValidExtFilename("a"));
+ assertTrue(FileUtils.isValidExtFilename("foo.bar"));
+ assertTrue(FileUtils.isValidExtFilename("foo bar.baz"));
+ assertTrue(FileUtils.isValidExtFilename("foo.bar.baz"));
+ assertTrue(FileUtils.isValidExtFilename(".bar"));
+ assertTrue(FileUtils.isValidExtFilename("foo~!@#$%^&*()_[]{}+bar"));
+
+ assertFalse(FileUtils.isValidExtFilename(null));
+ assertFalse(FileUtils.isValidExtFilename("."));
+ assertFalse(FileUtils.isValidExtFilename("../foo"));
+ assertFalse(FileUtils.isValidExtFilename("/foo"));
+
+ assertEquals(".._foo", FileUtils.buildValidExtFilename("../foo"));
+ assertEquals("_foo", FileUtils.buildValidExtFilename("/foo"));
+ assertEquals("foo_bar", FileUtils.buildValidExtFilename("foo\0bar"));
+ assertEquals(".foo", FileUtils.buildValidExtFilename(".foo"));
+ assertEquals("foo.bar", FileUtils.buildValidExtFilename("foo.bar"));
+ }
+
+ public void testValidFatFilename() throws Exception {
+ assertTrue(FileUtils.isValidFatFilename("a"));
+ assertTrue(FileUtils.isValidFatFilename("foo bar.baz"));
+ assertTrue(FileUtils.isValidFatFilename("foo.bar.baz"));
+ assertTrue(FileUtils.isValidFatFilename(".bar"));
+ assertTrue(FileUtils.isValidFatFilename("foo.bar"));
+ assertTrue(FileUtils.isValidFatFilename("foo bar"));
+ assertTrue(FileUtils.isValidFatFilename("foo+bar"));
+ assertTrue(FileUtils.isValidFatFilename("foo,bar"));
+
+ assertFalse(FileUtils.isValidFatFilename("foo*bar"));
+ assertFalse(FileUtils.isValidFatFilename("foo?bar"));
+ assertFalse(FileUtils.isValidFatFilename("foo<bar"));
+ assertFalse(FileUtils.isValidFatFilename(null));
+ assertFalse(FileUtils.isValidFatFilename("."));
+ assertFalse(FileUtils.isValidFatFilename("../foo"));
+ assertFalse(FileUtils.isValidFatFilename("/foo"));
+
+ assertEquals(".._foo", FileUtils.buildValidFatFilename("../foo"));
+ assertEquals("_foo", FileUtils.buildValidFatFilename("/foo"));
+ assertEquals(".foo", FileUtils.buildValidFatFilename(".foo"));
+ assertEquals("foo.bar", FileUtils.buildValidFatFilename("foo.bar"));
+ assertEquals("foo_bar__baz", FileUtils.buildValidFatFilename("foo?bar**baz"));
+ }
+
private void touch(String name, long age) throws Exception {
final File file = new File(mDir, name);
file.createNewFile();
diff --git a/packages/ExternalStorageProvider/src/com/android/externalstorage/ExternalStorageProvider.java b/packages/ExternalStorageProvider/src/com/android/externalstorage/ExternalStorageProvider.java
index 066acac..073d9c7 100644
--- a/packages/ExternalStorageProvider/src/com/android/externalstorage/ExternalStorageProvider.java
+++ b/packages/ExternalStorageProvider/src/com/android/externalstorage/ExternalStorageProvider.java
@@ -43,6 +43,7 @@
import android.webkit.MimeTypeMap;
import com.android.internal.annotations.GuardedBy;
+import com.android.internal.annotations.VisibleForTesting;
import com.google.android.collect.Lists;
import com.google.android.collect.Maps;
@@ -53,6 +54,7 @@
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Map;
+import java.util.Objects;
public class ExternalStorageProvider extends DocumentsProvider {
private static final String TAG = "ExternalStorage";
@@ -313,27 +315,19 @@
@Override
public String createDocument(String docId, String mimeType, String displayName)
throws FileNotFoundException {
+ displayName = FileUtils.buildValidFatFilename(displayName);
+
final File parent = getFileForDocId(docId);
if (!parent.isDirectory()) {
throw new IllegalArgumentException("Parent document isn't a directory");
}
- File file;
+ final File file = buildUniqueFile(parent, mimeType, displayName);
if (Document.MIME_TYPE_DIR.equals(mimeType)) {
- file = new File(parent, displayName);
if (!file.mkdir()) {
throw new IllegalStateException("Failed to mkdir " + file);
}
} else {
- displayName = removeExtension(mimeType, displayName);
- file = new File(parent, addExtension(mimeType, displayName));
-
- // If conflicting file, try adding counter suffix
- int n = 0;
- while (file.exists() && n++ < 32) {
- file = new File(parent, addExtension(mimeType, displayName + " (" + n + ")"));
- }
-
try {
if (!file.createNewFile()) {
throw new IllegalStateException("Failed to touch " + file);
@@ -342,11 +336,78 @@
throw new IllegalStateException("Failed to touch " + file + ": " + e);
}
}
+
return getDocIdForFile(file);
}
+ private static File buildFile(File parent, String name, String ext) {
+ if (TextUtils.isEmpty(ext)) {
+ return new File(parent, name);
+ } else {
+ return new File(parent, name + "." + ext);
+ }
+ }
+
+ @VisibleForTesting
+ public static File buildUniqueFile(File parent, String mimeType, String displayName)
+ throws FileNotFoundException {
+ String name;
+ String ext;
+
+ if (Document.MIME_TYPE_DIR.equals(mimeType)) {
+ name = displayName;
+ ext = null;
+ } else {
+ String mimeTypeFromExt;
+
+ // Extract requested extension from display name
+ final int lastDot = displayName.lastIndexOf('.');
+ if (lastDot >= 0) {
+ name = displayName.substring(0, lastDot);
+ ext = displayName.substring(lastDot + 1);
+ mimeTypeFromExt = MimeTypeMap.getSingleton().getMimeTypeFromExtension(
+ ext.toLowerCase());
+ } else {
+ name = displayName;
+ ext = null;
+ mimeTypeFromExt = null;
+ }
+
+ if (mimeTypeFromExt == null) {
+ mimeTypeFromExt = "application/octet-stream";
+ }
+
+ final String extFromMimeType = MimeTypeMap.getSingleton().getExtensionFromMimeType(
+ mimeType);
+ if (Objects.equals(mimeType, mimeTypeFromExt) || Objects.equals(ext, extFromMimeType)) {
+ // Extension maps back to requested MIME type; allow it
+ } else {
+ // No match; insist that create file matches requested MIME
+ name = displayName;
+ ext = extFromMimeType;
+ }
+ }
+
+ File file = buildFile(parent, name, ext);
+
+ // If conflicting file, try adding counter suffix
+ int n = 0;
+ while (file.exists()) {
+ if (n++ >= 32) {
+ throw new FileNotFoundException("Failed to create unique file");
+ }
+ file = buildFile(parent, name + " (" + n + ")", ext);
+ }
+
+ return file;
+ }
+
@Override
public String renameDocument(String docId, String displayName) throws FileNotFoundException {
+ // Since this provider treats renames as generating a completely new
+ // docId, we're okay with letting the MIME type change.
+ displayName = FileUtils.buildValidFatFilename(displayName);
+
final File before = getFileForDocId(docId);
final File after = new File(before.getParentFile(), displayName);
if (after.exists()) {
@@ -482,34 +543,6 @@
return "application/octet-stream";
}
- /**
- * Remove file extension from name, but only if exact MIME type mapping
- * exists. This means we can reapply the extension later.
- */
- private static String removeExtension(String mimeType, String name) {
- final int lastDot = name.lastIndexOf('.');
- if (lastDot >= 0) {
- final String extension = name.substring(lastDot + 1).toLowerCase();
- final String nameMime = MimeTypeMap.getSingleton().getMimeTypeFromExtension(extension);
- if (mimeType.equals(nameMime)) {
- return name.substring(0, lastDot);
- }
- }
- return name;
- }
-
- /**
- * Add file extension to name, but only if exact MIME type mapping exists.
- */
- private static String addExtension(String mimeType, String name) {
- final String extension = MimeTypeMap.getSingleton()
- .getExtensionFromMimeType(mimeType);
- if (extension != null) {
- return name + "." + extension;
- }
- return name;
- }
-
private void startObserving(File file, Uri notifyUri) {
synchronized (mObservers) {
DirectoryObserver observer = mObservers.get(file);
diff --git a/packages/ExternalStorageProvider/tests/Android.mk b/packages/ExternalStorageProvider/tests/Android.mk
new file mode 100644
index 0000000..830731a
--- /dev/null
+++ b/packages/ExternalStorageProvider/tests/Android.mk
@@ -0,0 +1,16 @@
+
+LOCAL_PATH := $(call my-dir)
+include $(CLEAR_VARS)
+
+LOCAL_MODULE_TAGS := tests
+
+LOCAL_SRC_FILES := $(call all-java-files-under, src)
+
+LOCAL_JAVA_LIBRARIES := android.test.runner
+
+LOCAL_PACKAGE_NAME := ExternalStorageProviderTests
+LOCAL_INSTRUMENTATION_FOR := ExternalStorageProvider
+
+LOCAL_CERTIFICATE := platform
+
+include $(BUILD_PACKAGE)
diff --git a/packages/ExternalStorageProvider/tests/AndroidManifest.xml b/packages/ExternalStorageProvider/tests/AndroidManifest.xml
new file mode 100644
index 0000000..ffcd499
--- /dev/null
+++ b/packages/ExternalStorageProvider/tests/AndroidManifest.xml
@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+ package="com.android.externalstorage.tests">
+
+ <application>
+ <uses-library android:name="android.test.runner" />
+ </application>
+
+ <instrumentation android:name="android.test.InstrumentationTestRunner"
+ android:targetPackage="com.android.externalstorage"
+ android:label="Tests for ExternalStorageProvider" />
+
+</manifest>
diff --git a/packages/ExternalStorageProvider/tests/src/com/android/externalstorage/ExternalStorageProviderTest.java b/packages/ExternalStorageProvider/tests/src/com/android/externalstorage/ExternalStorageProviderTest.java
new file mode 100644
index 0000000..f980b60
--- /dev/null
+++ b/packages/ExternalStorageProvider/tests/src/com/android/externalstorage/ExternalStorageProviderTest.java
@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2013 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.externalstorage;
+
+import static com.android.externalstorage.ExternalStorageProvider.buildUniqueFile;
+
+import android.os.FileUtils;
+import android.provider.DocumentsContract.Document;
+import android.test.AndroidTestCase;
+import android.test.suitebuilder.annotation.MediumTest;
+
+import java.io.File;
+
+@MediumTest
+public class ExternalStorageProviderTest extends AndroidTestCase {
+
+ private File mTarget;
+
+ @Override
+ protected void setUp() throws Exception {
+ super.setUp();
+ mTarget = getContext().getFilesDir();
+ FileUtils.deleteContents(mTarget);
+ }
+
+ @Override
+ protected void tearDown() throws Exception {
+ super.tearDown();
+ FileUtils.deleteContents(mTarget);
+ }
+
+ public void testBuildUniqueFile_normal() throws Exception {
+ assertNameEquals("test.jpg", buildUniqueFile(mTarget, "image/jpeg", "test"));
+ assertNameEquals("test.jpg", buildUniqueFile(mTarget, "image/jpeg", "test.jpg"));
+ assertNameEquals("test.jpeg", buildUniqueFile(mTarget, "image/jpeg", "test.jpeg"));
+ assertNameEquals("TEst.JPeg", buildUniqueFile(mTarget, "image/jpeg", "TEst.JPeg"));
+ assertNameEquals("test.png.jpg", buildUniqueFile(mTarget, "image/jpeg", "test.png.jpg"));
+ assertNameEquals("test.png.jpg", buildUniqueFile(mTarget, "image/jpeg", "test.png"));
+
+ assertNameEquals("test.flac", buildUniqueFile(mTarget, "audio/flac", "test"));
+ assertNameEquals("test.flac", buildUniqueFile(mTarget, "audio/flac", "test.flac"));
+ assertNameEquals("test.flac", buildUniqueFile(mTarget, "application/x-flac", "test"));
+ assertNameEquals("test.flac", buildUniqueFile(mTarget, "application/x-flac", "test.flac"));
+ }
+
+ public void testBuildUniqueFile_unknown() throws Exception {
+ assertNameEquals("test", buildUniqueFile(mTarget, "application/octet-stream", "test"));
+ assertNameEquals("test.jpg", buildUniqueFile(mTarget, "application/octet-stream", "test.jpg"));
+ assertNameEquals(".test", buildUniqueFile(mTarget, "application/octet-stream", ".test"));
+
+ assertNameEquals("test", buildUniqueFile(mTarget, "lolz/lolz", "test"));
+ assertNameEquals("test.lolz", buildUniqueFile(mTarget, "lolz/lolz", "test.lolz"));
+ }
+
+ public void testBuildUniqueFile_dir() throws Exception {
+ assertNameEquals("test", buildUniqueFile(mTarget, Document.MIME_TYPE_DIR, "test"));
+ new File(mTarget, "test").mkdir();
+ assertNameEquals("test (1)", buildUniqueFile(mTarget, Document.MIME_TYPE_DIR, "test"));
+
+ assertNameEquals("test.jpg", buildUniqueFile(mTarget, Document.MIME_TYPE_DIR, "test.jpg"));
+ new File(mTarget, "test.jpg").mkdir();
+ assertNameEquals("test.jpg (1)", buildUniqueFile(mTarget, Document.MIME_TYPE_DIR, "test.jpg"));
+ }
+
+ public void testBuildUniqueFile_increment() throws Exception {
+ assertNameEquals("test.jpg", buildUniqueFile(mTarget, "image/jpeg", "test.jpg"));
+ new File(mTarget, "test.jpg").createNewFile();
+ assertNameEquals("test (1).jpg", buildUniqueFile(mTarget, "image/jpeg", "test.jpg"));
+ new File(mTarget, "test (1).jpg").createNewFile();
+ assertNameEquals("test (2).jpg", buildUniqueFile(mTarget, "image/jpeg", "test.jpg"));
+ }
+
+ private static void assertNameEquals(String expected, File actual) {
+ assertEquals(expected, actual.getName());
+ }
+}