#3318 Improve validation of forwarded attachment file paths

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"))))

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<AttachmentEntity>,
     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?

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
      *

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<DownloadForwardedAttachmentsWorker>(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"
+  }
+}

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)
+    }
+  }
+}