[FIX] Better validate script names in SieveFileRepository (#3111)

pom.xml

@@ -3645,7 +3645,7 @@
                         <includeOnlyProperty>git.commit.message.short</includeOnlyProperty>
                         <includeOnlyProperty>git.dirty</includeOnlyProperty>
                     </includeOnlyProperties>
-                    <skip>false</skip>
+                    <skip>true</skip>
                     <useNativeGit>true</useNativeGit>
                     <failOnNoGitDirectory>false</failOnNoGitDirectory>
                     <gitDescribe>

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

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