Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class SevenZipHelperCallable(
entries.addAll(
consolidate(
entriesMap.keys.filter {
it.startsWith(relativePath)
CompressedHelper.isEntryPathValid(it) && it.startsWith(relativePath)
},
if (relativePath == "") {
0
Expand Down Expand Up @@ -101,7 +101,7 @@ class SevenZipHelperCallable(
} catch (e: PasswordRequiredException) {
// this is so that the caller can use onError to ask the user for the password
throw e
} catch (e: IOException) {
} catch (_: IOException) {
throw ArchiveException(String.format("7zip archive %s is corrupt", filePath))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,7 @@ public boolean isCancelled() {
} else {
LOG.error("Error while extracting file " + compressedPath, e);
AppConfig.toast(getApplicationContext(), extractService.getString(R.string.error));
paused = true;
publishProgress(e);
return false;
}
} catch (Throwable unhandledException) {
LOG.error("Unhandled exception thrown", unhandledException);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,6 @@ public static Decompressor getCompressorInstance(@NonNull Context context, @NonN
// without the compression extension
decompressor = new UnknownCompressedFileDecompressor(context);
} else {
if (BuildConfig.DEBUG) {
throw new IllegalArgumentException("The compressed file has no way of opening it: " + file);
}

LOG.error("The compressed file has no way of opening it: " + file);
decompressor = null;
}
Expand Down Expand Up @@ -228,8 +224,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) {
Comment thread
EmmanuelMess marked this conversation as resolved.
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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -96,11 +96,12 @@ abstract class AbstractCommonsArchiveExtractor(
entry: ArchiveEntry,
outputDir: String,
) {
val outputFile = File(outputDir, entry.name)
Comment thread
EmmanuelMess marked this conversation as resolved.
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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (C) 2014-2021 Arpit Khurana <arpitkh96@gmail.com>, Vishal Nehra <vishalmeham2@gmail.com>,
* Emmanuel Messulam<emmanuelbendavid@gmail.com>, Raymond Lai <airwave209gt at gmail.com> and Contributors.
*
* This file is part of Amaze File Manager.
*
* Amaze File Manager is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.amaze.filemanager.asynchronous.asynctasks.compress

import android.os.Environment
import org.junit.Assert.assertFalse
import org.junit.Test
import java.io.File

class SevenZipHelperCallableMaliciousTest : AbstractCompressedHelperCallableTest() {
@Test
fun testRootDoesNotExposeParentFolderEntries() {

Check notice on line 30 in app/src/test/java/com/amaze/filemanager/asynchronous/asynctasks/compress/SevenZipHelperCallableMaliciousTest.kt

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

app/src/test/java/com/amaze/filemanager/asynchronous/asynctasks/compress/SevenZipHelperCallableMaliciousTest.kt#L30

The function testRootDoesNotExposeParentFolderEntries is missing documentation.
val archive = File(Environment.getExternalStorageDirectory(), "malicious.7z")
val result = SevenZipHelperCallable(archive.absolutePath, "", false).call()

assertFalse(result.any { it.name == ".." || it.name.startsWith("../") })
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import org.awaitility.Awaitility.await
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Before
import org.junit.Ignore
Expand Down Expand Up @@ -95,6 +96,8 @@ class ExtractServiceTest {
private val tarXzfile: File
private val tarBz2file: File
private val sevenZipfile: File
private val maliciousTarGzFile: File
private val maliciousSevenZipFile: File
private val passwordProtectedZipfile: File
private val passwordProtected7Zipfile: File
private val listPasswordProtected7Zipfile: File
Expand Down Expand Up @@ -126,6 +129,8 @@ class ExtractServiceTest {
tarXzfile = File(this, "test-archive.tar.xz")
tarBz2file = File(this, "test-archive.tar.bz2")
sevenZipfile = File(this, "test-archive.7z")
maliciousTarGzFile = File(this, "malicious.tar.gz")
maliciousSevenZipFile = File(this, "malicious.7z")
passwordProtectedZipfile = File(this, "test-archive-encrypted.zip")
passwordProtected7Zipfile = File(this, "test-archive-encrypted.7z")
listPasswordProtected7Zipfile = File(this, "test-archive-encrypted-list.7z")
Expand Down Expand Up @@ -163,6 +168,8 @@ class ExtractServiceTest {
getResourceAsStream("test-archive.tar.xz").copyTo(FileOutputStream(tarXzfile))
getResourceAsStream("test-archive.tar.bz2").copyTo(FileOutputStream(tarBz2file))
getResourceAsStream("test-archive.7z").copyTo(FileOutputStream(sevenZipfile))
getResourceAsStream("malicious.tar.gz").copyTo(FileOutputStream(maliciousTarGzFile))
getResourceAsStream("malicious.7z").copyTo(FileOutputStream(maliciousSevenZipFile))
getResourceAsStream("test-archive-encrypted.zip")
.copyTo(FileOutputStream(passwordProtectedZipfile))
getResourceAsStream("test-archive-encrypted.7z")
Expand Down Expand Up @@ -326,6 +333,39 @@ class ExtractServiceTest {
assertNull(ShadowToast.getTextOfLatestToast())
}

/**
* Test malicious 7z extraction exits with a regular error toast.
*/
@Test
fun testExtractMalicious7z() {
performTest(maliciousSevenZipFile)
ShadowLooper.idleMainLooper()
await().atMost(10, TimeUnit.SECONDS).until { ShadowToast.getLatestToast() != null }
assertErrorOrInvalidEntriesToast()
}

/**
* Test malicious tar.gz extraction exits with a regular error toast.
*/
@Test
fun testExtractMaliciousTarGz() {
performTest(maliciousTarGzFile)
ShadowLooper.idleMainLooper()
await().atMost(10, TimeUnit.SECONDS).until { ShadowToast.getLatestToast() != null }
assertErrorOrInvalidEntriesToast()
}

private fun assertErrorOrInvalidEntriesToast() {
val context = ApplicationProvider.getApplicationContext<Context>()
val latestToastText = ShadowToast.getTextOfLatestToast()
assertTrue(
setOf(
context.getString(R.string.error),
context.getString(R.string.multiple_invalid_archive_entries),
).contains(latestToastText),
)
}

/**
* Test password-protected zip.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,4 +279,46 @@ public void getFileNameTest() throws Exception {
// no path
assertEquals("", CompressedHelper.getFileName(""));
}

/**
* isEntryPathValid() tests.
*
* <p>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"));
}
}
Loading
Loading