Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -228,8 +228,11 @@ 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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mark valid paths that contain ".." as invalid. For example: dir1/dir2/.. is valid if dir1 is in the compressed file.

return !entryPath.startsWith("..\\")
&& !entryPath.startsWith("../")
&& !entryPath.equals("..")
&& !entryPath.contains("/../");
Comment thread
TranceLove marked this conversation as resolved.
Outdated
}

private static boolean isZip(String type) {
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 @@ -101,6 +101,9 @@ abstract class AbstractCommonsArchiveExtractor(
return
}
val outputFile = File(outputDir, entry.name)
if (!outputFile.canonicalPath.startsWith(File(outputDir).canonicalPath + File.separator)) {
Comment thread
TranceLove marked this conversation as resolved.
Outdated
throw IOException("Incorrect archive entry path: ${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 @@ -111,6 +111,9 @@ class SevenZipExtractor(
return
}
val outputFile = File(outputDir, name)
Comment thread
TranceLove marked this conversation as resolved.
Outdated
if (!outputFile.canonicalPath.startsWith(File(outputDir).canonicalPath + File.separator)) {
throw IOException("Incorrect 7z entry path: $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,9 +107,9 @@ class ZipExtractor(
outputDir: String,
) {
val outputFile = File(outputDir, fixEntryName(entry.fileName))
if (!outputFile.canonicalPath.startsWith(outputDir) &&
(isRobolectricTest && !outputFile.canonicalPath.startsWith("/private$outputDir"))
) {
val canonicalOutput = outputFile.canonicalPath
val canonicalDir = File(outputDir).canonicalPath + File.separator
if (!canonicalOutput.startsWith(canonicalDir)) {
throw IOException("Incorrect ZipEntry path!")
Comment thread
TranceLove marked this conversation as resolved.
Outdated
}
if (entry.isDirectory) {
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,9 +146,9 @@ class RarExtractor(
CompressedHelper.SEPARATOR,
)
val outputFile = File(outputDir, name)
if (!outputFile.canonicalPath.startsWith(outputDir) &&
(isRobolectricTest && !outputFile.canonicalPath.startsWith("/private$outputDir"))
) {
val canonicalOutput = outputFile.canonicalPath
val canonicalDir = File(outputDir).canonicalPath + File.separator
if (!canonicalOutput.startsWith(canonicalDir)) {
throw IOException("Incorrect RAR FileHeader path!")
}
if (entry.isDirectory) {
Expand Down Expand Up @@ -232,7 +234,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
Expand Up @@ -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<out Extractor?> = 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(),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<out Extractor?> = 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(),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.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<out Extractor?> = 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
assertFalse(
"Malicious file must not escape the output directory",
File(outputDir.parentFile, "POC_ZIP_PROOF.txt").exists(),
)
Comment thread
TranceLove marked this conversation as resolved.
}
}
Binary file added app/src/test/resources/malicious.7z
Binary file not shown.
Binary file added app/src/test/resources/malicious.tar.gz
Binary file not shown.
Binary file added app/src/test/resources/malicious.zip
Binary file not shown.
Loading