Skip to content

Fix entitlement checks for relative links #124133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 10, 2025
Merged

Fix entitlement checks for relative links #124133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eacba7c
Fix entitlement checks for relative links
mosche Mar 5, 2025
ee2b3ef
Merge branch 'main' into entitlements/fix-relative-links
mosche Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions ...itled-plugin/src/main/java/org/elasticsearch/entitlement/qa/entitled/EntitledActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,19 @@ public static Path createTempSymbolicLink() throws IOException {
return Files.createSymbolicLink(readDir().resolve("entitlements-link-" + random.nextLong()), readWriteDir());
}

public static Path createK8sLikeMount() throws IOException {
Path baseDir = readDir().resolve("k8s");
var versionedDir = Files.createDirectories(baseDir.resolve("..version"));
var actualFileMount = Files.createFile(versionedDir.resolve("mount-" + random.nextLong() + ".tmp"));

var dataDir = Files.createSymbolicLink(baseDir.resolve("..data"), versionedDir.getFileName());
// mount-0.tmp -> ..data/mount-0.tmp -> ..version/mount-0.tmp
return Files.createSymbolicLink(
baseDir.resolve(actualFileMount.getFileName()),
dataDir.getFileName().resolve(actualFileMount.getFileName())
);
}

public static URLConnection createHttpURLConnection() throws IOException {
return URI.create("http://127.0.0.1:12345/").toURL().openConnection();
}
Expand Down
21 changes: 21 additions & 0 deletions ...ment-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/NioFilesActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,16 @@ static void checkFilesCreateSymbolicLink() throws IOException {
}
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkFilesCreateRelativeSymbolicLink() throws IOException {
var directory = EntitledActions.createTempDirectoryForWrite();
try {
Files.createSymbolicLink(directory.resolve("link"), Path.of("target"));
} catch (UnsupportedOperationException | FileSystemException e) {
// OK not to implement symbolic link in the filesystem
}
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkFilesCreateLink() throws IOException {
var directory = EntitledActions.createTempDirectoryForWrite();
Expand All @@ -150,6 +160,17 @@ static void checkFilesCreateLink() throws IOException {
}
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkFilesCreateRelativeLink() throws IOException {
var directory = EntitledActions.createTempDirectoryForWrite();
var target = directory.resolve("target");
try {
Files.createLink(directory.resolve("link"), Path.of("target"));
} catch (UnsupportedOperationException | FileSystemException e) {
// OK not to implement symbolic link in the filesystem
}
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkFilesDelete() throws IOException {
var file = EntitledActions.createTempFileForWrite();
Expand Down
7 changes: 7 additions & 0 deletions ...itlement-test-plugin/src/main/java/org/elasticsearch/entitlement/qa/test/PathActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

package org.elasticsearch.entitlement.qa.test;

import org.elasticsearch.entitlement.qa.entitled.EntitledActions;

import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.LinkOption;
Expand All @@ -24,6 +26,11 @@ static void checkToRealPath() throws IOException {
FileCheckActions.readFile().toRealPath();
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkToRealPathWithK8sLikeMount() throws IOException, Exception {
EntitledActions.createK8sLikeMount().toRealPath();
}

@EntitlementTest(expectedAccess = PLUGINS)
static void checkToRealPathNoFollow() throws IOException {
FileCheckActions.readFile().toRealPath(LinkOption.NOFOLLOW_LINKS);
Expand Down
1 change: 1 addition & 0 deletions ...ement/qa/src/javaRestTest/java/org/elasticsearch/entitlement/qa/EntitlementsTestRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class EntitlementsTestRule implements TestRule {
"files",
List.of(
Map.of("path", tempDir.resolve("read_dir"), "mode", "read_write"),
Map.of("path", tempDir.resolve("read_dir").resolve("k8s").resolve("..data"), "mode", "read", "exclusive", true),
Copy link
Contributor Author

@mosche mosche Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..data is marked as exclusive here, it shouldn't ever be visible when testing K8s like file mounts (mount-0.tmp-> ..data/mount-0.tmp-> ..version/mount-0.tmp)

Map.of("path", tempDir.resolve("read_write_dir"), "mode", "read_write"),
Map.of("path", tempDir.resolve("read_file"), "mode", "read"),
Map.of("path", tempDir.resolve("read_write_file"), "mode", "read_write")
Expand Down
24 changes: 10 additions & 14 deletions .../main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.io.FileDescriptor;
import java.io.FileFilter;
import java.io.FilenameFilter;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintStream;
Expand Down Expand Up @@ -74,7 +73,6 @@
import java.nio.file.FileStore;
import java.nio.file.FileVisitOption;
import java.nio.file.FileVisitor;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.OpenOption;
import java.nio.file.Path;
Expand Down Expand Up @@ -2050,16 +2048,21 @@ public void checkSelectorProviderInheritedChannel(Class<?> callerClass, Selector
policyManager.checkCreateTempFile(callerClass);
}

private static Path resolveLinkTarget(Path path, Path target) {
var parent = path.getParent();
return parent == null ? target : parent.resolve(target);
Copy link
Member

@rjernst rjernst Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If target is absolute shouldn't we always return it? ie, should we only resolve relative to the parent when target is relative?

Copy link
Contributor Author

@mosche mosche Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what parent.resolve(target) does. So didn't seem necessary to handle this case specially, but agreed it could make it easier to understand. Can do next week unless someone can pick this up

Copy link
Contributor

@ldematte ldematte Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, it should not be strictly needed. I think this could be a follow-up if we feel we need it.

}

@Override
public void check$java_nio_file_Files$$createSymbolicLink(Class<?> callerClass, Path link, Path target, FileAttribute<?>... attrs) {
policyManager.checkFileRead(callerClass, target);
policyManager.checkFileWrite(callerClass, link);
policyManager.checkFileRead(callerClass, resolveLinkTarget(link, target));
}

@Override
public void check$java_nio_file_Files$$createLink(Class<?> callerClass, Path link, Path existing) {
policyManager.checkFileRead(callerClass, existing);
policyManager.checkFileWrite(callerClass, link);
policyManager.checkFileRead(callerClass, resolveLinkTarget(link, existing));
}

@Override
Expand Down Expand Up @@ -2548,13 +2551,13 @@ public void checkCreateDirectory(Class<?> callerClass, FileSystemProvider that,
@Override
public void checkCreateSymbolicLink(Class<?> callerClass, FileSystemProvider that, Path link, Path target, FileAttribute<?>... attrs) {
policyManager.checkFileWrite(callerClass, link);
policyManager.checkFileRead(callerClass, target);
policyManager.checkFileRead(callerClass, resolveLinkTarget(link, target));
}

@Override
public void checkCreateLink(Class<?> callerClass, FileSystemProvider that, Path link, Path existing) {
policyManager.checkFileWrite(callerClass, link);
policyManager.checkFileRead(callerClass, existing);
policyManager.checkFileRead(callerClass, resolveLinkTarget(link, existing));
}

@Override
Expand Down Expand Up @@ -2748,14 +2751,7 @@ public void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... o
followLinks = false;
}
}
if (followLinks) {
try {
policyManager.checkFileRead(callerClass, Files.readSymbolicLink(that));
} catch (IOException | UnsupportedOperationException e) {
// that is not a link, or unrelated IOException or unsupported
}
}
policyManager.checkFileRead(callerClass, that);
policyManager.checkFileRead(callerClass, that, followLinks);
Copy link
Contributor Author

@mosche mosche Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the check into the PolicyManager, this allows to use toRealPath (without escaping the recursion here). See the PR description for why toRealPath instead of readSymbolicLink...

}

@Override
Expand Down
23 changes: 21 additions & 2 deletions ...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.logging.Logger;

import java.io.File;
import java.io.IOException;
import java.lang.StackWalker.StackFrame;
import java.lang.module.ModuleFinder;
import java.lang.module.ModuleReference;
Expand Down Expand Up @@ -325,6 +326,10 @@ private static boolean isPathOnDefaultFilesystem(Path path) {
}

public void checkFileRead(Class<?> callerClass, Path path) {
checkFileRead(callerClass, path, false);
}

public void checkFileRead(Class<?> callerClass, Path path, boolean followLinks) {
if (isPathOnDefaultFilesystem(path) == false) {
return;
}
Expand All @@ -334,14 +339,28 @@ public void checkFileRead(Class<?> callerClass, Path path) {
}

ModuleEntitlements entitlements = getEntitlements(requestingClass);
if (entitlements.fileAccess().canRead(path) == false) {

Path realPath = null;
boolean canRead = entitlements.fileAccess().canRead(path);
if (canRead && followLinks) {
try {
realPath = path.toRealPath();
Copy link
Contributor Author

@mosche mosche Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See PR description why using toRealPath

} catch (IOException e) {
// target not found or other IO error
Copy link
Member

@rjernst rjernst Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we set canRead = false in this case?

Copy link
Contributor Author

@mosche mosche Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the ideal case we would throw a FileNotFoundException, as done by toRealPath in this case...
Not sure there's a good solution in this case, don't have a strong opinion...

In the worst case we leaking the information that a file does not exist in a location the caller shouldn't be able to access.

The opposite way the location might be OK, but the link broken... throwing a runtime exception the caller doesn't expect / handle in this case seems problematic as well 😔

Copy link
Contributor

@ldematte ldematte Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the ongoing discussion about tweaking exceptions we throw from instrumentation, I would lean towards throwing FileNotFoundException, but that would require changing the signature of our functions in the chain, right?
I don't think it should be too difficult, but it is additional work.
My 2c: I am ok with anything you decide is best for now, (leaving this as-is or setting canRead = false -- which would mean throwing NotEntitledException in the end), but I'd consider a follow-up that let us propagate the IOException / throw a FileNotFoundException.
Does it make sense?

Copy link
Contributor Author

@mosche mosche Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this as is and will follow up with a separate PR to throw a NoSuchFileException, it might spark additional discussions ... In the case of toRealPath it doesn't make a difference, the same exception would be thrown if calling toRealPath for real once the permission check passed.
That's also why keeping canRead = true is fine, the same would be thrown if attempting to read the content of the link.

}
if (realPath != null && realPath.equals(path) == false) {
canRead = entitlements.fileAccess().canRead(realPath);
}
}

if (canRead == false) {
notEntitled(
Strings.format(
"Not entitled: component [%s], module [%s], class [%s], entitlement [file], operation [read], path [%s]",
entitlements.componentName(),
requestingClass.getModule().getName(),
requestingClass,
path
realPath == null ? path : Strings.format("%s -> %s", path, realPath)
),
callerClass
);
Expand Down