Skip to content

Commit 6d70679

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4801 from kubernetes-client/copilot/cherry-pick-4798-release-26
fix(util): prevent non-tar copy path traversal cherry-pick on release-26
2 parents daad504 + e4af1b3 commit 6d70679

2 files changed

Lines changed: 161 additions & 8 deletions

File tree

‎util/src/main/java/io/kubernetes/client/Copy.java‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.io.OutputStream;
3131
import java.nio.file.Path;
3232
import java.nio.file.Paths;
33+
import java.util.Objects;
3334
import java.util.concurrent.ExecutionException;
3435
import java.util.concurrent.Future;
3536
import java.util.concurrent.TimeUnit;
@@ -241,8 +242,7 @@ private void createFiles(
241242
for (TreeNode childNode : node.getChildren()) {
242243
if (!childNode.isDir) {
243244
// Create file which is under 'node'
244-
String filePath = genericPathBuilder(destination.toString(), childNode.name);
245-
File f = new File(filePath);
245+
File f = safeResolveWithinBase(destination, childNode.name).toFile();
246246
if (!f.createNewFile()) {
247247
throw new IOException("Failed to create file: " + f);
248248
}
@@ -254,8 +254,7 @@ private void createFiles(
254254
}
255255
} else {
256256
String newSrcPath = genericPathBuilder(srcPath, childNode.name);
257-
// TODO: Change this once the method genericPathBuilder is changed to varargs
258-
Path newDestination = Paths.get(destination.toString(), childNode.name);
257+
Path newDestination = safeResolveWithinBase(destination, childNode.name);
259258
createFiles(childNode, namespace, pod, container, newSrcPath, newDestination);
260259
}
261260
}
@@ -276,8 +275,7 @@ private void createDirectory(TreeNode node, Path destination) throws IOException
276275
}
277276
for (TreeNode childNode : node.getChildren()) {
278277
if (childNode.isDir) {
279-
String path = genericPathBuilder(destination.toString(), childNode.name);
280-
Path newPath = Paths.get(path);
278+
Path newPath = safeResolveWithinBase(destination, childNode.name);
281279
createDirectory(childNode, newPath);
282280
}
283281
}
@@ -304,22 +302,54 @@ private void createDirectoryTree(
304302
int len = line.length();
305303
// line = stripFileSeparators(line);
306304
if (line.charAt(line.length() - 1) == '/') {
305+
String safeName = sanitizeRelativeEntryName(line.substring(0, len - 1));
307306
TreeNode subDirTree =
308307
new TreeNode(
309308
true,
310-
line.substring(0, len - 1),
309+
safeName,
311310
false); // Stripping off '/' in the end of directory
312311
String path = genericPathBuilder(srcPath, subDirTree.name);
313312
createDirectoryTree(subDirTree, namespace, pod, container, path);
314313
node.getChildren().add(subDirTree);
315314
} else {
316-
node.getChildren().add(new TreeNode(false, line, false));
315+
node.getChildren().add(new TreeNode(false, sanitizeRelativeEntryName(line), false));
317316
}
318317
}
319318
}
320319
}
321320
}
322321

