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

4 files changed, 191 insertions, 39 deletions

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.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 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());
+    }
+}