Merge pull request #21841 from vespa-engine/freva/secure-directory-stream

Use SecureDirectoryStream in container FS

2 files changed, 98 insertions, 16 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
index 5405a5acd61..843503d1cbf 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
@@ -18,6 +18,7 @@ import java.nio.file.LinkOption;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.ProviderMismatchException;
+import java.nio.file.SecureDirectoryStream;
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileAttribute;
@@ -28,6 +29,7 @@ import java.nio.file.attribute.PosixFileAttributes;
 import java.nio.file.attribute.UserPrincipal;
 import java.nio.file.spi.FileSystemProvider;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
@@ -78,10 +80,12 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     @Override
     public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options, FileAttribute<?>... attrs) throws IOException {
         Path pathOnHost = pathOnHost(path);
-        boolean existedBefore = Files.exists(pathOnHost);
-        SeekableByteChannel seekableByteChannel = provider(pathOnHost).newByteChannel(pathOnHost, options, attrs);
-        if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(path));
-        return seekableByteChannel;
+        try (SecureDirectoryStream<Path> sds = leafDirectoryStream(pathOnHost)) {
+            boolean existedBefore = Files.exists(pathOnHost);
+            SeekableByteChannel seekableByteChannel = sds.newByteChannel(pathOnHost.getFileName(), addNoFollow(options), attrs);
+            if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(path));
+            return seekableByteChannel;
+        }
     }
 
     @Override
@@ -109,14 +113,16 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     public void copy(Path source, Path target, CopyOption... options) throws IOException {
         // Only called when both 'source' and 'target' have 'this' as the FS provider
         Path targetPathOnHost = pathOnHost(target);
-        provider(targetPathOnHost).copy(pathOnHost(source), targetPathOnHost, options);
+        provider(targetPathOnHost).copy(pathOnHost(source), targetPathOnHost, addNoFollow(options));
     }
 
     @Override
     public void move(Path source, Path target, CopyOption... options) throws IOException {
         // Only called when both 'source' and 'target' have 'this' as the FS provider
         Path targetPathOnHost = pathOnHost(target);
-        provider(targetPathOnHost).move(pathOnHost(source), targetPathOnHost, options);
+        try (SecureDirectoryStream<Path> sds = leafDirectoryStream(targetPathOnHost)) {
+            sds.move(pathOnHost(source), sds, targetPathOnHost.getFileName());
+        }
     }
 
     @Override
@@ -167,12 +173,12 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         if (!type.isAssignableFrom(PosixFileAttributeView.class)) return null;
         Path pathOnHost = pathOnHost(path);
         FileSystemProvider provider = pathOnHost.getFileSystem().provider();
-        if (type == BasicFileAttributeView.class) // Basic view doesnt have owner/group fields, forward to base FS provider
-            return provider.getFileAttributeView(pathOnHost, type, options);
+        if (type == BasicFileAttributeView.class) // Basic view doesn't have owner/group fields, forward to base FS provider
+            return provider.getFileAttributeView(pathOnHost, type, addNoFollow(options));
 
-        PosixFileAttributeView view = provider.getFileAttributeView(pathOnHost, PosixFileAttributeView.class, options);
+        PosixFileAttributeView view = provider.getFileAttributeView(pathOnHost, PosixFileAttributeView.class, addNoFollow(options));
         return (V) new ContainerPosixFileAttributeView(view,
-                uncheck(() -> new ContainerPosixFileAttributes(readAttributes(path, "unix:*", options))));
+                uncheck(() -> new ContainerPosixFileAttributes(readAttributes(path, "unix:*", addNoFollow(options)))));
     }
 
     @Override
@@ -181,10 +187,10 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         if (!type.isAssignableFrom(PosixFileAttributes.class)) throw new UnsupportedOperationException();
         Path pathOnHost = pathOnHost(path);
         if (type == BasicFileAttributes.class)
-            return pathOnHost.getFileSystem().provider().readAttributes(pathOnHost, type, options);
+            return pathOnHost.getFileSystem().provider().readAttributes(pathOnHost, type, addNoFollow(options));
 
         // Non-basic requests need to be upgraded to unix:* to get owner,group,uid,gid fields, which are then re-mapped
-        return (A) new ContainerPosixFileAttributes(readAttributes(path, "unix:*", options));
+        return (A) new ContainerPosixFileAttributes(readAttributes(path, "unix:*", addNoFollow(options)));
     }
 
     @Override
@@ -192,9 +198,9 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         Path pathOnHost = pathOnHost(path);
         int index = attributes.indexOf(':');
         if (index < 0 || attributes.startsWith("basic:"))
-            return provider(pathOnHost).readAttributes(pathOnHost, attributes, options);
+            return provider(pathOnHost).readAttributes(pathOnHost, attributes, addNoFollow(options));
 
-        Map<String, Object> attrs = new HashMap<>(provider(pathOnHost).readAttributes(pathOnHost, "unix:*", options));
+        Map<String, Object> attrs = new HashMap<>(provider(pathOnHost).readAttributes(pathOnHost, "unix:*", addNoFollow(options)));
         int uid = userPrincipalLookupService.userIdInContainer((int) attrs.get("uid"));
         int gid = userPrincipalLookupService.groupIdInContainer((int) attrs.get("gid"));
         attrs.put("uid", uid);
