Prevent parent path traversal in filesystem-nio2

Reported-by: Nico Waisman <[email protected]>
gaul · Feb 1, 2025 · 86b6ee4 · 86b6ee4
1 parent 83b159e
commit 86b6ee4
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 6 deletions.
diff --git a/src/main/java/org/gaul/s3proxy/nio2blob/AbstractNio2BlobStore.java b/src/main/java/org/gaul/s3proxy/nio2blob/AbstractNio2BlobStore.java
@@ -183,7 +183,10 @@ public final PageSet<? extends StorageMetadata> list(String container,
         } else {
             prefix = "";
         }
-        var pathPrefix = root.resolve(container).resolve(prefix);
+        var containerPath = root.resolve(container);
+        var pathPrefix = containerPath.resolve(prefix).normalize();
+        checkValidPath(containerPath, pathPrefix);
+        logger.debug("Listing blobs at: {}", pathPrefix);
         var set = ImmutableSortedSet.<StorageMetadata>naturalOrder();
         try {
             listHelper(set, container, dirPrefix, pathPrefix, delimiter);
@@ -344,7 +347,9 @@ public final Blob getBlob(String container, String key, GetOptions options) {
             throw new ContainerNotFoundException(container, "");
         }
 
-        var path = root.resolve(container).resolve(key);
+        var containerPath = root.resolve(container);
+        var path = containerPath.resolve(key);
+        checkValidPath(containerPath, path);
         logger.debug("Getting blob at: {}", path);
 
         try {
@@ -528,7 +533,9 @@ public final String putBlob(String container, Blob blob, PutOptions options) {
             throw new ContainerNotFoundException(container, "");
         }
 
-        var path = root.resolve(container).resolve(blob.getMetadata().getName());
+        var containerPath = root.resolve(container);
+        var path = containerPath.resolve(blob.getMetadata().getName());
+        checkValidPath(containerPath, path);
         // TODO: should we use a known suffix to filter these out during list?
         var tmpPath = root.resolve(container).resolve(blob.getMetadata().getName() + "-" + UUID.randomUUID());
         logger.debug("Creating blob at: {}", path);
@@ -693,7 +700,9 @@ public final String copyBlob(String fromContainer, String fromName,
     public final void removeBlob(String container, String key) {
         try {
             var containerPath = root.resolve(container);
-            var path = containerPath.resolve(key);
+            var path = containerPath.resolve(key).normalize();
+            checkValidPath(containerPath, path);
+            logger.debug("Deleting blob at: {}", path);
             Files.delete(path);
             removeEmptyParentDirectories(containerPath, path.getParent());
         } catch (NoSuchFileException nsfe) {
@@ -771,7 +780,10 @@ public final BlobAccess getBlobAccess(String container, String key) {
             throw new KeyNotFoundException(container, key, "");
         }
 
-        var path = root.resolve(container).resolve(key);
+        var containerPath = root.resolve(container);
+        var path = containerPath.resolve(key).normalize();
+        checkValidPath(containerPath, path);
+
         Set<PosixFilePermission> permissions;
         try {
             permissions = Files.getPosixFilePermissions(path);
@@ -791,7 +803,10 @@ public final void setBlobAccess(String container, String key, BlobAccess access)
             throw new KeyNotFoundException(container, key, "");
         }
 
-        var path = root.resolve(container).resolve(key);
+        var containerPath = root.resolve(container);
+        var path = containerPath.resolve(key).normalize();
+        checkValidPath(containerPath, path);
+
         Set<PosixFilePermission> permissions;
         try {
             permissions = new HashSet<>(Files.getPosixFilePermissions(path));
@@ -1117,4 +1132,10 @@ private static void writeCommonMetadataAttr(UserDefinedFileAttributeView view, B
             writeStringAttributeIfPresent(view, XATTR_USER_METADATA_PREFIX + entry.getKey(), entry.getValue());
         }
     }
+
+    private static void checkValidPath(Path container, Path path) {
+        if (!path.normalize().startsWith(container)) {
+            throw new IllegalArgumentException("Invalid key name: path traversal attempt detected: " + path);
+        }
+    }
 }
diff --git a/src/test/java/org/gaul/s3proxy/AwsSdkTest.java b/src/test/java/org/gaul/s3proxy/AwsSdkTest.java
@@ -1717,6 +1717,70 @@ public Map.Entry<String, BlobStore> locateBlobStore(
         }
     }
 
+    @Test
+    public void testCopyRelativePath() throws Exception {
+        assumeTrue(!blobStoreType.equals("azureblob-sdk"));
+        try {
+            client.copyObject(new CopyObjectRequest(
+                    containerName, "../evil.txt", containerName, "good.txt"));
+            Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
+        } catch (AmazonS3Exception e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void testDeleteRelativePath() throws Exception {
+        try {
+            client.deleteObject(containerName, "../evil.txt");
+            if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
+                Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
+            }
+        } catch (AmazonS3Exception e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void testGetRelativePath() throws Exception {
+        try {
+            client.getObject(containerName, "../evil.txt");
+            Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
+        } catch (AmazonS3Exception e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void testPutRelativePath() throws Exception {
+        try {
+            var metadata = new ObjectMetadata();
+            metadata.setContentLength(BYTE_SOURCE.size());
+            PutObjectResult result = client.putObject(containerName, "../evil.txt",
+                    BYTE_SOURCE.openStream(), metadata);
+            if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
+                Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
+            }
+        } catch (AmazonS3Exception e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void testListRelativePath() throws Exception {
+        assumeTrue(!blobStoreType.equals("filesystem"));
+        try {
+            client.listObjects(new ListObjectsRequest()
+                    .withBucketName(containerName)
+                    .withPrefix("../evil/"));
+            if (blobStoreType.equals("filesystem") || blobStoreType.equals("filesystem-nio2") || blobStoreType.equals("transient-nio2")) {
+                Fail.failBecauseExceptionWasNotThrown(AmazonS3Exception.class);
+            }
+        } catch (AmazonS3Exception e) {
+            // expected
+        }
+    }
+
     private static final class NullX509TrustManager
             implements X509TrustManager {
         @Override