🛡️ Sentinel: [CRITICAL] Fix Path Traversal in AbstractDiskBasedService

.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())`.

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;
     }
   }
 

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