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
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);
+ }
}