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

6 files changed, 310 insertions, 45 deletions

diff --git a/core/java/android/os/FileUtils.java b/core/java/android/os/FileUtils.java
index fe47f5b84d3f..0a724a1d881f 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 @@ public class FileUtils {
         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 == '/') {
+            if (isValidExtFilenameChar(c)) {
+                res.append(c);
+            } else {
+                res.append('_');
+            }
+        }
+        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 true;
+        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 93e68eb65140..5c9e8130e01d 100644
--- a/core/tests/coretests/src/android/os/FileUtilsTest.java
+++ b/core/tests/coretests/src/android/os/FileUtilsTest.java
@@ -180,6 +180,51 @@ public class FileUtilsTest extends AndroidTestCase {
         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 066acac2c22d..073d9c7c3003 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.util.Log;
 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.ArrayList;
 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 @@ public class ExternalStorageProvider extends DocumentsProvider {
     @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 @@ public class ExternalStorageProvider extends DocumentsProvider {
                 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 @@ public class ExternalStorageProvider extends DocumentsProvider {
         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 000000000000..830731aac8e6
--- /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 000000000000..ffcd4998f1df
--- /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 000000000000..f980b60d54a6
--- /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());
+    }
+}