[file_selector_android] Refactor interactions with ContentProvider provided filenames

packages/file_selector/file_selector_android/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.5.1+12
+
+* Fixes a security issue related to improperly trusting filenames provided by a `ContentProvider`.
+
 ## 0.5.1+11
 
 * Bumps androidx.annotation:annotation from 1.9.0 to 1.9.1.

packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java

@@ -4,6 +4,8 @@
 
 package dev.flutter.packages.file_selector_android;
 
+import static dev.flutter.packages.file_selector_android.FileUtils.FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH;
+
 import android.annotation.TargetApi;
 import android.app.Activity;
 import android.content.ClipData;
@@ -357,15 +359,40 @@ GeneratedFileSelectorApi.FileResponse toFileResponse(@NonNull Uri uri) {
       return null;
     }
 
-    final String uriPath =
-        FileUtils.getPathFromCopyOfFileFromUri(activityPluginBinding.getActivity(), uri);
+    String uriPath;
+    GeneratedFileSelectorApi.FileSelectorNativeException nativeError = null;
+
+    try {
+      uriPath = FileUtils.getPathFromCopyOfFileFromUri(activityPluginBinding.getActivity(), uri);
+    } catch (IOException e) {
+      // If closing the output stream fails, we cannot be sure that the
+      // target file was written in full. Flushing the stream merely moves
+      // the bytes into the OS, not necessarily to the file.
+      uriPath = null;
+    } catch (SecurityException e) {
+      // Calling `ContentResolver#openInputStream()` has been reported to throw a
+      // `SecurityException` on some devices in certain circumstances. Instead of crashing, we
+      // return `null`.
+      //
+      // See https://github.com/flutter/flutter/issues/100025 for more details.
+      uriPath = null;
+    } catch (IllegalArgumentException e) {
+      uriPath = FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH;
+      nativeError =
+          new GeneratedFileSelectorApi.FileSelectorNativeException.Builder()
+              .setMessage(e.getMessage() == null ? "" : e.getMessage())
+              .setFileSelectorExceptionCode(
+                  GeneratedFileSelectorApi.FileSelectorExceptionCode.ILLEGAL_ARGUMENT_EXCEPTION)
+              .build();
+    }
 
     return new GeneratedFileSelectorApi.FileResponse.Builder()
         .setName(name)
         .setBytes(bytes)
         .setPath(uriPath)
         .setMimeType(contentResolver.getType(uri))
         .setSize(size.longValue())
+        .setFileSelectorNativeException(nativeError)
         .build();
   }
 }

packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java

