diff --git a/processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java b/processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java index f852786cc8b2..c3a04d904c8e 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java @@ -42,6 +42,7 @@ import java.nio.file.AccessDeniedException; import java.nio.file.FileSystemException; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.StandardCopyOption; @@ -446,6 +447,30 @@ public static Path getTempDir() return new File(parentDirectory).toPath(); } + /** + * Resolves {@code path} below {@code directory}, rejecting absolute paths and parent traversal that would escape it. + * This is intended for paths containing externally supplied identifiers. + */ + public static File resolveFileWithinDirectory(final File directory, final String path) + { + final Path normalizedDirectory = directory.toPath().toAbsolutePath().normalize(); + final Path childPath; + try { + childPath = Path.of(path); + } + catch (InvalidPathException e) { + throw new IAE(e, "Path[%s] is not within directory[%s]", path, directory); + } + if (childPath.isAbsolute()) { + throw new IAE("Path[%s] is not within directory[%s]", path, directory); + } + final Path resolvedPath = normalizedDirectory.resolve(childPath).normalize(); + if (!resolvedPath.startsWith(normalizedDirectory)) { + throw new IAE("Path[%s] is not within directory[%s]", path, directory); + } + return resolvedPath.toFile(); + } + @SuppressForbidden(reason = "Files#createTempDirectory") public static File createTempDirInLocation(final Path parentDirectory, @Nullable final String prefix) { diff --git a/processing/src/test/java/org/apache/druid/java/util/common/FileUtilsTest.java b/processing/src/test/java/org/apache/druid/java/util/common/FileUtilsTest.java index d242d1fd211f..190c58d1f426 100644 --- a/processing/src/test/java/org/apache/druid/java/util/common/FileUtilsTest.java +++ b/processing/src/test/java/org/apache/druid/java/util/common/FileUtilsTest.java @@ -31,6 +31,8 @@ import java.io.IOException; import java.io.RandomAccessFile; import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; public class FileUtilsTest { @@ -171,6 +173,53 @@ public void testWriteAtomically() throws IOException Assertions.assertEquals("baz", StringUtils.fromUtf8(Files.readAllBytes(tmpFile.toPath()))); } + @Test + public void testResolveFileWithinDirectory() + { + final File resolved = FileUtils.resolveFileWithinDirectory(temporaryFolder, "nested/file"); + + Assertions.assertEquals( + temporaryFolder.toPath().toAbsolutePath().resolve(Path.of("nested", "file")).normalize(), + resolved.toPath() + ); + } + + @Test + public void testResolveFileWithinDirectoryRejectsTraversal() + { + Assertions.assertThrows( + IAE.class, + () -> FileUtils.resolveFileWithinDirectory(temporaryFolder, "../outside") + ); + } + + @Test + public void testResolveFileWithinDirectoryRejectsAbsolutePath() + { + Assertions.assertThrows( + IAE.class, + () -> FileUtils.resolveFileWithinDirectory( + temporaryFolder, + temporaryFolder.toPath().resolve("inside").toAbsolutePath().toString() + ) + ); + } + + @Test + public void testResolveFileWithinDirectoryRejectsInvalidPath() + { + final IAE exception = Assertions.assertThrows( + IAE.class, + () -> FileUtils.resolveFileWithinDirectory(temporaryFolder, "invalid\0path") + ); + + Assertions.assertEquals( + StringUtils.format("Path[%s] is not within directory[%s]", "invalid\0path", temporaryFolder), + exception.getMessage() + ); + Assertions.assertInstanceOf(InvalidPathException.class, exception.getCause()); + } + @Test public void testCreateTempDir() throws IOException { diff --git a/server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java b/server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java index 8ee13001f237..ed528732ef79 100644 --- a/server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java +++ b/server/src/main/java/org/apache/druid/segment/loading/SegmentLocalCacheManager.java @@ -33,6 +33,7 @@ import org.apache.druid.error.DruidException; import org.apache.druid.guice.annotations.Json; import org.apache.druid.java.util.common.FileUtils; +import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.Stopwatch; import org.apache.druid.java.util.common.concurrent.Execs; @@ -417,10 +418,15 @@ private File[] retrieveSegmentMetadataFiles() throws IOException return files == null ? new File[0] : files; } + private File getSegmentInfoFile(final DataSegment segment) + { + return FileUtils.resolveFileWithinDirectory(getEffectiveInfoDir(), segment.getId().toString()); + } + @Override public void storeInfoFile(final DataSegment segment) throws IOException { - final File segmentInfoCacheFile = new File(getEffectiveInfoDir(), segment.getId().toString()); + final File segmentInfoCacheFile = getSegmentInfoFile(segment); if (!segmentInfoCacheFile.exists()) { FileUtils.mkdirp(segmentInfoCacheFile.getParentFile()); FileUtils.writeAtomically( @@ -456,9 +462,16 @@ public void removeInfoFile(final DataSegment segment) } } - private void deleteSegmentInfoFile(DataSegment segment) + private void deleteSegmentInfoFile(final DataSegment segment) { - final File segmentInfoCacheFile = new File(getEffectiveInfoDir(), segment.getId().toString()); + final File segmentInfoCacheFile; + try { + segmentInfoCacheFile = getSegmentInfoFile(segment); + } + catch (IAE e) { + log.warn(e, "Refusing to delete info file with invalid path for segment[%s].", segment.getId()); + return; + } if (!segmentInfoCacheFile.delete()) { log.warn("Unable to delete cache file[%s] for segment[%s].", segmentInfoCacheFile, segment.getId()); } @@ -473,7 +486,7 @@ private void deleteSegmentInfoFile(DataSegment segment) */ private void writePartialInfoFile(DataSegment segment) throws IOException { - final File segmentInfoCacheFile = new File(getEffectiveInfoDir(), segment.getId().toString()); + final File segmentInfoCacheFile = getSegmentInfoFile(segment); FileUtils.mkdirp(getEffectiveInfoDir()); FileUtils.writeAtomically(segmentInfoCacheFile, out -> { jsonMapper.writeValue(out, segment); @@ -530,7 +543,7 @@ public AcquireSegmentAction acquireSegment(final DataSegment dataSegment, final try { if (hold != null) { // write the segment info file if it doesn't exist. this can happen if we are loading after a drop - final File segmentInfoCacheFile = new File(getEffectiveInfoDir(), dataSegment.getId().toString()); + final File segmentInfoCacheFile = getSegmentInfoFile(dataSegment); if (!segmentInfoCacheFile.exists()) { FileUtils.mkdirp(getEffectiveInfoDir()); FileUtils.writeAtomically(segmentInfoCacheFile, out -> { diff --git a/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerTest.java b/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerTest.java index 458da17f2194..6fbb538c2fe2 100644 --- a/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerTest.java +++ b/server/src/test/java/org/apache/druid/segment/loading/SegmentLocalCacheManagerTest.java @@ -29,6 +29,7 @@ import org.apache.druid.guice.LocalDataStorageDruidModule; import org.apache.druid.jackson.SegmentizerModule; import org.apache.druid.java.util.common.FileUtils; +import org.apache.druid.java.util.common.IAE; import org.apache.druid.java.util.common.Intervals; import org.apache.druid.java.util.emitter.EmittingLogger; import org.apache.druid.math.expr.ExprMacroTable; @@ -59,6 +60,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -247,6 +249,26 @@ public void testGetCachedSegmentsWithMissingSegmentFile() throws IOException Assert.assertFalse(segment3InfoFile.exists()); } + @Test + public void testSegmentInfoFileRejectsPathTraversal() throws IOException + { + final DataSegment segment = TestSegmentUtils.makeSegment( + "../../outside", + "v0", + Intervals.of("2014-10-20T00:00:00Z/P1D") + ); + final File infoDir = new File(localSegmentCacheDir, "info_dir"); + final File unsafeInfoFile = new File(infoDir, segment.getId().toString()); + FileUtils.mkdirp(infoDir); + + Assert.assertThrows(IAE.class, () -> manager.storeInfoFile(segment)); + Assert.assertFalse(unsafeInfoFile.exists()); + + Files.write(unsafeInfoFile.toPath(), new byte[]{1}); + manager.removeInfoFile(segment); + Assert.assertTrue(unsafeInfoFile.exists()); + } + @Test public void testGetCachedSegmentsLegacyPathsMigrated() throws Exception {