diff options
author | HÃ¥kon Hallingstad <hakon.hallingstad@gmail.com> | 2022-03-28 16:30:45 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-03-28 16:30:45 +0200 |
commit | 879d5a562f8103c0821110c65e999ebda67b5896 (patch) | |
tree | 595d9f73495adb45004156e7902caceb9d4b25f9 /node-admin/src | |
parent | 5c483c01acbe43c0ea80d5e6c545aec8a231bc4e (diff) | |
parent | f6e6a5e8ee9209bbe2185b4c4e661a59cb2160cf (diff) |
Merge pull request #21841 from vespa-engine/freva/secure-directory-stream
Use SecureDirectoryStream in container FS
Diffstat (limited to 'node-admin/src')
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); |