@@ -45,6 +45,8 @@ public class FileUtils {
   /** URI authority that represents access to external storage providers. */
   public static final String EXTERNAL_DOCUMENT_AUTHORITY = "com.android.externalstorage.documents";
 
+  public static final String FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH = "FILE_SELECTOR_EXCEPTION";
+
   /**
    * Retrieves path of directory represented by the specified {@code Uri}.
    *
@@ -98,6 +100,12 @@ public static String getPathFromUri(@NonNull Context context, @NonNull Uri uri)
    * Copies the file from the given content URI to a temporary directory, retaining the original
    * file name if possible.
    *
+   * <p>If the filename contains path indirection or separators (.. or /), the end file name will be
+   * the segment after the final separator, with indirection replaced by underscores. E.g.
+   * "example/../..file.png" -> "_file.png". See: <a
+   * href="https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename">Improperly
+   * trusting ContentProvider-provided filename</a>.
+   *
    * <p>Each file is placed in its own directory to avoid conflicts according to the following
    * scheme: {cacheDir}/{randomUuid}/{fileName}
    *
@@ -111,7 +119,8 @@ public static String getPathFromUri(@NonNull Context context, @NonNull Uri uri)
    * or if a security exception is encountered when opening the input stream to start the copying.
    */
   @Nullable
-  public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @NonNull Uri uri) {
+  public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @NonNull Uri uri)
+      throws IOException, SecurityException, IllegalArgumentException {
     try (InputStream inputStream = context.getContentResolver().openInputStream(uri)) {
       String uuid = UUID.nameUUIDFromBytes(uri.toString().getBytes()).toString();
       File targetDirectory = new File(context.getCacheDir(), uuid);
@@ -122,32 +131,21 @@ public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @Non
 
       if (fileName == null) {
         if (extension == null) {
-          throw new IllegalArgumentException("No name nor extension found for file.");
+          throw new IllegalStateException("No name nor extension found for file.");
         } else {
           fileName = "file_selector" + extension;
         }
       } else if (extension != null) {
         fileName = getBaseName(fileName) + extension;
       }
 
-      File file = new File(targetDirectory, fileName);
+      String filePath = new File(targetDirectory, fileName).getPath();
+      File outputFile = saferOpenFile(filePath, targetDirectory.getCanonicalPath());
 
-      try (OutputStream outputStream = new FileOutputStream(file)) {
+      try (OutputStream outputStream = new FileOutputStream(outputFile)) {
         copy(inputStream, outputStream);
-        return file.getPath();
+        return outputFile.getPath();
       }
-    } catch (IOException e) {
-      // If closing the output stream fails, we cannot be sure that the
-      // target file was written in full. Flushing the stream merely moves
-      // the bytes into the OS, not necessarily to the file.
-      return null;
-    } catch (SecurityException e) {
-      // Calling `ContentResolver#openInputStream()` has been reported to throw a
-      // `SecurityException` on some devices in certain circumstances. Instead of crashing, we
-      // return `null`.
-      //
-      // See https://github.com/flutter/flutter/issues/100025 for more details.
-      return null;
     }
   }
 
@@ -172,14 +170,17 @@ private static String getFileExtension(Context context, Uri uriFile) {
       return null;
     }
 
-    return "." + extension;
+    return "." + sanitizeFilename(extension);
   }
 
   /** Returns the name of the file provided by ContentResolver; this may be null. */
   private static String getFileName(Context context, Uri uriFile) {
     try (Cursor cursor = queryFileName(context, uriFile)) {
-      if (cursor == null || !cursor.moveToFirst() || cursor.getColumnCount() < 1) return null;
-      return cursor.getString(0);
+      if (cursor == null || !cursor.moveToFirst() || cursor.getColumnCount() < 1) {
+        return null;
+      }
+      String unsanitizedFileName = cursor.getString(0);
+      return sanitizeFilename(unsanitizedFileName);
     }
   }
 
@@ -206,4 +207,38 @@ private static String getBaseName(String fileName) {
     // Basename is everything before the last '.'.
     return fileName.substring(0, lastDotIndex);
   }
+
+  // From https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename#sanitize-provided-filenames.
+  protected static @Nullable String sanitizeFilename(@Nullable String displayName) {
+    if (displayName == null) {
+      return null;
+    }
+
+    String[] badCharacters = new String[] {"..", "/"};
+    String[] segments = displayName.split("/");
+    String fileName = segments[segments.length - 1];
+    for (String suspString : badCharacters) {
+      fileName = fileName.replace(suspString, "_");
+    }
+    return fileName;
+  }
+
+  /**
+   * Use with file name sanatization and an non-guessable directory. From
+   * https://developer.android.com/privacy-and-security/risks/path-traversal#path-traversal-mitigations.
+   */
+  protected static @NonNull File saferOpenFile(@NonNull String path, @NonNull String expectedDir)
+      throws IllegalArgumentException, IOException {
+    File f = new File(path);
+    String canonicalPath = f.getCanonicalPath();
+    if (!canonicalPath.startsWith(expectedDir)) {
+      throw new IllegalArgumentException(
+          "Trying to open path outside of the expected directory. File: "
+              + f.getCanonicalPath()
+              + " was expected to be within directory: "
+              + expectedDir
+              + ".");
+    }
+    return f;
+  }
 }