From 33e01ffe35a8c89a1e885334d3a3323aa1c391dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Mathieu?= Date: Fri, 3 Jan 2025 10:39:15 +0100 Subject: [PATCH] fix(storage-local): path traversal guard should include File.separator Today, we check that a file didn't contains '..' which is too aggressive, we should check that it didn't contains '../' or '..\' only. --- .../core/storages/StorageInterface.java | 9 +++++++ .../kestra/plugin/core/http/DownloadTest.java | 25 +++++++++++++++++++ .../io/kestra/storage/local/LocalStorage.java | 6 ----- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/io/kestra/core/storages/StorageInterface.java b/core/src/main/java/io/kestra/core/storages/StorageInterface.java index 5c5fdd7ba2b..6325830b7b6 100644 --- a/core/src/main/java/io/kestra/core/storages/StorageInterface.java +++ b/core/src/main/java/io/kestra/core/storages/StorageInterface.java @@ -100,4 +100,13 @@ default URI from(Execution execution, String input, File file) throws IOExceptio URI uri = StorageContext.forInput(execution, input, file.getName()).getContextStorageURI(); return this.put(execution.getTenantId(), execution.getNamespace(), uri, new BufferedInputStream(new FileInputStream(file))); } + + /** + * Throws an IllegalArgumentException if the URI is not absolute: a.k.a., if it contains ".." + File.separator. + */ + default void parentTraversalGuard(URI uri) { + if (uri != null && uri.toString().contains(".." + File.separator)) { + throw new IllegalArgumentException("File should be accessed with their full path and not using relative '..' path."); + } + } } diff --git a/core/src/test/java/io/kestra/plugin/core/http/DownloadTest.java b/core/src/test/java/io/kestra/plugin/core/http/DownloadTest.java index 4cddb2a4aaf..2a2917311a1 100644 --- a/core/src/test/java/io/kestra/plugin/core/http/DownloadTest.java +++ b/core/src/test/java/io/kestra/plugin/core/http/DownloadTest.java @@ -179,6 +179,25 @@ void failed() throws Exception { } } + @Test + void contentDispositionWithDoubleDot() throws Exception { + EmbeddedServer embeddedServer = applicationContext.getBean(EmbeddedServer.class); + embeddedServer.start(); + + Download task = Download.builder() + .id(DownloadTest.class.getSimpleName()) + .type(DownloadTest.class.getName()) + .uri(embeddedServer.getURI() + "/content-disposition-double-dot") + .build(); + + RunContext runContext = TestsUtils.mockRunContext(this.runContextFactory, task, ImmutableMap.of()); + + Download.Output output = task.run(runContext); + + assertThat(output.getUri().toString(), not(containsString("/secure-path/"))); + assertThat(output.getUri().toString(), endsWith("filename..jpg")); + } + @Controller() public static class SlackWebController { @Get("500") @@ -202,5 +221,11 @@ public HttpResponse contentDispositionWithPath() { return HttpResponse.ok("Hello World".getBytes()) .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"/secure-path/filename.jpg\""); } + + @Get("content-disposition-double-dot") + public HttpResponse contentDispositionWithDoubleDot() { + return HttpResponse.ok("Hello World".getBytes()) + .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"/secure-path/filename..jpg\""); + } } } diff --git a/storage-local/src/main/java/io/kestra/storage/local/LocalStorage.java b/storage-local/src/main/java/io/kestra/storage/local/LocalStorage.java index f3c1a448da3..33f22af61e9 100644 --- a/storage-local/src/main/java/io/kestra/storage/local/LocalStorage.java +++ b/storage-local/src/main/java/io/kestra/storage/local/LocalStorage.java @@ -240,10 +240,4 @@ private URI getKestraUri(String tenantId, Path path) { Path.of(basePath.toAbsolutePath().toString(), tenantId); return URI.create("kestra:///" + prefix.relativize(path).toString().replace("\\", "/")); } - - private void parentTraversalGuard(URI uri) { - if (uri.toString().contains("..")) { - throw new IllegalArgumentException("File should be accessed with their full path and not using relative '..' path."); - } - } }