diff --git a/FlowCrypt/build.gradle.kts b/FlowCrypt/build.gradle.kts index 52c29ea9f6..ee0f30528b 100644 --- a/FlowCrypt/build.gradle.kts +++ b/FlowCrypt/build.gradle.kts @@ -494,6 +494,7 @@ dependencies { testImplementation("org.jetbrains.kotlin:kotlin-reflect:2.4.21") testImplementation("junit:junit:4.13.2") testImplementation("androidx.room:room-testing:2.8.5") + testImplementation("androidx.work:work-testing:2.12.0") testImplementation("io.github.classgraph:classgraph:5.0.0") implementation(fileTree(mapOf("dir" to "libs", "include" to listOf("*.jar")))) diff --git a/FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorker.kt b/FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorker.kt index 6f10048355..d590837f25 100644 --- a/FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorker.kt +++ b/FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorker.kt @@ -7,6 +7,7 @@ package com.flowcrypt.email.jetpack.workmanager import android.content.Context import android.net.Uri +import androidx.annotation.VisibleForTesting import androidx.core.net.toUri import androidx.work.CoroutineWorker import androidx.work.ExistingWorkPolicy @@ -34,6 +35,7 @@ import org.apache.commons.io.FileUtils import org.eclipse.angus.mail.imap.IMAPFolder import java.io.File import java.io.FileOutputStream +import java.io.IOException import java.io.InputStream import java.util.UUID @@ -242,7 +244,8 @@ class DownloadForwardedAttachmentsWorker(context: Context, params: WorkerParamet } } - private suspend fun loadAttachments( + @VisibleForTesting + internal suspend fun loadAttachments( attachmentEntities: List, parentDirectory: File, action: suspend (attachmentEntity: AttachmentEntity) -> InputStream? @@ -254,9 +257,13 @@ class DownloadForwardedAttachmentsWorker(context: Context, params: WorkerParamet continue } - val attName = attachmentEntity.name - - val attFile = File(parentDirectory, attName) + val attFile = try { + FileAndDirectoryUtils.getFileInsideDir(parentDirectory, attachmentEntity.name) + } catch (e: IOException) { + LogsUtil.d(TAG, "Rejected unsafe forwarded attachment name", e) + msgState = MessageState.ERROR_ORIGINAL_ATTACHMENT_NOT_FOUND + break + } val exists = attFile.exists() var uri: Uri? diff --git a/FlowCrypt/src/main/java/com/flowcrypt/email/util/FileAndDirectoryUtils.kt b/FlowCrypt/src/main/java/com/flowcrypt/email/util/FileAndDirectoryUtils.kt index 0e9a9ef05e..d718791954 100644 --- a/FlowCrypt/src/main/java/com/flowcrypt/email/util/FileAndDirectoryUtils.kt +++ b/FlowCrypt/src/main/java/com/flowcrypt/email/util/FileAndDirectoryUtils.kt @@ -148,6 +148,31 @@ class FileAndDirectoryUtils { } } + /** + * Resolve a file name inside a directory and ensure the result stays strictly inside it. + * + * @param directory The directory that must contain the resolved file. + * @param fileName The untrusted file name. It must not contain path separators. + * @return The canonical file located inside [directory]. + * @throws IOException If the file name is invalid or resolves outside [directory]. + */ + @Throws(IOException::class) + fun getFileInsideDir(directory: File, fileName: String): File { + if (fileName.indexOf(File.separatorChar) >= 0) { + throw IOException("File name contains a path separator: '$fileName'") + } + + val canonicalDirectory = directory.canonicalFile + val file = File(canonicalDirectory, fileName).canonicalFile + val directoryPrefix = canonicalDirectory.path.trimEnd(File.separatorChar) + File.separator + + if (file.path == canonicalDirectory.path || !file.path.startsWith(directoryPrefix)) { + throw IOException("Resolved file '$fileName' is outside of '$canonicalDirectory'") + } + + return file + } + /** * Get a directory. Create a new directory if it doesn't exist * diff --git a/FlowCrypt/src/test/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorkerTest.kt b/FlowCrypt/src/test/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorkerTest.kt new file mode 100644 index 0000000000..1a571fb43d --- /dev/null +++ b/FlowCrypt/src/test/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorkerTest.kt @@ -0,0 +1,124 @@ +/* + * © 2016-present FlowCrypt a.s. Limitations apply. Contact human@flowcrypt.com + * Contributors: denbond7 + */ + +package com.flowcrypt.email.jetpack.workmanager + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.work.testing.TestListenableWorkerBuilder +import com.flowcrypt.email.BuildConfig +import com.flowcrypt.email.Constants +import com.flowcrypt.email.database.FlowCryptRoomDatabase +import com.flowcrypt.email.database.MessageState +import com.flowcrypt.email.database.entity.AccountEntity +import com.flowcrypt.email.database.entity.AttachmentEntity +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.io.ByteArrayInputStream +import java.io.File +import java.util.UUID + +/** + * Checks that [DownloadForwardedAttachmentsWorker] validates a forwarded attachment name + * before any file operation, so a crafted name can't escape the forwarded attachments directory. + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, minSdk = BuildConfig.MIN_SDK_VERSION) +class DownloadForwardedAttachmentsWorkerTest { + private lateinit var context: Context + private lateinit var worker: DownloadForwardedAttachmentsWorker + private lateinit var parentDirectory: File + private var isAttachmentRequested = false + + @Before + fun setUp() { + context = ApplicationProvider.getApplicationContext() + worker = TestListenableWorkerBuilder(context).build() + parentDirectory = File( + File(File(context.cacheDir, Constants.ATTACHMENTS_CACHE_DIR), Constants.FORWARDED_ATTACHMENTS_CACHE_DIR), + UUID.randomUUID().toString() + ).apply { mkdirs() } + isAttachmentRequested = false + } + + @Test + fun rejectDatabaseJournalTraversalWhenTargetIsAbsent() { + val name = "../../../../databases/flowcrypt.db-journal" + val target = File( + context.getDatabasePath(FlowCryptRoomDatabase.DB_NAME).parentFile, + "flowcrypt.db-journal" + ).canonicalFile + //make sure the crafted name really points to the database directory + assertEquals(target, File(parentDirectory, name).canonicalFile) + target.delete() + + val state = loadAttachment(name) + + assertEquals(MessageState.ERROR_ORIGINAL_ATTACHMENT_NOT_FOUND, state) + assertFalse(isAttachmentRequested) + assertFalse(target.exists()) + assertTrue(parentDirectory.listFiles().isNullOrEmpty()) + } + + @Test + fun rejectTraversalWhenTargetExists() { + val name = "../../escape-marker" + val target = File(parentDirectory, name).canonicalFile.apply { + parentFile?.mkdirs() + writeText(ORIGINAL_CONTENT) + } + + val state = loadAttachment(name) + + assertEquals(MessageState.ERROR_ORIGINAL_ATTACHMENT_NOT_FOUND, state) + assertFalse(isAttachmentRequested) + assertEquals(ORIGINAL_CONTENT, target.readText()) + } + + @Test + fun downloadAttachmentWithSafeNameInsideDirectory() { + val state = loadAttachment("photo.jpg") + + assertEquals(MessageState.QUEUED, state) + assertTrue(isAttachmentRequested) + assertEquals(PAYLOAD, File(parentDirectory, "photo.jpg").readText()) + } + + private fun loadAttachment(name: String): MessageState = runBlocking { + worker.loadAttachments(listOf(createForwardedAttachment(name)), parentDirectory) { + isAttachmentRequested = true + ByteArrayInputStream(PAYLOAD.toByteArray()) + } + } + + private fun createForwardedAttachment(name: String) = AttachmentEntity( + id = null, + account = "user@flowcrypt.test", + accountType = AccountEntity.ACCOUNT_TYPE_GOOGLE, + folder = "Outbox", + uid = 1, + name = name, + encodedSize = PAYLOAD.length.toLong(), + type = Constants.MIME_TYPE_BINARY_DATA, + attachmentId = null, + fileUri = null, + forwardedFolder = "INBOX", + forwardedUid = 1, + decryptWhenForward = false, + path = "0/1" + ) + + companion object { + private const val PAYLOAD = "attacker-controlled bytes" + private const val ORIGINAL_CONTENT = "original content" + } +} diff --git a/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt new file mode 100644 index 0000000000..c91e32b2ce --- /dev/null +++ b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt @@ -0,0 +1,155 @@ +/* + * © 2016-present FlowCrypt a.s. Limitations apply. Contact human@flowcrypt.com + */ + +package com.flowcrypt.email.util + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Assume.assumeNoException +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import java.io.File +import java.io.IOException +import java.nio.file.Files + +class GetFileInsideDirTest { + @get:Rule + val temporaryFolder: TemporaryFolder = TemporaryFolder() + + @Test + fun acceptFileDirectlyInsideDirectory() { + val directory = temporaryFolder.newFolder("forwarded") + + val file = FileAndDirectoryUtils.getFileInsideDir(directory, "photo.jpg") + + assertEquals(File(directory.canonicalFile, "photo.jpg").path, file.path) + } + + @Test + fun rejectDatabaseJournalTraversal() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir( + directory, + "../../../../databases/flowcrypt.db-journal" + ) + } + } + + @Test + fun rejectNestedTraversal() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir( + directory, + "subdir/../../databases/flowcrypt.db-journal" + ) + } + } + + @Test + fun rejectAbsolutePathName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir( + directory, + "/data/data/com.flowcrypt.email/databases/flowcrypt.db-journal" + ) + } + } + + @Test + fun rejectNestedPathName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "subdir/flowcrypt.db-journal") + } + } + + @Test + fun rejectTrailingPathSeparator() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "attachment/") + } + } + + @Test + fun rejectParentDirectoryName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "..") + } + } + + @Test + fun rejectCurrentDirectoryName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, ".") + } + } + + @Test + fun rejectEmptyName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "") + } + } + + @Test + fun rejectSymlinkToSiblingSharingDirectoryNamePrefix() { + val root = temporaryFolder.newFolder("root") + val directory = File(root, "forwarded").apply { mkdirs() } + val siblingFile = File(root, "forwarded-other/payload").apply { + parentFile?.mkdirs() + createNewFile() + } + createSymbolicLinkOrSkip(File(directory, "link"), siblingFile) + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "link") + } + } + + @Test + fun rejectSymlinkEscapingDirectory() { + val directory = temporaryFolder.newFolder("forwarded") + val outsideFile = File(temporaryFolder.newFolder("outside"), "flowcrypt.db-journal").apply { + createNewFile() + } + createSymbolicLinkOrSkip(File(directory, "link"), outsideFile) + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "link") + } + } + + @Test + fun rejectInvalidFileName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "bad\u0000name") + } + } + + private fun createSymbolicLinkOrSkip(link: File, target: File) { + try { + Files.createSymbolicLink(link.toPath(), target.toPath()) + } catch (e: Exception) { + assumeNoException(e) + } + } +}