From e0557977e046d9499041ce95218e02fca77032b5 Mon Sep 17 00:00:00 2001 From: martgil Date: Fri, 23 Oct 2026 18:41:29 +0800 Subject: [PATCH 1/3] fix: confine forwarded attachments to their cache directory --- .../DownloadForwardedAttachmentsWorker.kt | 11 +- .../email/util/FileAndDirectoryUtils.kt | 25 +++ .../email/util/GetFileInsideDirTest.kt | 144 ++++++++++++++++++ 3 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt 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..6d2b3aff24 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 @@ -34,6 +34,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 @@ -254,9 +255,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/util/GetFileInsideDirTest.kt b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt new file mode 100644 index 0000000000..37d87e86ad --- /dev/null +++ b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt @@ -0,0 +1,144 @@ +/* + * © 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 rejectSiblingSharingDirectoryNamePrefix() { + val root = temporaryFolder.newFolder("root") + val directory = File(root, "forwarded").apply { mkdirs() } + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "../forwarded-other/payload") + } + } + + @Test + fun rejectSymlinkEscapingDirectory() { + val directory = temporaryFolder.newFolder("forwarded") + val outsideDirectory = temporaryFolder.newFolder("outside") + try { + Files.createSymbolicLink(File(directory, "link").toPath(), outsideDirectory.toPath()) + } catch (e: Exception) { + assumeNoException(e) + } + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "link/flowcrypt.db-journal") + } + } + + @Test + fun rejectInvalidFileName() { + val directory = temporaryFolder.newFolder("forwarded") + + assertThrows(IOException::class.java) { + FileAndDirectoryUtils.getFileInsideDir(directory, "bad\u0000name") + } + } +} From 1cbc68f3eeeb15fbcee0b70148617474423d46f2 Mon Sep 17 00:00:00 2001 From: Denys Bondarenko Date: Thu, 5 Nov 2026 10:51:30 +0200 Subject: [PATCH 2/3] test: cover forwarded attachment path validation in DownloadForwardedAttachmentsWorker --- FlowCrypt/build.gradle.kts | 1 + .../DownloadForwardedAttachmentsWorker.kt | 4 +- .../DownloadForwardedAttachmentsWorkerTest.kt | 124 ++++++++++++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 FlowCrypt/src/test/java/com/flowcrypt/email/jetpack/workmanager/DownloadForwardedAttachmentsWorkerTest.kt 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 6d2b3aff24..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 @@ -243,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? 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" + } +} From de88f2116552b0051b8bae01df7d95c592a988d7 Mon Sep 17 00:00:00 2001 From: Denys Bondarenko Date: Thu, 5 Nov 2026 11:14:26 +0200 Subject: [PATCH 3/3] test: make symlink and sibling-prefix cases reach canonical path checks --- .../email/util/GetFileInsideDirTest.kt | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt index 37d87e86ad..c91e32b2ce 100644 --- a/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt +++ b/FlowCrypt/src/test/java/com/flowcrypt/email/util/GetFileInsideDirTest.kt @@ -109,27 +109,30 @@ class GetFileInsideDirTest { } @Test - fun rejectSiblingSharingDirectoryNamePrefix() { + 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, "../forwarded-other/payload") + FileAndDirectoryUtils.getFileInsideDir(directory, "link") } } @Test fun rejectSymlinkEscapingDirectory() { val directory = temporaryFolder.newFolder("forwarded") - val outsideDirectory = temporaryFolder.newFolder("outside") - try { - Files.createSymbolicLink(File(directory, "link").toPath(), outsideDirectory.toPath()) - } catch (e: Exception) { - assumeNoException(e) + val outsideFile = File(temporaryFolder.newFolder("outside"), "flowcrypt.db-journal").apply { + createNewFile() } + createSymbolicLinkOrSkip(File(directory, "link"), outsideFile) assertThrows(IOException::class.java) { - FileAndDirectoryUtils.getFileInsideDir(directory, "link/flowcrypt.db-journal") + FileAndDirectoryUtils.getFileInsideDir(directory, "link") } } @@ -141,4 +144,12 @@ class GetFileInsideDirTest { FileAndDirectoryUtils.getFileInsideDir(directory, "bad\u0000name") } } + + private fun createSymbolicLinkOrSkip(link: File, target: File) { + try { + Files.createSymbolicLink(link.toPath(), target.toPath()) + } catch (e: Exception) { + assumeNoException(e) + } + } }