diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/CompressedHelper.java b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/CompressedHelper.java index 5d36ad0875..731d8bdc31 100644 --- a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/CompressedHelper.java +++ b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/CompressedHelper.java @@ -228,8 +228,27 @@ public static String getFileName(String compressedName) { } } - public static final boolean isEntryPathValid(String entryPath) { - return !entryPath.startsWith("..\\") && !entryPath.startsWith("../") && !entryPath.equals(".."); + public static boolean isEntryPathValid(String entryPath) { + if (entryPath == null || entryPath.isEmpty()) { + return false; + } + // Normalize path separators to handle both Unix and Windows-style paths. + String normalized = entryPath.replace('\\', '/'); + // Walk the path segments, tracking depth to detect escaping the archive root. + // A path like "dir/sub/.." is valid (it resolves to "dir"), but "../../evil" is not. + int depth = 0; + for (String segment : normalized.split("/")) { + if ("..".equals(segment)) { + depth--; + if (depth < 0) { + // Path would escape the archive root. + return false; + } + } else if (!segment.isEmpty() && !".".equals(segment)) { + depth++; + } + } + return true; } private static boolean isZip(String type) { diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/Extractor.java b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/Extractor.java index 3e1a5097a4..3e98b0acf1 100644 --- a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/Extractor.java +++ b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/Extractor.java @@ -23,6 +23,7 @@ import static com.amaze.filemanager.filesystem.compressed.CompressedHelper.SEPARATOR; import static com.amaze.filemanager.filesystem.compressed.CompressedHelper.SEPARATOR_CHAR; +import java.io.File; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; @@ -113,6 +114,27 @@ protected String fixEntryName(String entryName) { } } + /** + * Verifies that {@code outputFile} is contained within {@code outputDir}, guarding against + * zip-slip / path-traversal attacks. + * + * @throws IOException if the resolved canonical path of {@code outputFile} would land outside + * {@code outputDir}. + */ + protected static void checkEntryPath(File outputFile, String outputDir) throws IOException { + String canonicalOutput = outputFile.getCanonicalPath(); + String canonicalDir = new File(outputDir).getCanonicalPath() + File.separator; + if (!canonicalOutput.startsWith(canonicalDir) + && !canonicalOutput.equals(new File(outputDir).getCanonicalPath())) { + throw new IOException( + "Refusing to extract entry '" + + outputFile.getName() + + "' outside target directory '" + + canonicalDir + + "'"); + } + } + public static class EmptyArchiveNotice extends IOException {} public static class BadArchiveNotice extends IOException { diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/AbstractCommonsArchiveExtractor.kt b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/AbstractCommonsArchiveExtractor.kt index d9e8e63899..c030961fac 100644 --- a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/AbstractCommonsArchiveExtractor.kt +++ b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/AbstractCommonsArchiveExtractor.kt @@ -67,7 +67,7 @@ abstract class AbstractCommonsArchiveExtractor( } } } - if (archiveEntries.size > 0) { + if (archiveEntries.isNotEmpty()) { listener.onStart(totalBytes, archiveEntries[0].name) inputStream.close() inputStream = createFrom(FileInputStream(filePath)) @@ -96,11 +96,12 @@ abstract class AbstractCommonsArchiveExtractor( entry: ArchiveEntry, outputDir: String, ) { + val outputFile = File(outputDir, entry.name) + checkEntryPath(outputFile, outputDir) if (entry.isDirectory) { MakeDirectoryOperation.mkdir(File(outputDir, entry.name), context) return } - val outputFile = File(outputDir, entry.name) if (false == outputFile.parentFile?.exists()) { MakeDirectoryOperation.mkdir(outputFile.parentFile, context) } diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/SevenZipExtractor.kt b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/SevenZipExtractor.kt index b39c9db054..83e7a7b042 100644 --- a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/SevenZipExtractor.kt +++ b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/SevenZipExtractor.kt @@ -105,12 +105,12 @@ class SevenZipExtractor( entry: SevenZArchiveEntry, outputDir: String, ) { - val name = entry.name + val outputFile = File(outputDir, entry.name) + checkEntryPath(outputFile, outputDir) if (entry.isDirectory) { - MakeDirectoryOperation.mkdir(File(outputDir, name), context) + MakeDirectoryOperation.mkdir(File(outputDir, entry.name), context) return } - val outputFile = File(outputDir, name) if (!outputFile.parentFile.exists()) { MakeDirectoryOperation.mkdir(outputFile.parentFile, context) } diff --git a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/ZipExtractor.kt b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/ZipExtractor.kt index 368bbbf2b3..159cf3d772 100644 --- a/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/ZipExtractor.kt +++ b/app/src/main/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/ZipExtractor.kt @@ -21,7 +21,6 @@ package com.amaze.filemanager.filesystem.compressed.extractcontents.helpers import android.content.Context -import android.os.Build import com.amaze.filemanager.R import com.amaze.filemanager.application.AppConfig import com.amaze.filemanager.fileoperations.filesystem.compressed.ArchivePasswordCache @@ -47,8 +46,6 @@ class ZipExtractor( listener: OnUpdate, updatePosition: UpdatePosition, ) : Extractor(context, filePath, outputPath, listener, updatePosition) { - private val isRobolectricTest = Build.HARDWARE == "robolectric" - @Throws(IOException::class) override fun extractWithFilter(filter: Filter) { var totalBytes: Long = 0 @@ -110,11 +107,7 @@ class ZipExtractor( outputDir: String, ) { val outputFile = File(outputDir, fixEntryName(entry.fileName)) - if (!outputFile.canonicalPath.startsWith(outputDir) && - (isRobolectricTest && !outputFile.canonicalPath.startsWith("/private$outputDir")) - ) { - throw IOException("Incorrect ZipEntry path!") - } + checkEntryPath(outputFile, outputDir) if (entry.isDirectory) { // zip entry is a directory, return after creating new directory MakeDirectoryOperation.mkdir(outputFile, context) diff --git a/app/src/play/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/RarExtractor.kt b/app/src/play/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/RarExtractor.kt index 672298019f..3cb5ebe485 100644 --- a/app/src/play/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/RarExtractor.kt +++ b/app/src/play/java/com/amaze/filemanager/filesystem/compressed/extractcontents/helpers/RarExtractor.kt @@ -77,9 +77,11 @@ class RarExtractor( MainHeaderNullException::class.java.isAssignableFrom(it::class.java) -> { throw BadArchiveNotice(it) } + UnsupportedRarV5Exception::class.java.isAssignableFrom(it::class.java) -> { throw it } + else -> { throw PasswordRequiredException(filePath) } @@ -144,11 +146,7 @@ class RarExtractor( CompressedHelper.SEPARATOR, ) val outputFile = File(outputDir, name) - if (!outputFile.canonicalPath.startsWith(outputDir) && - (isRobolectricTest && !outputFile.canonicalPath.startsWith("/private$outputDir")) - ) { - throw IOException("Incorrect RAR FileHeader path!") - } + checkEntryPath(outputFile, outputDir) if (entry.isDirectory) { MakeDirectoryOperation.mkdir(outputFile, context) outputFile.setLastModified(entry.mTime.time) @@ -232,7 +230,12 @@ class RarExtractor( "\\\\".toRegex(), CompressedHelper.SEPARATOR, ) - extractEntry(context, archive, header, context.externalCacheDir!!.absolutePath) + extractEntry( + context, + archive, + header, + context.externalCacheDir!!.absolutePath, + ) return "${context.externalCacheDir!!.absolutePath}/$filename" } } diff --git a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/CompressedHelperTest.java b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/CompressedHelperTest.java index 8208508b35..0573930e05 100644 --- a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/CompressedHelperTest.java +++ b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/CompressedHelperTest.java @@ -279,4 +279,46 @@ public void getFileNameTest() throws Exception { // no path assertEquals("", CompressedHelper.getFileName("")); } + + /** + * isEntryPathValid() tests. + * + *

Validates that paths which resolve within the archive root are accepted, and that paths + * which would escape the archive root are rejected — including cases where {@code ..} components + * appear in the middle of an otherwise valid path. + */ + @Test + public void isEntryPathValidTest() { + // null / empty are always invalid + assertFalse(CompressedHelper.isEntryPathValid(null)); + assertFalse(CompressedHelper.isEntryPathValid("")); + + // simple valid paths + assertTrue(CompressedHelper.isEntryPathValid("test.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir/file.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir/sub/file.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir/")); + + // ".." that resolves back inside the root is VALID + // e.g. "dir/sub/.." resolves to "dir" — still inside the archive + assertTrue(CompressedHelper.isEntryPathValid("dir/sub/..")); + assertTrue(CompressedHelper.isEntryPathValid("dir/sub/../file.txt")); + assertTrue(CompressedHelper.isEntryPathValid("a/b/c/../../file.txt")); + + // paths that escape the archive root are INVALID + assertFalse(CompressedHelper.isEntryPathValid("../evil.txt")); + assertFalse(CompressedHelper.isEntryPathValid("../../evil.txt")); + assertFalse(CompressedHelper.isEntryPathValid("foo/../../evil.txt")); + assertFalse(CompressedHelper.isEntryPathValid("foo/../../../evil.txt")); + + // Windows-style separators are normalised first + assertFalse(CompressedHelper.isEntryPathValid("..\\evil.txt")); + assertFalse(CompressedHelper.isEntryPathValid("foo\\..\\..\\evil.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir\\sub\\file.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir\\sub\\..\\file.txt")); + + // "." segments are ignored (current directory) + assertTrue(CompressedHelper.isEntryPathValid("./test.txt")); + assertTrue(CompressedHelper.isEntryPathValid("dir/./file.txt")); + } } diff --git a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/SevenZipExtractorTest.kt b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/SevenZipExtractorTest.kt index d730b39644..2b6c7b7703 100644 --- a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/SevenZipExtractorTest.kt +++ b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/SevenZipExtractorTest.kt @@ -20,10 +20,66 @@ package com.amaze.filemanager.filesystem.compressed.extractcontents +import android.os.Environment +import androidx.test.core.app.ApplicationProvider +import com.amaze.filemanager.asynchronous.management.ServiceWatcherUtil import com.amaze.filemanager.filesystem.compressed.extractcontents.helpers.SevenZipExtractor +import org.junit.Assert.assertFalse +import org.junit.Assert.fail +import org.junit.Test +import java.io.File +import java.io.IOException open class SevenZipExtractorTest : AbstractArchiveExtractorTest() { override val archiveType: String = "7z" override fun extractorClass(): Class = SevenZipExtractor::class.java + + /** + * Verify that a 7-Zip archive carrying a path-traversal entry + * (../POC_7Z_PROOF.txt) is blocked by the canonical-path guard: + * - extractEverything() must throw IOException + * - no file is written outside the designated output directory + */ + @Test + fun testExtractMalicious7z() { + val maliciousArchive = File(Environment.getExternalStorageDirectory(), "malicious.7z") + val outputDir = Environment.getExternalStorageDirectory() + val extractor = + SevenZipExtractor( + ApplicationProvider.getApplicationContext(), + maliciousArchive.absolutePath, + outputDir.absolutePath, + object : Extractor.OnUpdate { + override fun onStart( + totalBytes: Long, + firstEntryName: String, + ) = Unit + + override fun onUpdate(entryPath: String) = Unit + + override fun isCancelled(): Boolean = false + + override fun onFinish() = Unit + }, + ServiceWatcherUtil.UPDATE_POSITION, + ) + + try { + extractor.extractEverything() + fail("Expected IOException: canonical-path guard must reject the traversal entry") + } catch (e: IOException) { + // Confirm the guard fired (not a generic bad-archive error) + assertFalse( + "Exception must not be a BadArchiveNotice", + e is Extractor.BadArchiveNotice, + ) + } + + // The malicious file must NOT have been written outside the output directory + assertFalse( + "Malicious file must not escape the output directory", + File(outputDir.parentFile, "POC_7Z_PROOF.txt").exists(), + ) + } } diff --git a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/TarGzExtractorTest.kt b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/TarGzExtractorTest.kt index 0f537554b4..bcf9805724 100644 --- a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/TarGzExtractorTest.kt +++ b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/TarGzExtractorTest.kt @@ -20,10 +20,67 @@ package com.amaze.filemanager.filesystem.compressed.extractcontents +import android.os.Environment +import androidx.test.core.app.ApplicationProvider +import com.amaze.filemanager.asynchronous.management.ServiceWatcherUtil import com.amaze.filemanager.filesystem.compressed.extractcontents.helpers.TarGzExtractor +import org.junit.Assert.assertFalse +import org.junit.Assert.fail +import org.junit.Test +import java.io.File +import java.io.IOException open class TarGzExtractorTest : AbstractArchiveExtractorTest() { override val archiveType: String = "tar.gz" override fun extractorClass(): Class = TarGzExtractor::class.java + + /** + * Verify that a tar.gz archive carrying a path-traversal entry + * (../POC_ZIPSLIP_PROOF.txt) is blocked by the canonical-path guard + * in AbstractCommonsArchiveExtractor: + * - extractEverything() must throw IOException + * - no file is written outside the designated output directory + */ + @Test + fun testExtractMaliciousTarGz() { + val maliciousArchive = File(Environment.getExternalStorageDirectory(), "malicious.tar.gz") + val outputDir = Environment.getExternalStorageDirectory() + val extractor = + TarGzExtractor( + ApplicationProvider.getApplicationContext(), + maliciousArchive.absolutePath, + outputDir.absolutePath, + object : Extractor.OnUpdate { + override fun onStart( + totalBytes: Long, + firstEntryName: String, + ) = Unit + + override fun onUpdate(entryPath: String) = Unit + + override fun isCancelled(): Boolean = false + + override fun onFinish() = Unit + }, + ServiceWatcherUtil.UPDATE_POSITION, + ) + + try { + extractor.extractEverything() + fail("Expected IOException: canonical-path guard must reject the traversal entry") + } catch (e: IOException) { + // Confirm the guard fired (not a generic bad-archive error) + assertFalse( + "Exception must not be a BadArchiveNotice", + e is Extractor.BadArchiveNotice, + ) + } + + // The malicious file must NOT have been written outside the output directory + assertFalse( + "Malicious file must not escape the output directory", + File(outputDir.parentFile, "POC_ZIPSLIP_PROOF.txt").exists(), + ) + } } diff --git a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/ZipExtractorTest.kt b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/ZipExtractorTest.kt index 418868998e..89f2ea2237 100644 --- a/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/ZipExtractorTest.kt +++ b/app/src/test/java/com/amaze/filemanager/filesystem/compressed/extractcontents/ZipExtractorTest.kt @@ -20,10 +20,72 @@ package com.amaze.filemanager.filesystem.compressed.extractcontents +import android.os.Environment +import androidx.test.core.app.ApplicationProvider +import com.amaze.filemanager.asynchronous.management.ServiceWatcherUtil import com.amaze.filemanager.filesystem.compressed.extractcontents.helpers.ZipExtractor +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.io.File class ZipExtractorTest : AbstractArchiveExtractorTest() { override val archiveType: String = "zip" override fun extractorClass(): Class = ZipExtractor::class.java + + /** + * Verify that a ZIP archive carrying a path-traversal entry + * (foo/../../POC_ZIP_PROOF.txt) is handled safely: + * - extraction completes without an exception + * - the offending entry is recorded in invalidArchiveEntries + * - no file is written outside the designated output directory + */ + @Test + fun testExtractMaliciousZip() { + val maliciousArchive = File(Environment.getExternalStorageDirectory(), "malicious.zip") + val outputDir = Environment.getExternalStorageDirectory() + val extractor = + ZipExtractor( + ApplicationProvider.getApplicationContext(), + maliciousArchive.absolutePath, + outputDir.absolutePath, + object : Extractor.OnUpdate { + override fun onStart( + totalBytes: Long, + firstEntryName: String, + ) = Unit + + override fun onUpdate(entryPath: String) = Unit + + override fun isCancelled(): Boolean = false + + override fun onFinish() = Unit + }, + ServiceWatcherUtil.UPDATE_POSITION, + ) + + // Extraction must succeed — path-traversal entries are quarantined, not thrown + extractor.extractEverything() + + // The traversal entry must be recorded as invalid … + assertTrue( + "Malicious path-traversal entry must be captured in invalidArchiveEntries", + extractor.invalidArchiveEntries.isNotEmpty(), + ) + assertTrue( + "invalidArchiveEntries must contain the POC entry", + extractor.invalidArchiveEntries.any { "POC_ZIP_PROOF" in it }, + ) + // … and must NOT have been written outside the output directory + val escapedFile = File(outputDir, "foo/../../POC_ZIP_PROOF.txt").canonicalFile + assertFalse( + "Malicious file must not escape the output directory", + escapedFile.exists(), + ) + assertFalse( + "Escaped file canonical path must not reside under output directory", + escapedFile.canonicalPath.startsWith(outputDir.canonicalPath), + ) + } } diff --git a/app/src/test/resources/malicious.7z b/app/src/test/resources/malicious.7z new file mode 100644 index 0000000000..5576b92418 Binary files /dev/null and b/app/src/test/resources/malicious.7z differ diff --git a/app/src/test/resources/malicious.rar b/app/src/test/resources/malicious.rar new file mode 100644 index 0000000000..a1ac941229 Binary files /dev/null and b/app/src/test/resources/malicious.rar differ diff --git a/app/src/test/resources/malicious.tar.gz b/app/src/test/resources/malicious.tar.gz new file mode 100644 index 0000000000..0139101ea6 Binary files /dev/null and b/app/src/test/resources/malicious.tar.gz differ diff --git a/app/src/test/resources/malicious.zip b/app/src/test/resources/malicious.zip new file mode 100644 index 0000000000..fe64c1a685 Binary files /dev/null and b/app/src/test/resources/malicious.zip differ diff --git a/app/src/testPlay/java/com/amaze/filemanager/filesystem/compressed/extractcontents/RarExtractorTest.kt b/app/src/testPlay/java/com/amaze/filemanager/filesystem/compressed/extractcontents/RarExtractorTest.kt index 81b4d7c32a..c9dc1376bf 100644 --- a/app/src/testPlay/java/com/amaze/filemanager/filesystem/compressed/extractcontents/RarExtractorTest.kt +++ b/app/src/testPlay/java/com/amaze/filemanager/filesystem/compressed/extractcontents/RarExtractorTest.kt @@ -27,6 +27,8 @@ import com.amaze.filemanager.asynchronous.management.ServiceWatcherUtil import com.amaze.filemanager.filesystem.compressed.extractcontents.helpers.RarExtractor import com.github.junrar.Archive import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Ignore import org.junit.Test import java.io.File @@ -89,6 +91,56 @@ class RarExtractorTest : AbstractArchiveExtractorTest() { } } + /** + * Verify that a RAR4 archive carrying a path-traversal entry + * (foo//../../POC_RAR_PROOF.txt/POC_RAR_PROOF.txt) is handled safely: + * - extraction completes without an exception + * - the offending entry is recorded in invalidArchiveEntries + * - no file is written outside the designated output directory + */ + @Test + fun testExtractMaliciousRar() { + val maliciousArchive = File(Environment.getExternalStorageDirectory(), "malicious.rar") + val outputDir = Environment.getExternalStorageDirectory() + val extractor = + RarExtractor( + ApplicationProvider.getApplicationContext(), + maliciousArchive.absolutePath, + outputDir.absolutePath, + object : Extractor.OnUpdate { + override fun onStart( + totalBytes: Long, + firstEntryName: String, + ) = Unit + + override fun onUpdate(entryPath: String) = Unit + + override fun isCancelled(): Boolean = false + + override fun onFinish() = Unit + }, + ServiceWatcherUtil.UPDATE_POSITION, + ) + + // Extraction must succeed — path-traversal entries are quarantined, not thrown + extractor.extractEverything() + + // The traversal entry must be recorded as invalid … + assertTrue( + "Malicious path-traversal entry must be captured in invalidArchiveEntries", + extractor.invalidArchiveEntries.isNotEmpty(), + ) + assertTrue( + "invalidArchiveEntries must contain the POC entry", + extractor.invalidArchiveEntries.any { "POC_RAR_PROOF" in it }, + ) + // … and must NOT have been written outside the output directory + assertFalse( + "Malicious file must not escape the output directory", + File(outputDir.parentFile, "POC_RAR_PROOF.txt").exists(), + ) + } + @Test @Ignore override fun testExtractBadArchive() = Unit }