This is an automated email from the ASF dual-hosted git repository.
chibenwa pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
The following commit(s) were added to refs/heads/master by this push:
new 2281e454a2 [FIX] Better validate script names in SieveFileRepository
(#3111)
2281e454a2 is described below
commit 2281e454a22c2beaaa8e2f2cee70bc202ab6a655
Author: Benoit TELLIER <[email protected]>
AuthorDate: Thu Aug 20 17:47:08 2026 +0700
[FIX] Better validate script names in SieveFileRepository (#3111)
---
.../sieverepository/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 f6720b8034..7d9a8f5226 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.InputStream;
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;
@@ -237,8 +239,7 @@ public class SieveFileRepository implements SieveRepository
{
@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());
}
@@ -249,8 +250,7 @@ public class SieveFileRepository implements SieveRepository
{
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);
}
@@ -330,6 +330,33 @@ public class SieveFileRepository implements
SieveRepository {
}
}
+ /**
+ * 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);
@@ -344,9 +371,7 @@ public class SieveFileRepository implements SieveRepository
{
} 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 {
@@ -382,11 +407,7 @@ public class SieveFileRepository implements
SieveRepository {
}
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 e9851015f1..3dc09585d6 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
@@ -19,12 +19,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;
@@ -110,4 +112,73 @@ class SieveFileRepositoryTest implements
SieveRepositoryContract {
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);
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
