From 3b94f7eb7be94dc2c7ebdf12b57ef1c2505f5add Mon Sep 17 00:00:00 2001 From: Mohamad Jaara Date: Thu, 16 Nov 2023 18:07:52 +0100 Subject: [PATCH] fix: validate the list of allowed file names when extracting files form zip folder (#2221) (#2230) * fix: validate the list of allowed file names when extracting files form zip folder * detekt * fix tests * detekt (cherry picked from commit d6c8d60e9a9bf38087e081ba44d98e23e6c321b1) --- .../com/wire/kalium/logic/util/BackupUtils.kt | 11 +++-- .../com/wire/kalium/logic/util/BackupUtils.kt | 40 ++++++++++++----- .../logic/feature/backup/BackupConstants.kt | 13 ++++++ .../feature/backup/RestoreBackupUseCase.kt | 25 +++++------ .../feature/backup/RestoreWebBackupUseCase.kt | 2 +- .../com/wire/kalium/logic/util/BackupUtils.kt | 15 ++++++- .../feature/backup/CreateBackupUseCaseTest.kt | 5 ++- .../backup/RestoreBackupUseCaseTest.kt | 45 ++++++++++++++----- 8 files changed, 114 insertions(+), 42 deletions(-) diff --git a/logic/src/appleMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt b/logic/src/appleMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt index ec434fc07fd..d104218c7f6 100644 --- a/logic/src/appleMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt +++ b/logic/src/appleMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt @@ -22,14 +22,19 @@ import com.wire.kalium.logic.CoreFailure import com.wire.kalium.logic.data.asset.KaliumFileSystem import com.wire.kalium.logic.functional.Either import okio.BufferedSource -import okio.Source -import okio.Sink import okio.Path +import okio.Sink +import okio.Source actual fun createCompressedFile(files: List>, outputSink: Sink): Either = TODO("Implement own iOS compression method") -actual fun extractCompressedFile(inputSource: Source, outputRootPath: Path, fileSystem: KaliumFileSystem): Either = +actual fun extractCompressedFile( + inputSource: Source, + outputRootPath: Path, + param: ExtractFilesParam, + fileSystem: KaliumFileSystem +): Either = TODO("Implement own iOS compression method") actual fun checkIfCompressedFileContainsFileTypes( diff --git a/logic/src/commonJvmAndroid/kotlin/com/wire/kalium/logic/util/BackupUtils.kt b/logic/src/commonJvmAndroid/kotlin/com/wire/kalium/logic/util/BackupUtils.kt index 105c3332043..3e68c8eb988 100644 --- a/logic/src/commonJvmAndroid/kotlin/com/wire/kalium/logic/util/BackupUtils.kt +++ b/logic/src/commonJvmAndroid/kotlin/com/wire/kalium/logic/util/BackupUtils.kt @@ -68,15 +68,22 @@ private fun addToCompressedFile(zipOutputStream: ZipOutputStream, fileSource: So } @Suppress("TooGenericExceptionCaught", "NestedBlockDepth") -actual fun extractCompressedFile(inputSource: Source, outputRootPath: Path, fileSystem: KaliumFileSystem): Either = try { +actual fun extractCompressedFile( + inputSource: Source, + outputRootPath: Path, + param: ExtractFilesParam, + fileSystem: KaliumFileSystem +): Either = try { var totalExtractedFilesSize = 0L ZipInputStream(inputSource.buffer().inputStream()).use { zipInputStream -> var entry: ZipEntry? = zipInputStream.nextEntry while (entry != null) { - readCompressedEntry(zipInputStream, outputRootPath, fileSystem, entry).let { - totalExtractedFilesSize += it.first - entry = it.second + totalExtractedFilesSize += when (param) { + is ExtractFilesParam.All -> readCompressedEntry(zipInputStream, outputRootPath, fileSystem, entry) + is ExtractFilesParam.Only -> readAndExtractIfMatch(zipInputStream, outputRootPath, fileSystem, entry, param.files) } + zipInputStream.closeEntry() + entry = zipInputStream.nextEntry } } Either.Right(totalExtractedFilesSize) @@ -84,6 +91,22 @@ actual fun extractCompressedFile(inputSource: Source, outputRootPath: Path, file Either.Left(StorageFailure.Generic(RuntimeException("There was an error trying to extract the provided compressed file", e))) } +private fun readAndExtractIfMatch( + zipInputStream: ZipInputStream, + outputRootPath: Path, + fileSystem: KaliumFileSystem, + entry: ZipEntry, + fileNames: Set +): Long { + return entry.name.let { + if (fileNames.contains(it)) { + readCompressedEntry(zipInputStream, outputRootPath, fileSystem, entry) + } else { + 0L + } + } +} + @Suppress("TooGenericExceptionCaught", "NestedBlockDepth") actual fun checkIfCompressedFileContainsFileTypes( compressedFilePath: Path, @@ -121,11 +144,7 @@ private fun readCompressedEntry( outputRootPath: Path, fileSystem: KaliumFileSystem, entry: ZipEntry -): Pair { - if (isInvalidEntryPathDestination(entry.name)) { - throw RuntimeException("The provided zip file is invalid or has invalid data") - } - +): Long { var totalExtractedFilesSize = 0L var byteCount: Int val entryPathName = "$outputRootPath/${entry.name}" @@ -137,8 +156,7 @@ private fun readCompressedEntry( } output.write(zipInputStream.readBytes()) } - zipInputStream.closeEntry() - return totalExtractedFilesSize to zipInputStream.nextEntry + return totalExtractedFilesSize } /** diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/BackupConstants.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/BackupConstants.kt index 17c270cb3bf..a877104a531 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/BackupConstants.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/BackupConstants.kt @@ -21,6 +21,9 @@ package com.wire.kalium.logic.feature.backup object BackupConstants { const val BACKUP_FILE_NAME_PREFIX = "WBX" const val BACKUP_ENCRYPTED_FILE_NAME = "user-backup.cc20" + + // BACKUP_METADATA_FILE_NAME and BACKUP_USER_DB_NAME must not be changed + // if there is a need to change them, please create a new file names and add it to the list of acceptedFileNames() const val BACKUP_USER_DB_NAME = "user-backup-database.db" const val BACKUP_METADATA_FILE_NAME = "export.json" const val BACKUP_ENCRYPTED_EXTENSION = "cc20" @@ -30,6 +33,16 @@ object BackupConstants { const val BACKUP_WEB_EVENTS_FILE_NAME = "events.json" const val BACKUP_WEB_CONVERSATIONS_FILE_NAME = "conversations.json" + /** + * list of accepted file names for the backup file + * this is used when extracting data from the zip file + */ + fun acceptedFileNames() = setOf( + BACKUP_USER_DB_NAME, + BACKUP_METADATA_FILE_NAME, + BACKUP_ENCRYPTED_FILE_NAME + ) + fun createBackupFileName(userHandle: String?, timestampIso: String) = // file names cannot have special characters "$BACKUP_FILE_NAME_PREFIX-$userHandle-${timestampIso.replace(":", "-")}.zip" diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCase.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCase.kt index 08899493709..ea6b2be6196 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCase.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCase.kt @@ -26,12 +26,13 @@ import com.wire.kalium.cryptography.backup.BackupHeader.HeaderDecodingErrors.INV import com.wire.kalium.cryptography.backup.Passphrase import com.wire.kalium.cryptography.utils.ChaCha20Decryptor.decryptBackupFile import com.wire.kalium.logic.data.asset.KaliumFileSystem +import com.wire.kalium.logic.data.id.CurrentClientIdProvider import com.wire.kalium.logic.data.id.IdMapper import com.wire.kalium.logic.data.user.UserId import com.wire.kalium.logic.data.user.UserRepository import com.wire.kalium.logic.di.MapperProvider -import com.wire.kalium.logic.data.id.CurrentClientIdProvider import com.wire.kalium.logic.feature.backup.BackupConstants.BACKUP_ENCRYPTED_EXTENSION +import com.wire.kalium.logic.feature.backup.BackupConstants.acceptedFileNames import com.wire.kalium.logic.feature.backup.BackupConstants.createBackupFileName import com.wire.kalium.logic.feature.backup.RestoreBackupResult.BackupRestoreFailure.BackupIOFailure import com.wire.kalium.logic.feature.backup.RestoreBackupResult.BackupRestoreFailure.DecryptionFailure @@ -43,6 +44,7 @@ import com.wire.kalium.logic.functional.flatMap import com.wire.kalium.logic.functional.fold import com.wire.kalium.logic.functional.mapLeft import com.wire.kalium.logic.kaliumLogger +import com.wire.kalium.logic.util.ExtractFilesParam import com.wire.kalium.logic.util.extractCompressedFile import com.wire.kalium.logic.wrapStorageRequest import com.wire.kalium.network.tools.KtxSerializer @@ -192,23 +194,19 @@ internal class RestoreBackupUseCaseImpl( when (decodingError) { INVALID_USER_ID -> InvalidUserId INVALID_VERSION -> IncompatibleBackup("The provided backup version is lower than the minimum supported version") - INVALID_FORMAT -> IncompatibleBackup("The provided backup format is not supported") + INVALID_FORMAT -> IncompatibleBackup("mappedDecodingError: The provided backup format is not supported") } private suspend fun checkIsValidEncryption(extractedBackupPath: Path): Either = with(kaliumFileSystem) { - val encryptedFilePath = listDirectories(extractedBackupPath).firstOrNull { - it.name.substringAfterLast('.', "") == BACKUP_ENCRYPTED_EXTENSION - } - return if (encryptedFilePath == null) { - Either.Left(Failure(DecryptionFailure("No encrypted backup file found"))) - } else { - Either.Right(encryptedFilePath) - } + listDirectories(extractedBackupPath) + .firstOrNull { it.name.endsWith(".$BACKUP_ENCRYPTED_EXTENSION") }?.let { + Either.Right(it) + } ?: Either.Left(Failure(DecryptionFailure("No encrypted backup file found"))) } private fun extractFiles(inputSource: Source, extractedBackupRootPath: Path) = - extractCompressedFile(inputSource, extractedBackupRootPath, kaliumFileSystem) + extractCompressedFile(inputSource, extractedBackupRootPath, ExtractFilesParam.Only(acceptedFileNames()), kaliumFileSystem) private suspend fun getDbPathAndImport( extractedBackupRootPath: Path, @@ -230,14 +228,15 @@ internal class RestoreBackupUseCaseImpl( private suspend fun backupMetadata(extractedBackupPath: Path): Either = kaliumFileSystem.listDirectories(extractedBackupPath) .firstOrNull { it.name == BackupConstants.BACKUP_METADATA_FILE_NAME } - ?.let { metadataFile -> + .let { it ?: return Either.Left(Failure(IncompatibleBackup("backupMetadata: No metadata file found"))) } + .let { metadataFile -> try { kaliumFileSystem.source(metadataFile).buffer() .use { Either.Right(KtxSerializer.json.decodeFromString(it.readUtf8())) } } catch (e: SerializationException) { Either.Left(Failure(IncompatibleBackup(e.toString()))) } - } ?: Either.Left(Failure(IncompatibleBackup("The provided backup format is not supported"))) + } private fun isValidBackupAuthor(metadata: BackupMetadata): Either = if (metadata.userId == userId.toString() || metadata.userId == userId.value) { diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreWebBackupUseCase.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreWebBackupUseCase.kt index 147dce806e2..762833e8ad7 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreWebBackupUseCase.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/backup/RestoreWebBackupUseCase.kt @@ -78,7 +78,7 @@ internal class RestoreWebBackupUseCaseImpl( if (metadata.version == "19") { importWebBackup(backupRootPath, this) } else { - Either.Left(IncompatibleBackup("The provided backup format is not supported")) + Either.Left(IncompatibleBackup("invoke: The provided backup format is not supported")) }.fold({ RestoreBackupResult.Failure(it) }, { RestoreBackupResult.Success }) } diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt index ddbbed89228..8d792a0199b 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/util/BackupUtils.kt @@ -27,11 +27,24 @@ import okio.Sink import okio.Source expect fun createCompressedFile(files: List>, outputSink: Sink): Either -expect fun extractCompressedFile(inputSource: Source, outputRootPath: Path, fileSystem: KaliumFileSystem): Either +expect fun extractCompressedFile( + inputSource: Source, + outputRootPath: Path, + param: ExtractFilesParam, + fileSystem: KaliumFileSystem +): Either + expect fun checkIfCompressedFileContainsFileTypes( compressedFilePath: Path, fileSystem: KaliumFileSystem, expectedFileExtensions: List ): Either> +sealed interface ExtractFilesParam { + data object All : ExtractFilesParam + data class Only(val files: Set) : ExtractFilesParam { + constructor(vararg files: String) : this(files.toSet()) + } +} + expect inline fun decodeBufferSequence(bufferedSource: BufferedSource): Sequence diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/CreateBackupUseCaseTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/CreateBackupUseCaseTest.kt index 1ceadd35c28..42f1ea0337a 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/CreateBackupUseCaseTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/CreateBackupUseCaseTest.kt @@ -30,6 +30,7 @@ import com.wire.kalium.logic.feature.backup.BackupConstants.BACKUP_METADATA_FILE import com.wire.kalium.logic.framework.TestUser.SELF import com.wire.kalium.logic.functional.Either import com.wire.kalium.logic.test_util.TestKaliumDispatcher +import com.wire.kalium.logic.util.ExtractFilesParam import com.wire.kalium.logic.util.IgnoreIOS import com.wire.kalium.logic.util.SecurityHelper import com.wire.kalium.logic.util.extractCompressedFile @@ -107,7 +108,7 @@ class CreateBackupUseCaseTest { with(fakeFileSystem) { val extractedFilesPath = tempFilePath() createDirectory(extractedFilesPath) - extractCompressedFile(source(result.backupFilePath), extractedFilesPath, fakeFileSystem) + extractCompressedFile(source(result.backupFilePath), extractedFilesPath, ExtractFilesParam.All, fakeFileSystem) assertTrue(listDirectories(extractedFilesPath).firstOrNull { it.name == BACKUP_METADATA_FILE_NAME } != null) val extractedDB = listDirectories(extractedFilesPath).firstOrNull { @@ -174,7 +175,7 @@ class CreateBackupUseCaseTest { with(fakeFileSystem) { val extractedFilesPath = tempFilePath() createDirectory(extractedFilesPath) - extractCompressedFile(source(result.backupFilePath), extractedFilesPath, fakeFileSystem) + extractCompressedFile(source(result.backupFilePath), extractedFilesPath, ExtractFilesParam.All, fakeFileSystem) val extractedDBPath = listDirectories(extractedFilesPath).firstOrNull { it.name.contains(".cc20") } assertEquals(BACKUP_ENCRYPTED_FILE_NAME, extractedDBPath?.name) } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCaseTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCaseTest.kt index 356ae7e2b6f..a8b9102083a 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCaseTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/feature/backup/RestoreBackupUseCaseTest.kt @@ -21,18 +21,24 @@ package com.wire.kalium.logic.feature.backup import com.wire.kalium.cryptography.backup.BackupCoder import com.wire.kalium.cryptography.backup.Passphrase import com.wire.kalium.cryptography.utils.ChaCha20Encryptor.encryptBackupFile +import com.wire.kalium.logic.CoreFailure import com.wire.kalium.logic.clientPlatform import com.wire.kalium.logic.data.asset.FakeKaliumFileSystem +import com.wire.kalium.logic.data.asset.KaliumFileSystem import com.wire.kalium.logic.data.conversation.ClientId import com.wire.kalium.logic.data.user.SelfUser import com.wire.kalium.logic.data.user.UserId import com.wire.kalium.logic.data.user.UserRepository import com.wire.kalium.logic.di.MapperProvider import com.wire.kalium.logic.data.id.CurrentClientIdProvider +import com.wire.kalium.logic.feature.backup.BackupConstants.BACKUP_ENCRYPTED_FILE_NAME +import com.wire.kalium.logic.feature.backup.BackupConstants.BACKUP_USER_DB_NAME import com.wire.kalium.logic.framework.TestUser import com.wire.kalium.logic.functional.Either +import com.wire.kalium.logic.util.ExtractFilesParam import com.wire.kalium.logic.util.IgnoreIOS import com.wire.kalium.logic.util.createCompressedFile +import com.wire.kalium.logic.util.extractCompressedFile import com.wire.kalium.persistence.backup.DatabaseImporter import com.wire.kalium.persistence.db.UserDBSecret import com.wire.kalium.util.DateTimeUtil @@ -48,14 +54,15 @@ import kotlinx.coroutines.test.runTest import kotlinx.serialization.encodeToString import kotlinx.serialization.json.Json import okio.Path +import okio.Source import okio.buffer import okio.use import kotlin.test.Ignore import kotlin.test.Test +import kotlin.test.assertIs import kotlin.test.assertTrue @IgnoreIOS // TODO re-enable when BackupUtils is implemented on Darwin -@OptIn(ExperimentalCoroutinesApi::class) class RestoreBackupUseCaseTest { private val fakeFileSystem = FakeKaliumFileSystem() @@ -76,7 +83,7 @@ class RestoreBackupUseCaseTest { val result = useCase(backupPath, "") // then - assertTrue(result is RestoreBackupResult.Success) + assertIs(result) verify(arrangement.databaseImporter) .suspendFunction(arrangement.databaseImporter::importFromFile) .with(any(), any()) @@ -99,8 +106,8 @@ class RestoreBackupUseCaseTest { val result = useCase(backupPath, "") // then - assertTrue(result is RestoreBackupResult.Failure) - assertTrue(result.failure is RestoreBackupResult.BackupRestoreFailure.InvalidUserId) + assertIs(result) + assertIs(result.failure) verify(arrangement.databaseImporter) .suspendFunction(arrangement.databaseImporter::importFromFile) @@ -122,8 +129,8 @@ class RestoreBackupUseCaseTest { val result = useCase(backupPath, "") // then - assertTrue(result is RestoreBackupResult.Failure) - assertTrue(result.failure is RestoreBackupResult.BackupRestoreFailure.IncompatibleBackup) + assertIs(result) + assertIs(result.failure) verify(arrangement.databaseImporter) .suspendFunction(arrangement.databaseImporter::importFromFile) @@ -148,7 +155,7 @@ class RestoreBackupUseCaseTest { val result = useCase(backupPath, password) // then - assertTrue(result is RestoreBackupResult.Success) + assertIs(result) verify(arrangement.databaseImporter) .suspendFunction(arrangement.databaseImporter::importFromFile) @@ -225,8 +232,8 @@ class RestoreBackupUseCaseTest { val result = useCase(backupPath, password) // then - assertTrue(result is RestoreBackupResult.Failure) - assertTrue(result.failure is RestoreBackupResult.BackupRestoreFailure.BackupIOFailure) + assertIs(result) + assertIs(result.failure) verify(arrangement.databaseImporter) .suspendFunction(arrangement.databaseImporter::importFromFile) .with(any(), any()) @@ -247,7 +254,7 @@ class RestoreBackupUseCaseTest { @Mock val userRepository = mock(classOf()) - val fakeDBFileName = "fakeDBFile.db" + val fakeDBFileName = BACKUP_USER_DB_NAME private val selfUserId = currentTestUserId private val fakeDBData = fakeDBFileName.encodeToByteArray() private val idMapper = MapperProvider.idMapper() @@ -317,7 +324,7 @@ class RestoreBackupUseCaseTest { suspend fun withEncryptedBackup(path: Path, userId: UserId, password: String) = apply { with(fakeFileSystem) { - val encryptedBackupPath = fakeFileSystem.tempFilePath("backup.cc20") + val encryptedBackupPath = fakeFileSystem.tempFilePath(BACKUP_ENCRYPTED_FILE_NAME) createEncryptedBackup(encryptedBackupPath, userId, password) val outputSink = sink(path) createCompressedFile(listOf(source(encryptedBackupPath) to encryptedBackupPath.name), outputSink) @@ -352,6 +359,22 @@ class RestoreBackupUseCaseTest { .thenReturn(selfUser) } + lateinit var extractZipFile: ( + inputSource: Source, + outputRootPath: Path, + param: ExtractFilesParam, + fileSystem: KaliumFileSystem + ) -> Either + + fun withSuccessfulExtractZipFile() = apply { + extractZipFile = { _, _, _, _ -> Either.Right(10L) } + } + + fun withDefaultExtractZipFile() = apply { + extractZipFile = ::extractCompressedFile + } + + fun arrange() = this to RestoreBackupUseCaseImpl( databaseImporter = databaseImporter, kaliumFileSystem = fakeFileSystem,