Merge pull request #3935 from TeamAmaze/bugfix/path-param-fixes

Sanitize parameters to prevent path injection
TeamAmaze · Oct 29, 2023 · e3647f1 · e3647f1
2 parents 8cb7f00 + c167e8b
commit e3647f1
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 10 deletions.
diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/FileUtil.java b/app/src/main/java/com/amaze/filemanager/filesystem/FileUtil.java
@@ -105,7 +105,7 @@ public static OutputStream getOutputStream(final File target, Context context)
   /** Writes uri stream from external application to the specified path */
   public static final void writeUriToStorage(
       @NonNull final MainActivity mainActivity,
-      @NonNull final ArrayList<Uri> uris,
+      @NonNull final List<Uri> uris,
       @NonNull final ContentResolver contentResolver,
       @NonNull final String currentPath) {
 

diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/root/MountPathCommand.kt b/app/src/main/java/com/amaze/filemanager/filesystem/root/MountPathCommand.kt
@@ -22,6 +22,7 @@ package com.amaze.filemanager.filesystem.root
 
 import android.os.Build
 import com.amaze.filemanager.fileoperations.exceptions.ShellNotRunningException
+import com.amaze.filemanager.filesystem.RootHelper
 import com.amaze.filemanager.filesystem.root.base.IRootCommand
 
 object MountPathCommand : IRootCommand() {
@@ -38,7 +39,8 @@ object MountPathCommand : IRootCommand() {
      * @return String the root of mount point that was ro, and mounted to rw; null otherwise
      */
     @Throws(ShellNotRunningException::class)
-    fun mountPath(path: String, operation: String): String? {
+    fun mountPath(pathArg: String, operation: String): String? {
+        val path = RootHelper.getCommandLineString(pathArg)
         return when (operation) {
             READ_WRITE -> mountReadWrite(path)
             READ_ONLY -> {
@@ -96,7 +98,9 @@ object MountPathCommand : IRootCommand() {
                 return if (mountOutput.isNotEmpty()) {
                     // command failed, and we got a reason echo'ed
                     null
-                } else mountPoint
+                } else {
+                    mountPoint
+                }
             }
         }
         return null

diff --git a/app/src/main/java/com/amaze/filemanager/ui/activities/MainActivity.java b/app/src/main/java/com/amaze/filemanager/ui/activities/MainActivity.java
@@ -247,7 +247,7 @@ public class MainActivity extends PermissionsActivity
   public ArrayList<String> oppatheList;
 
   // This holds the Uris to be written at initFabToSave()
-  private ArrayList<Uri> urisToBeSaved;
+  private List<Uri> urisToBeSaved;
 
   public static final String PASTEHELPER_BUNDLE = "pasteHelper";
 
@@ -630,9 +630,15 @@ private void checkForExternalIntent(Intent intent) {
       } else {
         // save a single file to filesystem
         Uri uri = intent.getParcelableExtra(Intent.EXTRA_STREAM);
-        ArrayList<Uri> uris = new ArrayList<>();
-        uris.add(uri);
-        initFabToSave(uris);
+        if (uri != null
+            && uri.getScheme() != null
+            && uri.getScheme().startsWith(ContentResolver.SCHEME_FILE)) {
+          ArrayList<Uri> uris = new ArrayList<>();
+          uris.add(uri);
+          initFabToSave(uris);
+        } else {
+          Toast.makeText(this, R.string.error_unsupported_or_null_uri, Toast.LENGTH_LONG).show();
+        }
       }
       // disable screen rotation just for convenience purpose
       // TODO: Support screen rotation when saving a file
@@ -651,7 +657,7 @@ private void checkForExternalIntent(Intent intent) {
   }
 
   /** Initializes the floating action button to act as to save data from an external intent */
-  private void initFabToSave(final ArrayList<Uri> uris) {
+  private void initFabToSave(final List<Uri> uris) {
     Utils.showThemedSnackbar(
         this,
         getString(R.string.select_save_location),
@@ -660,7 +666,7 @@ private void initFabToSave(final ArrayList<Uri> uris) {
         () -> saveExternalIntent(uris));
   }
 
-  private void saveExternalIntent(final ArrayList<Uri> uris) {
+  private void saveExternalIntent(final List<Uri> uris) {
     executeWithMainFragment(
         mainFragment -> {
           if (uris != null && uris.size() > 0) {

diff --git a/app/src/main/java/com/amaze/filemanager/ui/fragments/CompressedExplorerFragment.kt b/app/src/main/java/com/amaze/filemanager/ui/fragments/CompressedExplorerFragment.kt
@@ -287,7 +287,18 @@ class CompressedExplorerFragment : Fragment(), BottomBarButtonPath {
                 )?.run {
                     try {
                         if (moveToFirst()) {
-                            fileName = getString(0)
+                            fileName = getString(0).let {
+                                /*
+                                 * Strip any slashes to prevent possibility to access files outside
+                                 * cache dir if malicious ContentProvider gives malicious value
+                                 * of MediaStore.MediaColumns.DISPLAY_NAME when querying
+                                 */
+                                if (it.contains(File.pathSeparator)) {
+                                    it.substringAfterLast(File.pathSeparatorChar)
+                                } else {
+                                    it
+                                }
+                            }
                             compressedFile = File(requireContext().cacheDir, fileName)
                         } else {
                             // At this point, we know nothing the file the URI represents, we are doing everything

diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml
@@ -813,6 +813,7 @@ You only need to do this once, until the next time you select a new location for
     <string name="try_indexed_search">Try Indexed Search!</string>
     <string name="search_recent">Recent</string>
     <string name="results">Results</string>
+    <string name="error_unsupported_or_null_uri">Invalid, unsupported, or no URI was provided</string>
     <string name="complete_paste_warning">Please complete paste operation first</string>
     <string name="same_dir_move_error">Destination and source folder shouldn\'t match to move.</string>
     <string name="checking_conflicts">Checking for conflicts</string>