diff --git a/app/src/main/kotlin/com/syncflow/data/providers/sftp/SftpProvider.kt b/app/src/main/kotlin/com/syncflow/data/providers/sftp/SftpProvider.kt index a02df52..853569e 100644 --- a/app/src/main/kotlin/com/syncflow/data/providers/sftp/SftpProvider.kt +++ b/app/src/main/kotlin/com/syncflow/data/providers/sftp/SftpProvider.kt @@ -43,17 +43,21 @@ class SftpProvider(private val account: CloudAccount, private val credentialStor override suspend fun listFiles(remotePath: String): Result> = runCatching { withSftp { sftp -> - sftp.ls(remotePath).map { entry -> - RemoteFile( - path = "$remotePath/${entry.name}".replace("//", "/"), - name = entry.name, - isDirectory = entry.isDirectory, - sizeBytes = entry.attributes.size, - modifiedAt = Instant.ofEpochSecond(entry.attributes.mtime.toLong()), - etag = null, - mimeType = null, - ) - } + sftp.ls(remotePath) + // Drop "."/".." and any name with a path separator so a hostile server can't + // smuggle a traversal segment into a local/remote path. + .filter { it.name != "." && it.name != ".." && !it.name.contains('/') && !it.name.contains('\\') } + .map { entry -> + RemoteFile( + path = "$remotePath/${entry.name}".replace("//", "/"), + name = entry.name, + isDirectory = entry.isDirectory, + sizeBytes = entry.attributes.size, + modifiedAt = Instant.ofEpochSecond(entry.attributes.mtime.toLong()), + etag = null, + mimeType = null, + ) + } } } diff --git a/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt b/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt index 22358aa..9f32ba3 100644 --- a/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt +++ b/app/src/main/kotlin/com/syncflow/domain/sync/SyncEngine.kt @@ -111,6 +111,14 @@ class SyncEngine @Inject constructor( allPaths.map { rel -> async { semaphore.withPermit { + // Defense-in-depth against a malicious/compromised remote returning a + // path that escapes the sync root (e.g. "../../evil"). Skip rather than + // write outside pair.localPath / pair.remotePath. + if (isUnsafeSyncPath(rel)) { + Timber.w("SyncEngine: skipping unsafe path for pair ${pair.id}: $rel") + logEvent(pair.id, SyncEventType.FILE_SKIPPED, rel, "unsafe path", 0) + return@withPermit FileOutcome(skipped = 1) + } val local = localFiles[rel] val remote = remoteFiles[rel] val known = knownStates[rel] @@ -363,6 +371,19 @@ internal fun syncDecide( enum class SyncDecision { UPLOAD, DOWNLOAD, DELETE_LOCAL, DELETE_REMOTE, CONFLICT, SKIP } +/** + * True if a relative sync path is unsafe to act on — empty, absolute, or containing a ".." + * segment that would let a hostile remote escape the sync root via path traversal. Applied to + * every path before any file operation as defense-in-depth (WebDAV already filters names at the + * parser; SFTP and any future provider are covered here). + */ +internal fun isUnsafeSyncPath(rel: String): Boolean { + if (rel.isBlank()) return true + val normalized = rel.replace('\\', '/') + if (normalized.startsWith("/")) return true + return normalized.split('/').any { it == ".." } +} + data class SyncResult( val uploaded: Int = 0, val downloaded: Int = 0, diff --git a/app/src/test/kotlin/com/syncflow/domain/sync/PathSafetyTest.kt b/app/src/test/kotlin/com/syncflow/domain/sync/PathSafetyTest.kt new file mode 100644 index 0000000..3e8be49 --- /dev/null +++ b/app/src/test/kotlin/com/syncflow/domain/sync/PathSafetyTest.kt @@ -0,0 +1,36 @@ +package com.syncflow.domain.sync + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Path-traversal guard: a hostile/compromised remote must not be able to make the engine read + * or write outside the sync root via "..", absolute, or separator-smuggled paths. + */ +class PathSafetyTest { + + @Test fun `normal relative paths are allowed`() { + assertFalse(isUnsafeSyncPath("photo.jpg")) + assertFalse(isUnsafeSyncPath("sub/dir/photo.jpg")) + assertFalse(isUnsafeSyncPath("a.b..c/file.txt")) // ".." only inside a name, not a segment + } + + @Test fun `parent-dir traversal is rejected`() { + assertTrue(isUnsafeSyncPath("..")) + assertTrue(isUnsafeSyncPath("../evil")) + assertTrue(isUnsafeSyncPath("a/../../etc/passwd")) + assertTrue(isUnsafeSyncPath("sub/../../escape")) + } + + @Test fun `backslash traversal is rejected`() { + assertTrue(isUnsafeSyncPath("..\\evil")) + assertTrue(isUnsafeSyncPath("a\\..\\..\\escape")) + } + + @Test fun `absolute and empty paths are rejected`() { + assertTrue(isUnsafeSyncPath("/etc/passwd")) + assertTrue(isUnsafeSyncPath("")) + assertTrue(isUnsafeSyncPath(" ")) + } +}