diff --git a/util/src/main/java/io/kubernetes/client/Copy.java b/util/src/main/java/io/kubernetes/client/Copy.java index 8e56f437b7..7e1df5ed11 100644 --- a/util/src/main/java/io/kubernetes/client/Copy.java +++ b/util/src/main/java/io/kubernetes/client/Copy.java @@ -30,6 +30,7 @@ import java.io.OutputStream; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Objects; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -241,8 +242,7 @@ private void createFiles( for (TreeNode childNode : node.getChildren()) { if (!childNode.isDir) { // Create file which is under 'node' - String filePath = genericPathBuilder(destination.toString(), childNode.name); - File f = new File(filePath); + File f = safeResolveWithinBase(destination, childNode.name).toFile(); if (!f.createNewFile()) { throw new IOException("Failed to create file: " + f); } @@ -254,8 +254,7 @@ private void createFiles( } } else { String newSrcPath = genericPathBuilder(srcPath, childNode.name); - // TODO: Change this once the method genericPathBuilder is changed to varargs - Path newDestination = Paths.get(destination.toString(), childNode.name); + Path newDestination = safeResolveWithinBase(destination, childNode.name); createFiles(childNode, namespace, pod, container, newSrcPath, newDestination); } } @@ -276,8 +275,7 @@ private void createDirectory(TreeNode node, Path destination) throws IOException } for (TreeNode childNode : node.getChildren()) { if (childNode.isDir) { - String path = genericPathBuilder(destination.toString(), childNode.name); - Path newPath = Paths.get(path); + Path newPath = safeResolveWithinBase(destination, childNode.name); createDirectory(childNode, newPath); } } @@ -304,22 +302,54 @@ private void createDirectoryTree( int len = line.length(); // line = stripFileSeparators(line); if (line.charAt(line.length() - 1) == '/') { + String safeName = sanitizeRelativeEntryName(line.substring(0, len - 1)); TreeNode subDirTree = new TreeNode( true, - line.substring(0, len - 1), + safeName, false); // Stripping off '/' in the end of directory String path = genericPathBuilder(srcPath, subDirTree.name); createDirectoryTree(subDirTree, namespace, pod, container, path); node.getChildren().add(subDirTree); } else { - node.getChildren().add(new TreeNode(false, line, false)); + node.getChildren().add(new TreeNode(false, sanitizeRelativeEntryName(line), false)); } } } } } + private Path safeResolveWithinBase(Path base, String entryName) throws IOException { + Path normalizedBase = base.toAbsolutePath().normalize(); + Path resolved = normalizedBase.resolve(entryName).normalize(); + if (!resolved.startsWith(normalizedBase)) { + throw new IOException("Invalid path entry: " + entryName); + } + return resolved; + } + + private String sanitizeRelativeEntryName(String entryName) throws IOException { + String sanitized = Objects.requireNonNull(entryName, "entryName").trim(); + if (sanitized.isEmpty()) { + throw new IOException("Invalid empty path entry"); + } + + // ls -F output should be a single relative path segment. Reject path separators and traversal. + if (sanitized.contains("/") || sanitized.contains("\\")) { + throw new IOException("Invalid path entry: " + entryName); + } + + String normalized = FilenameUtils.normalize(sanitized, true); + if (normalized == null || normalized.startsWith("../") || normalized.startsWith("/")) { + throw new IOException("Invalid path entry: " + entryName); + } + + if (normalized.endsWith("/")) { + normalized = normalized.substring(0, normalized.length() - 1); + } + return normalized; + } + /* Generic method to create path. TODO: Change this to varargs diff --git a/util/src/test/java/io/kubernetes/client/CopyTest.java b/util/src/test/java/io/kubernetes/client/CopyTest.java index 82fce66f66..f5f18c84ec 100644 --- a/util/src/test/java/io/kubernetes/client/CopyTest.java +++ b/util/src/test/java/io/kubernetes/client/CopyTest.java @@ -27,16 +27,25 @@ import io.kubernetes.client.openapi.models.V1Pod; import io.kubernetes.client.util.ClientBuilder; import io.kubernetes.client.util.exception.CopyNotSupportedException; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.api.io.TempDir; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + /** Tests for the Copy helper class */ class CopyTest { private String namespace; @@ -45,6 +54,71 @@ class CopyTest { private ApiClient client; + private static class MockProcess extends Process { + private final InputStream inputStream; + + MockProcess(String stdout) { + this.inputStream = new ByteArrayInputStream(stdout.getBytes()); + } + + @Override + public OutputStream getOutputStream() { + return OutputStream.nullOutputStream(); + } + + @Override + public InputStream getInputStream() { + return inputStream; + } + + @Override + public InputStream getErrorStream() { + return InputStream.nullInputStream(); + } + + @Override + public int waitFor() { + return 0; + } + + @Override + public int exitValue() { + return 0; + } + + @Override + public void destroy() { + // no-op for tests + } + } + + private static class MockCopy extends Copy { + private final Map listingsByPath; + + MockCopy(Map listingsByPath) { + this.listingsByPath = listingsByPath; + } + + @Override + public Process exec( + String namespace, + String name, + String[] command, + String container, + boolean stdin, + boolean tty) { + String cmd = command[2]; + String srcPath = cmd.substring("ls -F ".length()); + return new MockProcess(listingsByPath.getOrDefault(srcPath, "")); + } + + @Override + public InputStream copyFileFromPod(String namespace, String pod, String srcPath) + throws ApiException, IOException { + return new ByteArrayInputStream("test-data".getBytes()); + } + } + @RegisterExtension static WireMockExtension apiServer = WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build(); @@ -225,4 +299,53 @@ public void run() { .withQueryParam("command", equalTo("-c")) .withQueryParam("command", equalTo("tar --version"))); } + + @Test + void rejectsPathTraversalInNonTarCopy(@TempDir Path tempDir) throws Exception { + Path sourceRoot = Paths.get("/src"); + Path destinationRoot = tempDir.resolve("dest"); + Path escapedFile = tempDir.resolve("escape.txt"); + + Map listingsByPath = new HashMap<>(); + listingsByPath.put(sourceRoot.toString(), "../escape.txt\n"); + + Copy copy = new MockCopy(listingsByPath); + + assertThrows( + IOException.class, + () -> + copy.copyDirectoryFromPod( + namespace, + podName, + null, + sourceRoot.toString(), + destinationRoot, + false)); + + assertFalse(Files.exists(escapedFile)); + } + + @Test + void copiesSafeEntriesInNonTarMode(@TempDir Path tempDir) throws Exception { + Path sourceRoot = Paths.get("/src"); + Path destinationRoot = tempDir.resolve("dest"); + Files.createDirectories(destinationRoot); + + Map listingsByPath = new HashMap<>(); + listingsByPath.put(sourceRoot.toString(), "safe.txt\n"); + + Copy copy = new MockCopy(listingsByPath); + + copy.copyDirectoryFromPod( + namespace, + podName, + null, + sourceRoot.toString(), + destinationRoot, + false); + + Path copied = destinationRoot.resolve("safe.txt"); + assertTrue(Files.exists(copied)); + assertEquals("test-data", Files.readString(copied)); + } }