322+
private Path safeResolveWithinBase(Path base, String entryName) throws IOException {
323+
Path normalizedBase = base.toAbsolutePath().normalize();
324+
Path resolved = normalizedBase.resolve(entryName).normalize();
325+
if (!resolved.startsWith(normalizedBase)) {
326+
throw new IOException("Invalid path entry: " + entryName);
327+
}
328+
return resolved;
329+
}
330+
331+
private String sanitizeRelativeEntryName(String entryName) throws IOException {
332+
String sanitized = Objects.requireNonNull(entryName, "entryName").trim();
333+
if (sanitized.isEmpty()) {
334+
throw new IOException("Invalid empty path entry");
335+
}
336+
337+
// ls -F output should be a single relative path segment. Reject path separators and traversal.
338+
if (sanitized.contains("/") || sanitized.contains("\\")) {
339+
throw new IOException("Invalid path entry: " + entryName);
340+
}
341+
342+
String normalized = FilenameUtils.normalize(sanitized, true);
343+
if (normalized == null || normalized.startsWith("../") || normalized.startsWith("/")) {
344+
throw new IOException("Invalid path entry: " + entryName);
345+
}
346+
347+
if (normalized.endsWith("/")) {
348+
normalized = normalized.substring(0, normalized.length() - 1);
349+
}
350+
return normalized;
351+
}
352+
323353
/*
324354
Generic method to create path.
325355
TODO: Change this to varargs

‎util/src/test/java/io/kubernetes/client/CopyTest.java‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,25 @@
2727
import io.kubernetes.client.openapi.models.V1Pod;
2828
import io.kubernetes.client.util.ClientBuilder;
2929
import io.kubernetes.client.util.exception.CopyNotSupportedException;
30+
import java.io.ByteArrayInputStream;
3031
import java.io.IOException;
3132
import java.io.InputStream;
33+
import java.io.OutputStream;
3234
import java.nio.file.Files;
3335
import java.nio.file.Path;
3436
import java.nio.file.Paths;
37+
import java.util.HashMap;
38+
import java.util.Map;
3539
import org.junit.jupiter.api.BeforeEach;
3640
import org.junit.jupiter.api.Test;
3741
import org.junit.jupiter.api.extension.RegisterExtension;
3842
import org.junit.jupiter.api.io.TempDir;
3943

44+
import static org.junit.jupiter.api.Assertions.assertFalse;
45+
import static org.junit.jupiter.api.Assertions.assertEquals;
46+
import static org.junit.jupiter.api.Assertions.assertThrows;
47+
import static org.junit.jupiter.api.Assertions.assertTrue;
48+
4049
/** Tests for the Copy helper class */
4150
class CopyTest {
4251
private String namespace;
@@ -45,6 +54,71 @@ class CopyTest {
4554

4655
private ApiClient client;
4756

57+
private static class MockProcess extends Process {
58+
private final InputStream inputStream;
59+
60+
MockProcess(String stdout) {
61+
this.inputStream = new ByteArrayInputStream(stdout.getBytes());
62+
}
63+
64+
@Override
65+
public OutputStream getOutputStream() {
66+
return OutputStream.nullOutputStream();
67+
}
68+
69+
@Override
70+
public InputStream getInputStream() {
71+
return inputStream;
72+
}
73+
74+
@Override
75+
public InputStream getErrorStream() {
76+
return InputStream.nullInputStream();
77+
}
78+
79+
@Override
80+
public int waitFor() {
81+
return 0;
82+
}
83+
84+
@Override
85+
public int exitValue() {
86+
return 0;
87+
}
88+
89+
@Override
90+
public void destroy() {
91+
// no-op for tests
92+
}
93+
}
94+
95+
private static class MockCopy extends Copy {
96+
private final Map<String, String> listingsByPath;
97+
98+
MockCopy(Map<String, String> listingsByPath) {
99+
this.listingsByPath = listingsByPath;
100+
}
101+
102+
@Override
103+
public Process exec(
104+
String namespace,
105+
String name,
106+
String[] command,
107+
String container,
108+
boolean stdin,
109+
boolean tty) {
110+
String cmd = command[2];
111+
String srcPath = cmd.substring("ls -F ".length());
112+
return new MockProcess(listingsByPath.getOrDefault(srcPath, ""));
113+
}
114+
115+
@Override
116+
public InputStream copyFileFromPod(String namespace, String pod, String srcPath)
117+
throws ApiException, IOException {
118+
return new ByteArrayInputStream("test-data".getBytes());
119+
}
120+
}
121+
48122
@RegisterExtension
49123
static WireMockExtension apiServer =
50124
WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
@@ -225,4 +299,53 @@ public void run() {
225299
.withQueryParam("command", equalTo("-c"))
226300
.withQueryParam("command", equalTo("tar --version")));
227301
}
302+
303+
@Test
304+
void rejectsPathTraversalInNonTarCopy(@TempDir Path tempDir) throws Exception {
305+
Path sourceRoot = Paths.get("/src");
306+
Path destinationRoot = tempDir.resolve("dest");
307+
Path escapedFile = tempDir.resolve("escape.txt");
308+
309+
Map<String, String> listingsByPath = new HashMap<>();
310+
listingsByPath.put(sourceRoot.toString(), "../escape.txt\n");
311+
312+
Copy copy = new MockCopy(listingsByPath);
313+
314+
assertThrows(
315+
IOException.class,
316+
() ->
317+
copy.copyDirectoryFromPod(
318+
namespace,
319+
podName,
320+
null,
321+
sourceRoot.toString(),
322+
destinationRoot,
323+
false));
324+
325+
assertFalse(Files.exists(escapedFile));
326+
}
327+
328+
@Test
329+
void copiesSafeEntriesInNonTarMode(@TempDir Path tempDir) throws Exception {
330+
Path sourceRoot = Paths.get("/src");
331+
Path destinationRoot = tempDir.resolve("dest");
332+
Files.createDirectories(destinationRoot);
333+
334+
Map<String, String> listingsByPath = new HashMap<>();
335+
listingsByPath.put(sourceRoot.toString(), "safe.txt\n");
336+
337+
Copy copy = new MockCopy(listingsByPath);
338+
339+
copy.copyDirectoryFromPod(
340+
namespace,
341+
podName,
342+
null,
343+
sourceRoot.toString(),
344+
destinationRoot,
345+
false);
346+
347+
Path copied = destinationRoot.resolve("safe.txt");
348+
assertTrue(Files.exists(copied));
349+
assertEquals("test-data", Files.readString(copied));
350+
}
228351
}

0 commit comments

Comments
 (0)