From 0cce5355b45d835f95a8918b8b803fd977d374e4 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 26 Nov 2014 13:38:26 -0800 Subject: 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 --- .../externalstorage/ExternalStorageProvider.java | 111 +++++++++++++-------- packages/ExternalStorageProvider/tests/Android.mk | 16 +++ .../tests/AndroidManifest.xml | 13 +++ .../ExternalStorageProviderTest.java | 90 +++++++++++++++++ 4 files changed, 191 insertions(+), 39 deletions(-) create mode 100644 packages/ExternalStorageProvider/tests/Android.mk create mode 100644 packages/ExternalStorageProvider/tests/AndroidManifest.xml create mode 100644 packages/ExternalStorageProvider/tests/src/com/android/externalstorage/ExternalStorageProviderTest.java (limited to 'packages/ExternalStorageProvider') 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 @@ + + + + + + + + + + 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()); + } +} -- cgit v1.1