From 4f62898aa88878d1e62109aea21c966dcaa65dac Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Wed, 18 Jan 2023 11:31:28 +0000 Subject: [PATCH 1/4] Prevent path traversal vulnerability in TurboUriHelper --- .../dev/hotwire/turbo/util/TurboUriHelper.kt | 27 ++++++- .../hotwire/turbo/util/TurboUriHelperTest.kt | 79 +++++++++++++++++++ 2 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt index 145c69db..fe569d55 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt @@ -11,15 +11,19 @@ import java.io.IOException internal class TurboUriHelper(val context: Context) { @Suppress("BlockingMethodInNonBlockingContext") // https://youtrack.jetbrains.com/issue/KT-39684 - suspend fun writeFileTo(uri: Uri, directory: File): File? { + suspend fun writeFileTo(uri: Uri, destDirectory: File): File? { val uriAttributes = getAttributes(uri) ?: return null return withContext(dispatcherProvider.io) { - val file = File(directory, uriAttributes.fileName).also { - if (it.exists()) it.delete() - } + val file = File(destDirectory, uriAttributes.fileName) try { + file.checkForPathTraversalVulnerability(destDirectory) + + if (file.exists()) { + file.delete() + } + context.contentResolver.openInputStream(uri).use { val outputStream = file.outputStream() it?.copyTo(outputStream) @@ -104,6 +108,21 @@ internal class TurboUriHelper(val context: Context) { fileSize = 0 ) } + + /** + * Checks for path traversal vulnerability (caused e.g. by input filename containing "../") + * which could lead to modification of the destination directory. + * + * More information: https://developer.android.com/topic/security/risks/path-traversal + */ + private fun File.checkForPathTraversalVulnerability(destDirectory: File) { + val destinationDirectoryPath = destDirectory.canonicalPath + val outputFilePath = this.canonicalPath + + if (!outputFilePath.startsWith(destinationDirectoryPath)) { + throw IOException("Found Path Traversal Vulnerability with $outputFilePath") + } + } /** * Determine if the file points to an app resource. Symbolic link diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt new file mode 100644 index 00000000..0006cda1 --- /dev/null +++ b/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt @@ -0,0 +1,79 @@ +package dev.hotwire.turbo.util + +import android.content.Context +import android.net.Uri +import android.os.Build +import androidx.test.core.app.ApplicationProvider +import dev.hotwire.turbo.BaseUnitTest +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows +import org.robolectric.annotation.Config +import java.io.File + +@ExperimentalCoroutinesApi +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [Build.VERSION_CODES.O]) +class TurboUriHelperTest : BaseUnitTest() { + + private val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher() + + private lateinit var context: Context + private lateinit var turboUriHelper: TurboUriHelper + + @Before + override fun setup() { + super.setup() + Dispatchers.setMain(testDispatcher) + dispatcherProvider.io = Dispatchers.Main + + context = ApplicationProvider.getApplicationContext() + turboUriHelper = TurboUriHelper(context) + } + + override fun teardown() { + super.teardown() + Dispatchers.resetMain() + testDispatcher.cleanupTestCoroutines() + } + + @Test + fun validUriIsWrittenToFileSuccessfully() = runTest { + val inputFile = File("/tmp/file.txt") + val inputFileUri = Uri.fromFile(inputFile) + Shadows.shadowOf(context.contentResolver).registerInputStream(inputFileUri, "fileContent".byteInputStream()) + + val destFile = turboUriHelper.writeFileTo(inputFileUri, TurboFileProvider.directory(context)) + + assertThat(destFile).isNotNull() + } + + @Test + fun pathTraversingUriIsFailsToWriteToFileV1() = runTest { + val inputFileUri = Uri.parse("../../tmp/file.txt") + Shadows.shadowOf(context.contentResolver).registerInputStream(inputFileUri, "fileContent".byteInputStream()) + + val destFile = turboUriHelper.writeFileTo(inputFileUri, TurboFileProvider.directory(context)) + + assertThat(destFile).isNull() + } + + @Test + fun pathTraversingUriIsFailsToWriteToFileV2() = runTest { + val inputFileUri = Uri.parse("content://malicious.app?path=/data/data/malicious.app/files/file.txt&name=../../file.txt") + Shadows.shadowOf(context.contentResolver).registerInputStream(inputFileUri, "fileContent".byteInputStream()) + + val destFile = turboUriHelper.writeFileTo(inputFileUri, TurboFileProvider.directory(context)) + + assertThat(destFile).isNull() + } +} \ No newline at end of file From 9207081fb993dab06d04a5d24427181e718f364b Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Wed, 18 Jan 2023 13:57:01 +0000 Subject: [PATCH 2/4] Move file initialisation into try/catch block --- turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt index fe569d55..bd21165e 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt @@ -15,9 +15,8 @@ internal class TurboUriHelper(val context: Context) { val uriAttributes = getAttributes(uri) ?: return null return withContext(dispatcherProvider.io) { - val file = File(destDirectory, uriAttributes.fileName) - try { + val file = File(destDirectory, uriAttributes.fileName) file.checkForPathTraversalVulnerability(destDirectory) if (file.exists()) { From 3eb0453b5e9d66c91e9e58012040847f8955a95e Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Wed, 18 Jan 2023 13:57:19 +0000 Subject: [PATCH 3/4] Improve test names --- .../test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt b/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt index 0006cda1..d622095f 100644 --- a/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt +++ b/turbo/src/test/kotlin/dev/hotwire/turbo/util/TurboUriHelperTest.kt @@ -58,7 +58,7 @@ class TurboUriHelperTest : BaseUnitTest() { } @Test - fun pathTraversingUriIsFailsToWriteToFileV1() = runTest { + fun pathTraversingUriWithRelativePathFailsToWriteToFile() = runTest { val inputFileUri = Uri.parse("../../tmp/file.txt") Shadows.shadowOf(context.contentResolver).registerInputStream(inputFileUri, "fileContent".byteInputStream()) @@ -68,7 +68,7 @@ class TurboUriHelperTest : BaseUnitTest() { } @Test - fun pathTraversingUriIsFailsToWriteToFileV2() = runTest { + fun pathTraversingUriWithNameArgFailsToWriteToFile() = runTest { val inputFileUri = Uri.parse("content://malicious.app?path=/data/data/malicious.app/files/file.txt&name=../../file.txt") Shadows.shadowOf(context.contentResolver).registerInputStream(inputFileUri, "fileContent".byteInputStream()) From 7993b6bc1df767025d30ec69f41883ce7f182573 Mon Sep 17 00:00:00 2001 From: Milan Barta Date: Wed, 18 Jan 2023 14:32:35 +0000 Subject: [PATCH 4/4] Change the path traversal function to return boolean --- .../dev/hotwire/turbo/util/TurboUriHelper.kt | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt index bd21165e..e8b523e7 100644 --- a/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt +++ b/turbo/src/main/kotlin/dev/hotwire/turbo/util/TurboUriHelper.kt @@ -13,12 +13,14 @@ internal class TurboUriHelper(val context: Context) { @Suppress("BlockingMethodInNonBlockingContext") // https://youtrack.jetbrains.com/issue/KT-39684 suspend fun writeFileTo(uri: Uri, destDirectory: File): File? { val uriAttributes = getAttributes(uri) ?: return null + val file = File(destDirectory, uriAttributes.fileName) + + if (file.hasPathTraversalVulnerability(destDirectory)) { + return null + } return withContext(dispatcherProvider.io) { try { - val file = File(destDirectory, uriAttributes.fileName) - file.checkForPathTraversalVulnerability(destDirectory) - if (file.exists()) { file.delete() } @@ -114,12 +116,15 @@ internal class TurboUriHelper(val context: Context) { * * More information: https://developer.android.com/topic/security/risks/path-traversal */ - private fun File.checkForPathTraversalVulnerability(destDirectory: File) { - val destinationDirectoryPath = destDirectory.canonicalPath - val outputFilePath = this.canonicalPath + private fun File.hasPathTraversalVulnerability(destDirectory: File): Boolean { + return try { + val destinationDirectoryPath = destDirectory.canonicalPath + val outputFilePath = this.canonicalPath - if (!outputFilePath.startsWith(destinationDirectoryPath)) { - throw IOException("Found Path Traversal Vulnerability with $outputFilePath") + !outputFilePath.startsWith(destinationDirectoryPath) + } catch (e: Exception) { + TurboLog.e("${e.message}") + false } }