@@ -207,7 +213,7 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     @Override
     public void setAttribute(Path path, String attribute, Object value, LinkOption... options) throws IOException {
         Path pathOnHost = pathOnHost(path);
-        provider(pathOnHost).setAttribute(pathOnHost, attribute, fixAttributeValue(attribute, value), options);
+        provider(pathOnHost).setAttribute(pathOnHost, attribute, fixAttributeValue(attribute, value), addNoFollow(options));
     }
 
     private Object fixAttributeValue(String attribute, Object value) {
@@ -238,6 +244,17 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         setAttribute(path, "unix:gid", path.user().gid(), LinkOption.NOFOLLOW_LINKS);
     }
 
+    private SecureDirectoryStream<Path> leafDirectoryStream(Path pathOnHost) throws IOException {
+        Path containerRoot = containerFs.containerRootOnHost();
+        SecureDirectoryStream<Path> sds = ((SecureDirectoryStream<Path>) Files.newDirectoryStream(containerRoot));
+        for (int i = containerRoot.getNameCount(); i < pathOnHost.getNameCount() - 1; i++) {
+            SecureDirectoryStream<Path> next = sds.newDirectoryStream(pathOnHost.getName(i), LinkOption.NOFOLLOW_LINKS);
+            sds.close();
+            sds = next;
+        }
+        return sds;
+    }
+
     private class ContainerDirectoryStream implements DirectoryStream<Path> {
         private final DirectoryStream<Path> hostDirectoryStream;
         private final UnixUser user;
@@ -270,7 +287,6 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         }
     }
 
-
     static ContainerPath toContainerPath(Path path) {
         return cast(path, ContainerPath.class);
     }
@@ -287,4 +303,27 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     private static FileSystemProvider provider(Path path) {
         return path.getFileSystem().provider();
     }
+
+    private static Set<? extends OpenOption> addNoFollow(Set<? extends OpenOption> options) {
+        if (options.contains(LinkOption.NOFOLLOW_LINKS)) return options;
+        Set<OpenOption> copy = new HashSet<>(options);
+        copy.add(LinkOption.NOFOLLOW_LINKS);
+        return copy;
+    }
+
+    private static LinkOption[] addNoFollow(LinkOption... options) {
+        if (Set.of(options).contains(LinkOption.NOFOLLOW_LINKS)) return options;
+        LinkOption[] copy = new LinkOption[options.length + 1];
+        System.arraycopy(options, 0, copy, 0, options.length);
+        copy[options.length] = LinkOption.NOFOLLOW_LINKS;
+        return copy;
+    }
+
+    private static CopyOption[] addNoFollow(CopyOption... options) {
+        if (Set.of(options).contains(LinkOption.NOFOLLOW_LINKS)) return options;
+        CopyOption[] copy = new CopyOption[options.length + 1];
+        System.arraycopy(options, 0, copy, 0, options.length);
+        copy[options.length] = LinkOption.NOFOLLOW_LINKS;
+        return copy;
+    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
index b5f2ef41a1a..07ef0b80d91 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
@@ -19,6 +19,8 @@ import java.nio.file.StandardOpenOption;
 import java.util.Map;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
  * @author freva
@@ -140,6 +142,47 @@ class ContainerFileSystemTest {
         assertOwnership(source, 0, 0, 10000, 11000);
     }
 
+    @Test
+    public void disallow_operations_on_symlink() throws IOException {
+        Path destination = fileSystem.getPath("/dir/file");
+        Files.createDirectories(destination.getParent());
+
+        ContainerPath link = containerFs.getPath("/link");
+        Files.createSymbolicLink(link, destination);
+
+        // Cannot write file via symlink
+        assertThrows(IOException.class, () -> Files.writeString(link, "hello"));
+
+        assertOwnership(link, 0, 0, 10_000, 11_000);
+        Files.setAttribute(link, "unix:uid", 10); // This succeeds because attribute is set on the link (destination does not exist)
+        assertFalse(Files.exists(destination));
+        assertOwnership(link, 10, 0, 10_010, 11_000);
+    }
+
+    @Test
+    public void disallow_operations_on_parent_symlink() throws IOException {
+        Path destination = fileSystem.getPath("/dir/sub/folder");
+        Files.createDirectories(destination.getParent());
+
+        // Create symlink /some/dir/link -> /dir/sub
+        ContainerPath link = containerFs.getPath("/some/dir/link");
+        Files.createDirectories(link.getParent());
+        Files.createSymbolicLink(link, destination.getParent());
+
+        { // Cannot write file via symlink
+            ContainerPath file = link.resolve("file");
+            assertThrows(IOException.class, () -> Files.writeString(file, "hello"));
+            Files.writeString(file.pathOnHost(), "hello"); // Writing through host FS works
+        }
+
+        { // Cannot move via symlink
+            ContainerPath file = containerFs.getPath("/file");
+            Files.writeString(file, "world");
+            assertThrows(IOException.class, () -> Files.move(file, link.resolve("dest")));
+            Files.move(file.pathOnHost(), link.resolve("dest").pathOnHost()); // Moving through host FS works
+        }
+    }
+
     private static void assertOwnership(ContainerPath path, int contUid, int contGid, int hostUid, int hostGid) throws IOException {
         assertOwnership(path, contUid, contGid);
         assertOwnership(path.pathOnHost(), hostUid, hostGid);