From bca02ec4f9b929aeea7e4def76c2b94c8d59757e Mon Sep 17 00:00:00 2001 From: Patrick Doyle <810052+prdoyle@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:17:19 -0500 Subject: [PATCH] Entitlement policies correct handling of prefixes that are not directories (#121598) (#121808) * Fix FileAccessTree for prefixes that aren't parents * Support backslashes * Whoops, nio * Move normalization responsibility to FileEntitlement * Normalize to native separators * Avoid forbidden API --- .../runtime/policy/FileAccessTree.java | 25 +++++++++++++------ .../policy/entitlements/FileEntitlement.java | 16 ++++-------- .../runtime/policy/FileAccessTreeTests.java | 21 ++++++++++++++++ 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java index d574609d13218..3333eefa4f716 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java @@ -17,8 +17,11 @@ import java.util.List; import java.util.Objects; +import static org.elasticsearch.core.PathUtils.getDefaultFileSystem; + public final class FileAccessTree { public static final FileAccessTree EMPTY = new FileAccessTree(List.of()); + private static final String FILE_SEPARATOR = getDefaultFileSystem().getSeparator(); private final String[] readPaths; private final String[] writePaths; @@ -27,11 +30,11 @@ private FileAccessTree(List fileEntitlements) { List readPaths = new ArrayList<>(); List writePaths = new ArrayList<>(); for (FileEntitlement fileEntitlement : fileEntitlements) { - var mode = fileEntitlement.mode(); - if (mode == FileEntitlement.Mode.READ_WRITE) { - writePaths.add(fileEntitlement.path()); + String path = normalizePath(Path.of(fileEntitlement.path())); + if (fileEntitlement.mode() == FileEntitlement.Mode.READ_WRITE) { + writePaths.add(path); } - readPaths.add(fileEntitlement.path()); + readPaths.add(path); } readPaths.sort(String::compareTo); @@ -46,14 +49,20 @@ public static FileAccessTree of(List fileEntitlements) { } boolean canRead(Path path) { - return checkPath(normalize(path), readPaths); + return checkPath(normalizePath(path), readPaths); } boolean canWrite(Path path) { - return checkPath(normalize(path), writePaths); + return checkPath(normalizePath(path), writePaths); } - private static String normalize(Path path) { + /** + * @return the "canonical" form of the given {@code path}, to be used for entitlement checks. + */ + static String normalizePath(Path path) { + // Note that toAbsolutePath produces paths separated by the default file separator, + // so on Windows, if the given path uses forward slashes, this consistently + // converts it to backslashes. return path.toAbsolutePath().normalize().toString(); } @@ -64,7 +73,7 @@ private static boolean checkPath(String path, String[] paths) { int ndx = Arrays.binarySearch(paths, path); if (ndx < -1) { String maybeParent = paths[-ndx - 2]; - return path.startsWith(maybeParent); + return path.startsWith(maybeParent) && path.startsWith(FILE_SEPARATOR, maybeParent.length()); } return ndx >= 0; } diff --git a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FileEntitlement.java b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FileEntitlement.java index 5a2492b1b2313..01d882e4d9e28 100644 --- a/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FileEntitlement.java +++ b/libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FileEntitlement.java @@ -12,10 +12,12 @@ import org.elasticsearch.entitlement.runtime.policy.ExternalEntitlement; import org.elasticsearch.entitlement.runtime.policy.PolicyValidationException; -import java.nio.file.Paths; - /** - * Describes a file entitlement with a path and mode. + * Describes entitlement to access files at a particular location. + * + * @param path the location of the files. For directories, implicitly includes access to + * all contained files and (recursively) subdirectories. + * @param mode the type of operation */ public record FileEntitlement(String path, Mode mode) implements Entitlement { @@ -24,14 +26,6 @@ public enum Mode { READ_WRITE } - public FileEntitlement { - path = normalizePath(path); - } - - private static String normalizePath(String path) { - return Paths.get(path).toAbsolutePath().normalize().toString(); - } - private static Mode parseMode(String mode) { if (mode.equals("read")) { return Mode.READ; diff --git a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java index 28fec6da73897..48c03cfd2f9b8 100644 --- a/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java +++ b/libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java @@ -16,6 +16,7 @@ import java.nio.file.Path; import java.util.List; +import static org.elasticsearch.core.PathUtils.getDefaultFileSystem; import static org.hamcrest.Matchers.is; public class FileAccessTreeTests extends ESTestCase { @@ -41,7 +42,9 @@ public void testRead() { var tree = FileAccessTree.of(List.of(entitlement("foo", "read"))); assertThat(tree.canRead(path("foo")), is(true)); assertThat(tree.canRead(path("foo/subdir")), is(true)); + assertThat(tree.canRead(path("food")), is(false)); assertThat(tree.canWrite(path("foo")), is(false)); + assertThat(tree.canWrite(path("food")), is(false)); assertThat(tree.canRead(path("before")), is(false)); assertThat(tree.canRead(path("later")), is(false)); @@ -51,7 +54,9 @@ public void testWrite() { var tree = FileAccessTree.of(List.of(entitlement("foo", "read_write"))); assertThat(tree.canWrite(path("foo")), is(true)); assertThat(tree.canWrite(path("foo/subdir")), is(true)); + assertThat(tree.canWrite(path("food")), is(false)); assertThat(tree.canRead(path("foo")), is(true)); + assertThat(tree.canRead(path("food")), is(false)); assertThat(tree.canWrite(path("before")), is(false)); assertThat(tree.canWrite(path("later")), is(false)); @@ -83,6 +88,22 @@ public void testNormalizePath() { assertThat(tree.canRead(path("")), is(false)); } + public void testForwardSlashes() { + String sep = getDefaultFileSystem().getSeparator(); + var tree = FileAccessTree.of(List.of(entitlement("a/b", "read"), entitlement("m" + sep + "n", "read"))); + + // Native separators work + assertThat(tree.canRead(path("a" + sep + "b")), is(true)); + assertThat(tree.canRead(path("m" + sep + "n")), is(true)); + + // Forward slashes also work + assertThat(tree.canRead(path("a/b")), is(true)); + assertThat(tree.canRead(path("m/n")), is(true)); + + // In case the native separator is a backslash, don't treat that as an escape + assertThat(tree.canRead(path("m\n")), is(false)); + } + FileEntitlement entitlement(String path, String mode) { Path p = path(path); return FileEntitlement.create(p.toString(), mode);