diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..7dff65d --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2024-05-18 - Prevent Path Traversal in AbstractDiskBasedService +**Vulnerability:** A Path Traversal vulnerability existed in `AbstractDiskBasedService`'s `getPathInStorageDirectory` method because it blindly resolved the `UploadId`'s value against the base storage directory. +**Learning:** Even though `UploadId` may attempt to generate safe IDs, user-provided `UploadId`s or corrupted state might cause paths like `../../../etc/passwd` to be resolved, breaking out of the designated directory. +**Prevention:** Always normalize and verify that file paths derived from user input or generic variables remain within their intended boundary directory by using `path.normalize().toAbsolutePath().startsWith(basePath.normalize().toAbsolutePath())`. diff --git a/src/main/java/me/desair/tus/server/upload/disk/AbstractDiskBasedService.java b/src/main/java/me/desair/tus/server/upload/disk/AbstractDiskBasedService.java index 0c12472..1f3c5e0 100644 --- a/src/main/java/me/desair/tus/server/upload/disk/AbstractDiskBasedService.java +++ b/src/main/java/me/desair/tus/server/upload/disk/AbstractDiskBasedService.java @@ -37,7 +37,12 @@ protected Path getPathInStorageDirectory(UploadId id) { if (id == null) { return null; } else { - return storagePath.resolve(id.toString()); + Path path = storagePath.resolve(id.toString()); + if (!path.normalize().toAbsolutePath().startsWith(storagePath.normalize().toAbsolutePath())) { + throw new IllegalArgumentException( + "Upload ID is not valid and would result in a path traversal"); + } + return path; } } diff --git a/src/test/java/me/desair/tus/server/upload/disk/AbstractDiskBasedServiceTest.java b/src/test/java/me/desair/tus/server/upload/disk/AbstractDiskBasedServiceTest.java new file mode 100644 index 0000000..dca4afe --- /dev/null +++ b/src/test/java/me/desair/tus/server/upload/disk/AbstractDiskBasedServiceTest.java @@ -0,0 +1,60 @@ +package me.desair.tus.server.upload.disk; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThrows; + +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.UUID; +import me.desair.tus.server.upload.UploadId; +import org.apache.commons.io.FileUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class AbstractDiskBasedServiceTest { + + private Path storagePath; + private AbstractDiskBasedService service; + + @Before + public void setUp() { + storagePath = Paths.get("target", "tus", "abstract-test").toAbsolutePath(); + service = new AbstractDiskBasedService(storagePath.toString()); + } + + @After + public void tearDown() throws IOException { + FileUtils.deleteDirectory(storagePath.toFile()); + } + + @Test + public void testValidUploadId() { + UploadId id = new UploadId(UUID.randomUUID().toString()); + Path path = service.getPathInStorageDirectory(id); + assertThat(path.startsWith(storagePath), is(true)); + } + + @Test + public void testPathTraversalUploadId() { + UploadId id = new UploadId("../../..%2F/etc/passwd"); + assertThrows( + IllegalArgumentException.class, + () -> { + service.getPathInStorageDirectory(id); + }); + } + + @Test + public void testPathTraversalUploadIdEncoded() { + UploadId id = new UploadId("..%2f..%2f..%2f..%2f..%2fetc%2fpasswd"); + // Since URLCodec encodes "/", it becomes `..%2f..%2f..%2f..%2f..%2fetc%2fpasswd` literally, + // which Java interprets as a single directory name. This doesn't actually traverse directories. + // We expect NO exception to be thrown because it evaluates to a safe path starting with the + // storage root. + Path path = service.getPathInStorageDirectory(id); + assertThat(path.startsWith(storagePath), is(true)); + } +}