diff --git a/gradle/changelog/prevent_path_traversal.yaml b/gradle/changelog/prevent_path_traversal.yaml new file mode 100644 index 0000000000..f86745e972 --- /dev/null +++ b/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)) diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/ModifyWorkerHelper.java b/scm-core/src/main/java/sonia/scm/repository/spi/ModifyWorkerHelper.java index 6b1315a739..bd19a4f3c9 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/ModifyWorkerHelper.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/ModifyWorkerHelper.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.spi; import org.apache.commons.lang.StringUtils; @@ -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; + } } diff --git a/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java b/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java index a06572c885..9c28e6d55e 100644 --- a/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java +++ b/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. * diff --git a/scm-core/src/test/java/sonia/scm/repository/spi/ModifyWorkerHelperTest.java b/scm-core/src/test/java/sonia/scm/repository/spi/ModifyWorkerHelperTest.java index 1d675e0743..1d04c48a25 100644 --- a/scm-core/src/test/java/sonia/scm/repository/spi/ModifyWorkerHelperTest.java +++ b/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; + } } } diff --git a/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java b/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java index e430ae334b..5792231bd8 100644 --- a/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java +++ b/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", diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModifyCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModifyCommand.java index 355c84e4b5..014dc0c06e 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModifyCommand.java +++ b/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; diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java index 6c67f1e82b..2310be7c68 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java +++ b/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); + } } diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgModifyCommand.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgModifyCommand.java index 5a34f341f8..f966703e88 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgModifyCommand.java +++ b/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); diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgModifyCommandTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgModifyCommandTest.java index 51756c7cce..e67a976d41 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgModifyCommandTest.java +++ b/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); + } } diff --git a/scm-ui/ui-components/src/validation.test.ts b/scm-ui/ui-components/src/validation.test.ts index 5d5537aaf4..a229319572 100644 --- a/scm-ui/ui-components/src/validation.test.ts +++ b/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); + }); + } +}); diff --git a/scm-ui/ui-components/src/validation.ts b/scm-ui/ui-components/src/validation.ts index 2ea710c03a..ee364528a7 100644 --- a/scm-ui/ui-components/src/validation.ts +++ b/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("./"); +}; diff --git a/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java b/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java index 692dad457e..86399fa4af 100644 --- a/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java +++ b/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 { private final GenericDAO dao;