From d1de2d1cd7c30880c195eaa9c24810462f7fa6af Mon Sep 17 00:00:00 2001 From: Benoit TELLIER Date: Thu, 20 Aug 2026 17:47:08 +0700 Subject: [PATCH 1/2] [FIX] Better validate script names in SieveFileRepository (#3111) --- .../file/SieveFileRepository.java | 45 ++++++++---- .../file/SieveFileRepositoryTest.java | 71 +++++++++++++++++++ 2 files changed, 104 insertions(+), 12 deletions(-) diff --git a/server/data/data-file/src/main/java/org/apache/james/sieverepository/file/SieveFileRepository.java b/server/data/data-file/src/main/java/org/apache/james/sieverepository/file/SieveFileRepository.java index aeac6604cb3..ec5f3b7f65f 100644 --- a/server/data/data-file/src/main/java/org/apache/james/sieverepository/file/SieveFileRepository.java +++ b/server/data/data-file/src/main/java/org/apache/james/sieverepository/file/SieveFileRepository.java @@ -30,12 +30,14 @@ import java.io.OutputStreamWriter; import java.io.Writer; import java.nio.file.Files; +import java.nio.file.Path; import java.time.Instant; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.util.Arrays; import java.util.List; import java.util.NoSuchElementException; +import java.util.Objects; import java.util.Optional; import java.util.Scanner; import java.util.function.Predicate; @@ -229,8 +231,7 @@ private Predicate isActiveValidator(File activeFile) { @Override public void putScript(Username username, ScriptName name, ScriptContent content) throws StorageException, QuotaExceededException { synchronized (lock) { - File file = new File(getUserDirectory(username), name.getValue()); - enforceRoot(file); + File file = resolveUserScript(username, name); haveSpace(username, name, content.length()); toFile(file, content.getValue()); } @@ -241,8 +242,7 @@ public void renameScript(Username username, ScriptName oldName, ScriptName newNa throws ScriptNotFoundException, DuplicateException, StorageException { synchronized (lock) { File oldFile = getScriptFile(username, oldName); - File newFile = new File(getUserDirectory(username), newName.getValue()); - enforceRoot(newFile); + File newFile = resolveUserScript(username, newName); if (newFile.exists()) { throw new DuplicateException("User: " + username.asString() + "Script: " + newName); } @@ -322,6 +322,33 @@ private void enforceRoot(File file) throws StorageException { } } + /** + * Resolves a script name within the directory of its owner. + * + * Enforcing the sieve root alone is not enough: it prevents escaping the repository but still lets a crafted + * name reach a sibling user directory, and thus lets one user write another user's scripts and '.active' marker. + * Scripts therefore need to resolve as a direct child of their own user directory. + */ + private File resolveInUserDirectory(File userDirectory, String name) throws StorageException { + if (name == null || name.trim().isEmpty()) { + throw new StorageException(new IllegalArgumentException("Script name should not be empty")); + } + if (SYSTEM_FILES.contains(name)) { + throw new StorageException(new IllegalArgumentException("Script name should not collide with system file '" + name + "'")); + } + File file = new File(userDirectory, name); + Path parent = file.toPath().normalize().getParent(); + if (!Objects.equals(parent, userDirectory.toPath().normalize())) { + throw new StorageException(new IllegalArgumentException("Script name should not allow path traversal outside of the user directory")); + } + enforceRoot(file); + return file; + } + + private File resolveUserScript(Username username, ScriptName name) throws StorageException { + return resolveInUserDirectory(getUserDirectory(username), name.getValue()); + } + protected File getUserDirectoryFile(Username username) throws StorageException { final File userFile = new File(getSieveRootDirectory(), username.asString() + '/'); enforceRoot(userFile); @@ -336,9 +363,7 @@ protected File getActiveFile(Username username) throws ScriptNotFoundException, } catch (FileNotFoundException ex) { throw new ScriptNotFoundException("There is no active script for user " + username.asString()); } - File scriptFile = new File(dir, content); - enforceRoot(scriptFile); - return scriptFile; + return resolveInUserDirectory(dir, content); } protected boolean isActiveFile(Username username, File file) throws StorageException { @@ -374,11 +399,7 @@ protected void setActiveFile(File scriptToBeActivated, Username userName, boolea } protected File getScriptFile(Username username, ScriptName name) throws ScriptNotFoundException, StorageException { - if (name.getValue().contains("/")) { - throw new StorageException(new IllegalArgumentException("Script name should not contain '/' as it can allow path traversal")); - } - File file = new File(getUserDirectory(username), name.getValue()); - enforceRoot(file); + File file = resolveUserScript(username, name); if (!file.exists()) { throw new ScriptNotFoundException("User: " + username + "Script: " + name); } diff --git a/server/data/data-file/src/test/java/org/apache/james/sieverepository/file/SieveFileRepositoryTest.java b/server/data/data-file/src/test/java/org/apache/james/sieverepository/file/SieveFileRepositoryTest.java index 1077ec8e3ef..d18a9d5037d 100644 --- a/server/data/data-file/src/test/java/org/apache/james/sieverepository/file/SieveFileRepositoryTest.java +++ b/server/data/data-file/src/test/java/org/apache/james/sieverepository/file/SieveFileRepositoryTest.java @@ -2,12 +2,14 @@ package org.apache.james.sieverepository.file; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import org.apache.commons.io.FileUtils; import org.apache.james.core.Username; @@ -93,4 +95,73 @@ void getScriptShouldNotAllowToReadScriptsOfOtherUsersWhenPrefix() throws Excepti new ScriptName("../other/script"))) .isInstanceOf(StorageException.class); } + + @Test + void putScriptShouldNotAllowToWriteScriptsOfOtherUsers() throws Exception { + sieveRepository().putScript(Username.of("victim"), new ScriptName("script"), SCRIPT_CONTENT); + + assertThatThrownBy(() -> sieveRepository().putScript(Username.of("attacker"), + new ScriptName("../victim/script"), new ScriptContent("PWND!!!"))) + .isInstanceOf(StorageException.class); + + assertThat(sieveRepository().getScript(Username.of("victim"), new ScriptName("script"))) + .hasContent(SCRIPT_CONTENT.getValue()); + } + + @Test + void putScriptShouldNotAllowToOverwriteTheActiveMarkerOfOtherUsers() throws Exception { + sieveRepository().putScript(Username.of("victim"), new ScriptName("script"), SCRIPT_CONTENT); + + assertThatThrownBy(() -> sieveRepository().putScript(Username.of("attacker"), + new ScriptName("../victim/.active"), new ScriptContent("script"))) + .isInstanceOf(StorageException.class); + + assertThat(new File(fileSystem.getFile(SIEVE_ROOT), "victim/.active")).doesNotExist(); + } + + @Test + void putScriptShouldNotAllowToOverwriteSystemFiles() { + assertThatThrownBy(() -> sieveRepository().putScript(Username.of("test"), + new ScriptName(".active"), SCRIPT_CONTENT)) + .isInstanceOf(StorageException.class); + + assertThatThrownBy(() -> sieveRepository().putScript(Username.of("test"), + new ScriptName(".quota"), SCRIPT_CONTENT)) + .isInstanceOf(StorageException.class); + } + + @Test + void putScriptShouldNotAllowToOverwriteTheGlobalQuotaFile() throws Exception { + assertThatThrownBy(() -> sieveRepository().putScript(Username.of("test"), + new ScriptName("../.quota"), new ScriptContent("1"))) + .isInstanceOf(StorageException.class); + + assertThat(new File(fileSystem.getFile(SIEVE_ROOT), ".quota")).doesNotExist(); + } + + @Test + void renameScriptShouldNotAllowToWriteScriptsOfOtherUsers() throws Exception { + Username attacker = Username.of("attacker"); + sieveRepository().putScript(Username.of("victim"), new ScriptName("script"), SCRIPT_CONTENT); + sieveRepository().putScript(attacker, new ScriptName("evil"), new ScriptContent("PWND!!!")); + sieveRepository().setActive(attacker, new ScriptName("evil")); + + assertThatThrownBy(() -> sieveRepository().renameScript(attacker, + new ScriptName("evil"), new ScriptName("../victim/evil"))) + .isInstanceOf(StorageException.class); + + assertThat(new File(fileSystem.getFile(SIEVE_ROOT), "victim/evil")).doesNotExist(); + assertThat(new File(fileSystem.getFile(SIEVE_ROOT), "victim/.active")).doesNotExist(); + } + + @Test + void getActiveShouldNotFollowACraftedActiveMarker() throws Exception { + sieveRepository().putScript(Username.of("other"), new ScriptName("script"), new ScriptContent("PWND!!!")); + sieveRepository().putScript(Username.of("test"), new ScriptName("script"), SCRIPT_CONTENT); + FileUtils.write(new File(fileSystem.getFile(SIEVE_ROOT), "test/.active"), + "../other/script", StandardCharsets.UTF_8); + + assertThatThrownBy(() -> sieveRepository().getActive(Username.of("test"))) + .isInstanceOf(StorageException.class); + } } From d171aa1ee9872462cf2e6ba84c70325176138464 Mon Sep 17 00:00:00 2001 From: Benoit TELLIER Date: Sun, 23 Aug 2026 10:36:58 +0700 Subject: [PATCH 2/2] Disable faulty git maven plugin --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 91ecaef797e..da66e306b3d 100644 --- a/pom.xml +++ b/pom.xml @@ -3645,7 +3645,7 @@ git.commit.message.short git.dirty - false + true true false