diff --git a/src/main/java/net/ripe/rpki/rsyncit/rsync/RsyncWriter.java b/src/main/java/net/ripe/rpki/rsyncit/rsync/RsyncWriter.java index c38aeaf..35dc73d 100644 --- a/src/main/java/net/ripe/rpki/rsyncit/rsync/RsyncWriter.java +++ b/src/main/java/net/ripe/rpki/rsyncit/rsync/RsyncWriter.java @@ -6,6 +6,7 @@ import net.ripe.rpki.rsyncit.rrdp.RpkiObject; import org.apache.tomcat.util.http.fileupload.FileUtils; +import java.io.File; import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; @@ -16,6 +17,7 @@ import java.time.Instant; import java.time.ZoneId; import java.time.format.DateTimeFormatter; +import java.util.Collection; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -68,39 +70,45 @@ private Path writeObjectToNewDirectory(List objects, Instant now) th final Path temporaryDirectory = Files.createTempDirectory(Paths.get(config.rsyncPath()), "tmp-" + formattedNow + "-"); try { groupedByHost.forEach((hostName, os) -> { - // creeate a directory per hostname (in realistic cases there will be just one) - var hostDir = temporaryDirectory.resolve(hostName); + // create a directory per hostname (in realistic cases there will be just one) + var hostBasedPath = temporaryDirectory.resolve(hostName); try { - Files.createDirectories(hostDir); + Files.createDirectories(hostBasedPath); } catch (IOException e) { - log.error("Could not create {}", hostDir); + log.error("Could not create {}", hostBasedPath); } - // create directories in "shortest first" order - os.stream() - .map(o -> + // Filter out objects with potentially insecure URLs + var wellBehavingObjects = filterOutBadUrls(hostBasedPath, os); + + // Create directories in "shortest first" order. + // Use canonical path to avoid potential troubles with relative ".." paths + wellBehavingObjects + .stream() + .map(o -> { // remove the filename, i.e. /foo/bar/object.cer -> /foo/bar - Paths.get(relativePath(o.url().getPath())).getParent() - ) + var objectParentDir = Paths.get(relativePath(o.url().getPath())).getParent(); + return hostBasedPath.resolve(objectParentDir).normalize(); + }) .sorted() .distinct() .forEach(dir -> { try { - Files.createDirectories(temporaryDirectory.resolve(hostName).resolve(dir)); + Files.createDirectories(dir); } catch (IOException ex) { log.error("Could not create directory {}", dir, ex); } }); - fileWriterPool.submit(() -> os.stream() + fileWriterPool.submit(() -> wellBehavingObjects.stream() .parallel() .forEach(o -> { var path = Paths.get(relativePath(o.url().getPath())); - var fullPath = temporaryDirectory.resolve(hostName).resolve(path); try { - Files.write(fullPath, o.bytes()); + var normalizedPath = hostBasedPath.resolve(path).normalize(); + Files.write(normalizedPath, o.bytes()); // rsync relies on the correct timestamp for fast synchronization - Files.setLastModifiedTime(fullPath, FileTime.from(o.modificationTime())); + Files.setLastModifiedTime(normalizedPath, FileTime.from(o.modificationTime())); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -123,6 +131,23 @@ private Path writeObjectToNewDirectory(List objects, Instant now) th } } + static List filterOutBadUrls(Path hostBasedPath, Collection objects) { + final String normalizedHostPath = hostBasedPath.normalize().toString(); + return objects.stream().flatMap(object -> { + var objectRelativePath = Paths.get(relativePath(object.url().getPath())); + // Check that the resulting path of the object stays within `hostBasedPath` + // to prevent URLs like rsync://bla.net/path/../../../../../PATH_INJECTION.txt + // writing data outside of the controlled path. + final String normalizedPath = hostBasedPath.resolve(objectRelativePath).normalize().toString(); + if (normalizedPath.startsWith(normalizedHostPath)) { + return Stream.of(object); + } else { + log.error("The object with url {} was skipped.", object.url()); + } + return Stream.empty(); + }).collect(Collectors.toList()); + } + private void atomicallyReplacePublishedSymlink(Path baseDirectory, Path targetDirectory) throws IOException { Path targetSymlink = baseDirectory.resolve("published"); @@ -171,7 +196,7 @@ private FileTime getLastModifiedTime(Path path) { } } - private String relativePath(final String path) { + private static String relativePath(final String path) { if (path.startsWith("/")) { return path.substring(1); } diff --git a/src/test/java/net/ripe/rpki/rsyncit/rsync/RsyncWriterTest.java b/src/test/java/net/ripe/rpki/rsyncit/rsync/RsyncWriterTest.java index 79ce266..0728f05 100644 --- a/src/test/java/net/ripe/rpki/rsyncit/rsync/RsyncWriterTest.java +++ b/src/test/java/net/ripe/rpki/rsyncit/rsync/RsyncWriterTest.java @@ -69,6 +69,20 @@ public void testWriteMultipleObjects(@TempDir Path tmpPath) throws Exception { }); } + @Test + public void testIgnoreBadUrls(@TempDir Path tmpPath) throws Exception { + withRsyncWriter(tmpPath, rsyncWriter -> { + var o1 = new RpkiObject(URI.create("rsync://bla.net/path1/a.cer"), someBytes(), Instant.now()); + var o2 = new RpkiObject(URI.create("rsync://bla.net/path1/../../PATH_INJECTION.txt"), someBytes(), Instant.now()); + var o3 = new RpkiObject(URI.create("rsync://bla.net/path1/path2/../NOT_REALLY_PATH_INJECTION.txt"), someBytes(), Instant.now()); + rsyncWriter.writeObjects(Arrays.asList(o1, o2, o3)); + final String root = rsyncWriter.getConfig().rsyncPath(); + checkFile(Paths.get(root, "published", "bla.net", "path1", "a.cer"), o1.bytes()); + assertThat(Paths.get(root, "published", "PATH_INJECTION.txt").toFile().exists()).isFalse(); + checkFile(Paths.get(root, "published", "bla.net", "path1", "NOT_REALLY_PATH_INJECTION.txt"), o3.bytes()); + }); + } + @Test public void testRemoveOldDirectoriesWhenTheyAreOld(@TempDir Path tmpPath) throws Exception { final Function writeSomeObjects = rsyncWriter ->