Validate filepath and filename to prevent path traversal (#1604)

Validate filepath and filename to prevent path traversal in modification
command and provide validations for editor plugin.

Co-authored-by: René Pfeuffer <rene.pfeuffer@cloudogu.com>

gradle/changelog/prevent_path_traversal.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Adjust path and filename validation to prevent path traversal ([#1604](https://github.com/scm-manager/scm-manager/pull/1604))

scm-core/src/main/java/sonia/scm/repository/spi/ModifyWorkerHelper.java

@@ -41,6 +41,7 @@ import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 import static sonia.scm.AlreadyExistsException.alreadyExists;
 import static sonia.scm.ContextEntry.ContextBuilder.entity;
 import static sonia.scm.NotFoundException.notFound;
+import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
 
 /**
  * This "interface" is not really intended to be used as an interface but rather as
@@ -52,7 +53,7 @@ public interface ModifyWorkerHelper extends ModifyCommand.Worker {
 
   @Override
   default void delete(String toBeDeleted) throws IOException {
-    Path fileToBeDeleted = new File(getWorkDir(), toBeDeleted).toPath();
+    Path fileToBeDeleted = getTargetFile(toBeDeleted);
     try {
       Files.delete(fileToBeDeleted);
     } catch (NoSuchFileException e) {
@@ -65,7 +66,7 @@ public interface ModifyWorkerHelper extends ModifyCommand.Worker {
 
   @Override
   default void create(String toBeCreated, File file, boolean overwrite) throws IOException {
-    Path targetFile = new File(getWorkDir(), toBeCreated).toPath();
+    Path targetFile = getTargetFile(toBeCreated);
     createDirectories(targetFile);
     if (overwrite) {
       Files.move(file.toPath(), targetFile, REPLACE_EXISTING);
@@ -80,7 +81,7 @@ public interface ModifyWorkerHelper extends ModifyCommand.Worker {
   }
 
   default void modify(String path, File file) throws IOException {
-    Path targetFile = new File(getWorkDir(), path).toPath();
+    Path targetFile = getTargetFile(path);
     createDirectories(targetFile);
     if (!targetFile.toFile().exists()) {
       throw notFound(createFileContext(path));
@@ -112,9 +113,32 @@ public interface ModifyWorkerHelper extends ModifyCommand.Worker {
     }
   }
 
+  default Path getTargetFile(String path) {
+    File workDir = getWorkDir();
+    // WARNING: 'new File(workDir, path)' is not the same as
+    // 'workDir.toPath().resolve(path)'! The first one does not
+    // mind whether 'path' starts with a '/', the nio api does.
+    // So using the file api the two paths '/file' and 'file'
+    // lead to the same result, whereas the nio api would
+    // lead to an absolute path starting at the ssytem root in the
+    // first example starting with a '/'.
+    Path targetFile = new File(workDir, path).toPath().normalize();
+    doThrow()
+      .violation("illegal path traversal: " + path)
+      .when(!targetFile.startsWith(workDir.toPath().normalize()));
+    doThrow()
+      .violation("protected path: " + path)
+      .when(isProtectedPath(targetFile));
+    return targetFile;
+  }
+
   File getWorkDir();
 
   Repository getRepository();
 
   String getBranch();
+
+  default boolean isProtectedPath(Path path) {
+    return false;
+  }
 }

scm-core/src/main/java/sonia/scm/util/ValidationUtil.java

@@ -51,6 +51,21 @@ public final class ValidationUtil {
     return Util.isNotEmpty(filename) && isNotContaining(filename, "/", "\\", ":");
   }
 
+  /**
+   *  Checks if the path is a valid path and does not enable path traversal
+   *
+   *  @param path path to be validated
+   *
+   *  @return {@code true} if path is valid else false
+   */
+  public static boolean isPathValid(String path)
+  {
+    return !path.equals(".")
+      && !path.contains("../")
+      && !path.contains("//")
+      && !path.equals("..");
+  }
+
   /**
    * Returns {@code true} if the mail is valid.
    *

scm-core/src/test/java/sonia/scm/repository/spi/ModifyWorkerHelperTest.java

@@ -26,6 +26,7 @@ package sonia.scm.repository.spi;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.repository.Repository;
 
 import java.io.File;
@@ -34,9 +35,12 @@ import java.io.IOException;
 import java.nio.file.Path;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 class ModifyWorkerHelperTest {
 
+  private boolean pathProtected = false;
+
   @Test
   void shouldKeepExecutableFlag(@TempDir Path temp) throws IOException {
 
@@ -52,6 +56,33 @@ class ModifyWorkerHelperTest {
     assertThat(target.canExecute()).isTrue();
   }
 
+  @Test
+  void shouldNotWriteInProtectedPath(@TempDir Path temp) throws IOException {
+
+    pathProtected = true;
+    File target = createFile(temp, "some.txt");
+
+    ModifyWorkerHelper helper = new MinimalModifyWorkerHelper(temp);
+
+    assertThrows(
+      ScmConstraintViolationException.class,
+      () -> helper.create("some.txt", target, true)
+    );
+  }
+
+  @Test
+  void shouldNotWritePathWithPathTraversal(@TempDir Path temp) throws IOException {
+
+    File target = createFile(temp, "some.txt");
+
+    ModifyWorkerHelper helper = new MinimalModifyWorkerHelper(temp);
+
+    assertThrows(
+      ScmConstraintViolationException.class,
+      () -> helper.create("../some.txt", target, true)
+    );
+  }
+
   private File createFile(Path temp, String fileName) throws IOException {
     File file = new File(temp.toFile(), fileName);
     FileWriter source = new FileWriter(file);
@@ -60,7 +91,7 @@ class ModifyWorkerHelperTest {
     return file;
   }
 
-  private static class MinimalModifyWorkerHelper implements ModifyWorkerHelper {
+  private class MinimalModifyWorkerHelper implements ModifyWorkerHelper {
 
     private final Path temp;
 
@@ -92,5 +123,10 @@ class ModifyWorkerHelperTest {
     public String getBranch() {
       return null;
     }
+
+    @Override
+    public boolean isProtectedPath(Path path) {
+      return pathProtected;
+    }
   }
 }

scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java

@@ -55,6 +55,28 @@ class ValidationUtilTest {
     assertFalse(ValidationUtil.isFilenameValid(value));
   }
 
+  @ParameterizedTest
+  @ValueSource(strings = {
+    "test",
+    "test 123"
+  })
+  void shouldAcceptPath(String value) {
+    // true
+    assertTrue(ValidationUtil.isPathValid(value));
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {
+    "..",
+    "../",
+    "../../",
+    "../ka",
+    "test/../.."
+  })
+  void shouldRejectPath(String value) {
+    assertFalse(ValidationUtil.isPathValid(value));
+  }
+
   @ParameterizedTest
   @ValueSource(strings = {
     "s.sdorra@ostfalia.de",

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModifyCommand.java

@@ -176,6 +176,11 @@ public class GitModifyCommand extends AbstractGitCommand implements ModifyComman
       }
     }
 
+    @Override
+    public boolean isProtectedPath(Path path) {
+      return path.startsWith(getClone().getRepository().getDirectory().toPath().normalize());
+    }
+
     @Override
     public File getWorkDir() {
       return workDir;

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java

@@ -34,6 +34,7 @@ import sonia.scm.AlreadyExistsException;
 import sonia.scm.BadRequestException;
 import sonia.scm.ConcurrentModificationException;
 import sonia.scm.NotFoundException;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.repository.GitTestHelper;
 import sonia.scm.repository.Person;
 import sonia.scm.repository.RepositoryHookType;
@@ -355,4 +356,18 @@ public class GitModifyCommandTest extends GitModifyCommandTestBase {
         .fireHookEvent(argThat(argument -> argument.getType() == RepositoryHookType.POST_RECEIVE))
     );
   }
+
+  @Test(expected = ScmConstraintViolationException.class)
+  public void shouldFailIfPathInGitMetadata() throws IOException {
+    File newFile = Files.write(temporaryFolder.newFile().toPath(), "other".getBytes()).toFile();
+
+    GitModifyCommand command = createCommand();
+
+    ModifyCommandRequest request = new ModifyCommandRequest();
+    request.setCommitMessage("test commit");
+    request.addRequest(new ModifyCommandRequest.CreateFileRequest(".git/ome.txt", newFile, true));
+    request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
+
+    command.execute(request);
+  }
 }

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgModifyCommand.java

@@ -93,6 +93,11 @@ public class HgModifyCommand extends AbstractWorkingCopyCommand implements Modif
               private void addFileToHg(File file) {
                 workingRepository.workingCopy().add(file.getAbsolutePath());
               }
+
+              @Override
+              public boolean isProtectedPath(Path path) {
+                return path.startsWith(workingRepository.getDirectory().toPath().normalize().resolve(".hg"));
+              }
             });
           } catch (IOException e) {
             throwInternalRepositoryException("could not execute command on repository", e);

scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgModifyCommandTest.java

@@ -32,6 +32,7 @@ import org.junit.rules.TemporaryFolder;
 import sonia.scm.AlreadyExistsException;
 import sonia.scm.NoChangesMadeException;
 import sonia.scm.NotFoundException;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.repository.Person;
 import sonia.scm.repository.work.NoneCachingWorkingCopyPool;
 import sonia.scm.repository.work.WorkdirProvider;
@@ -196,4 +197,17 @@ public class HgModifyCommandTest extends AbstractHgCommandTestBase {
     matcher.matches();
     assertThat(matcher.group(1)).isEqualTo("This is an error message");
   }
+
+  @Test(expected = ScmConstraintViolationException.class)
+  public void shouldFailIfPathInHgMetadata() throws IOException {
+    File testFile = temporaryFolder.newFile("a.txt");
+
+    new FileOutputStream(testFile).write(42);
+    ModifyCommandRequest request2 = new ModifyCommandRequest();
+    request2.addRequest(new ModifyCommandRequest.CreateFileRequest(".hg/some.txt", testFile, true));
+    request2.setCommitMessage("Now i really found the answer");
+    request2.setAuthor(new Person("Trillian Astra", "trillian@hitchhiker.com"));
+
+    hgModifyCommand.execute(request2);
+  }
 }

scm-ui/ui-components/src/validation.test.ts

@@ -136,7 +136,7 @@ describe("test number validation", () => {
 });
 
 describe("test path validation", () => {
-  const invalid = ["//", "some//path", "end//"];
+  const invalid = ["//", "some//path", "end//", ".", "..", "../"];
   for (const path of invalid) {
     it(`should return false for '${path}'`, () => {
       expect(validator.isPathValid(path)).toBe(false);
@@ -233,3 +233,18 @@ describe("test url validation", () => {
     });
   }
 });
+
+describe("test filename validation", () => {
+  const invalid = ["", "/", "some/file", ".", "..", "../", "\\", "\\name", "file:some"];
+  for (const filename of invalid) {
+    it(`should return false for '${filename}'`, () => {
+      expect(validator.isFilenameValid(filename)).toBe(false);
+    });
+  }
+  const valid = ["a", "test", "some_file", "end.txt", ".gitignore"];
+  for (const filename of valid) {
+    it(`should return true for '${filename}'`, () => {
+      expect(validator.isFilenameValid(filename)).toBe(true);
+    });
+  }
+});

scm-ui/ui-components/src/validation.ts

@@ -44,10 +44,11 @@ export const isNumberValid = (number: any) => {
   return !isNaN(number);
 };
 
-const pathRegex = /^((?!\/{2,}).)*$/;
-
 export const isPathValid = (path: string) => {
-  return pathRegex.test(path);
+  return path !== "."
+    && !path.includes("../")
+    && !path.includes("//")
+    && path !== "..";
 };
 
 const urlRegex = /^[A-Za-z0-9]+:\/\/[^\s$.?#].[^\s]*$/;
@@ -55,3 +56,9 @@ const urlRegex = /^[A-Za-z0-9]+:\/\/[^\s$.?#].[^\s]*$/;
 export const isUrlValid = (url: string) => {
   return urlRegex.test(url);
 };
+
+const filenameRegex = /^[^/\\:]+$/;
+
+export const isFilenameValid = (filename: string) => {
+  return filenameRegex.test(filename) && filename !== "." && filename !== ".." && !filename.includes("./");
+};

scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java

@@ -31,8 +31,6 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
-import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
-
 public class ManagerDaoAdapter<T extends ModelObject> {
 
   private final GenericDAO<T> dao;