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