diff --git a/pkl-core/src/main/java/org/pkl/core/SecurityManager.java b/pkl-core/src/main/java/org/pkl/core/SecurityManager.java index 23dc2422..24b9d077 100644 --- a/pkl-core/src/main/java/org/pkl/core/SecurityManager.java +++ b/pkl-core/src/main/java/org/pkl/core/SecurityManager.java @@ -44,6 +44,24 @@ public interface SecurityManager { */ void checkResolveResource(URI resource) throws SecurityManagerException; + /** + * Resolves the given {@code file:} URI to a secure, symlink-free path that has been verified to + * be within the root directory (if one is configured). The returned path can be opened with + * {@link java.nio.file.LinkOption#NOFOLLOW_LINKS}. + * + *

Returns {@code null} for non-{@code file:} URIs or if no root directory is configured. + * + * @param uri the URI to resolve + * @param isResource denotes if uri belongs to a resource (otherwise, a module) + * @return the resolved, symlink-free path under root directory, or {@code null} + * @throws SecurityManagerException if the resolved path is not within the root directory + * @throws IOException if the path cannot be resolved + */ + default @Nullable Path resolveSecurePath(URI uri, boolean isResource) + throws SecurityManagerException, IOException { + return null; + } + /** * Resolves the given {@code file:} URI to a secure, symlink-free path that has been verified to * be within the root directory (if one is configured). The returned path can be opened with @@ -57,6 +75,6 @@ public interface SecurityManager { * @throws IOException if the path cannot be resolved */ default @Nullable Path resolveSecurePath(URI uri) throws SecurityManagerException, IOException { - return null; + return resolveSecurePath(uri, false); } } diff --git a/pkl-core/src/main/java/org/pkl/core/SecurityManagers.java b/pkl-core/src/main/java/org/pkl/core/SecurityManagers.java index ab631627..9ba16614 100644 --- a/pkl-core/src/main/java/org/pkl/core/SecurityManagers.java +++ b/pkl-core/src/main/java/org/pkl/core/SecurityManagers.java @@ -173,14 +173,20 @@ public final class SecurityManagers { } @Override - public @Nullable Path resolveSecurePath(URI uri) throws SecurityManagerException, IOException { - if (rootDir == null || !uri.isAbsolute() || !uri.getScheme().equals("file")) { + public @Nullable Path resolveSecurePath(URI uri, boolean isResource) + throws SecurityManagerException, IOException { + if (rootDir == null + || !uri.isAbsolute() + || !uri.getScheme().equals("file") + || (uri.getAuthority() != null && !uri.getAuthority().isEmpty())) { return null; } var path = Path.of(uri); var realPath = path.toRealPath(); if (!realPath.startsWith(rootDir)) { - throw new SecurityManagerException(ErrorMessages.create("modulePastRootDir", uri, rootDir)); + var errorMessageKey = isResource ? "resourcePastRootDir" : "modulePastRootDir"; + var message = ErrorMessages.create(errorMessageKey, uri, rootDir); + throw new SecurityManagerException(message); } return realPath; } @@ -225,6 +231,16 @@ public final class SecurityManagers { if (rootDir == null || !checkUri.getScheme().equals("file")) return; var path = Path.of(checkUri); + + // uri represents a UNC path if authority is non-null + // so treat this like a potentially redirected HTTP read: + // check if both the given and real paths are under rootDir + if (checkUri.getAuthority() != null && !checkUri.getAuthority().isEmpty()) { + doCheckIsUnderRootDir(path.normalize(), uri, isResource); + } + // if given path is under rootDir, do I/O to determine if the real path is under the root dir + // this can result in a nasty timeout (~20s) in Files.exists if the server doesn't respond :( + if (Files.exists(path)) { try { path = path.toRealPath(); @@ -237,6 +253,12 @@ public final class SecurityManagers { path = path.normalize(); } + doCheckIsUnderRootDir(path, uri, isResource); + } + + private void doCheckIsUnderRootDir(Path path, URI uri, boolean isResource) + throws SecurityManagerException { + assert rootDir != null; if (!path.startsWith(rootDir)) { var errorMessageKey = isResource ? "resourcePastRootDir" : "modulePastRootDir"; var message = ErrorMessages.create(errorMessageKey, uri, rootDir); diff --git a/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java b/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java index cd80085c..42897540 100644 --- a/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java +++ b/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java @@ -262,7 +262,7 @@ public final class ResourceReaders { // Use resolveSecurePath to get a symlink-free path verified under rootDir. var securityManager = VmContext.get(null).getSecurityManager(); try { - var securePath = securityManager.resolveSecurePath(uri); + var securePath = securityManager.resolveSecurePath(uri, true); if (securePath != null) { try (var in = Files.newInputStream(securePath, LinkOption.NOFOLLOW_LINKS)) { var content = in.readAllBytes(); diff --git a/pkl-core/src/test/kotlin/org/pkl/core/EvaluatorTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/EvaluatorTest.kt index 4e755ff1..45fa40e7 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/EvaluatorTest.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/EvaluatorTest.kt @@ -22,6 +22,7 @@ import java.nio.file.FileSystems import java.nio.file.Files import java.nio.file.Path import java.util.* +import java.util.concurrent.TimeUnit import java.util.regex.Pattern import kotlin.io.path.createParentDirectories import kotlin.io.path.writeText @@ -29,7 +30,10 @@ import org.assertj.core.api.Assertions.assertThat import org.assertj.core.api.Assertions.assertThatCode import org.junit.jupiter.api.AfterAll import org.junit.jupiter.api.Test +import org.junit.jupiter.api.Timeout import org.junit.jupiter.api.assertThrows +import org.junit.jupiter.api.condition.EnabledOnOs +import org.junit.jupiter.api.condition.OS import org.junit.jupiter.api.io.TempDir import org.pkl.commons.createTempFile import org.pkl.commons.test.PackageServer @@ -540,6 +544,28 @@ class EvaluatorTest { } } + @Test + @EnabledOnOs(OS.WINDOWS) + @Timeout(1, unit = TimeUnit.SECONDS) + fun `root dir check happens without any UNC or SMB access`() { + val evaluator = + with(EvaluatorBuilder.preconfigured()) { + rootDir = Path.of("/tmp/test") + build() + } + // this uses a TEST-NET-1 IP which has no server running in order to force a timeout-driven + // failure (takes ~20s) + // root dir check failure should prevent any I/O and fail fast instead of hitting the timeout + val exc = + assertThrows { + evaluator.evaluate(text("result = import(\"file://192.0.2.1/share/nope.pkl\")")) + } + assertThat(exc) + .hasMessageContaining( + "Refusing to load module `file://192.0.2.1/share/nope.pkl` because it is not within the root directory (`--root-dir`)." + ) + } + private fun checkModule(module: PModule) { assertThat(module.properties.size).isEqualTo(2) assertThat(module.getProperty("name")).isEqualTo("pigeon")