Security: guard against path traversal from hostile remotes
Build & Release APK / build (push) Successful in 12m42s
Build & Release APK / build (push) Successful in 12m42s
WebDAV already sanitizes server-supplied names, but SFTP passed entry.name through unfiltered, and the engine had no central guard — a malicious or compromised remote could return '../../x' and (on the JavaFile backend) write outside the sync root. - SyncEngine: isUnsafeSyncPath() rejects empty, absolute, and any '..'-segment path; every file is checked before any read/write/delete (covers all providers). - SftpProvider.listFiles: drop '.'/'..' and names containing path separators. - PathSafetyTest covers traversal, backslash, absolute, and empty cases.
This commit is contained in:
@@ -43,7 +43,11 @@ class SftpProvider(private val account: CloudAccount, private val credentialStor
|
||||
|
||||
override suspend fun listFiles(remotePath: String): Result<List<RemoteFile>> = runCatching {
|
||||
withSftp { sftp ->
|
||||
sftp.ls(remotePath).map { entry ->
|
||||
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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(" "))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user