From b810f12ffab9cd609a680fe39fd958e76003bc3a Mon Sep 17 00:00:00 2001 From: Frank Chen Date: Thu, 30 Jul 2026 22:51:41 +0800 Subject: [PATCH 1/2] Guard segment info paths --- .../druid/java/util/common/FileUtils.java | 18 ++++++++++ .../druid/java/util/common/FileUtilsTest.java | 33 +++++++++++++++++++ .../loading/SegmentLocalCacheManager.java | 13 +++++--- .../loading/SegmentLocalCacheManagerTest.java | 22 +++++++++++++ 4 files changed, 82 insertions(+), 4 deletions(-) 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..307c6d4fa895 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 @@ -446,6 +446,24 @@ 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 = Path.of(path); + 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..561acdeead42 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,7 @@ import java.io.IOException; import java.io.RandomAccessFile; import java.nio.file.Files; +import java.nio.file.Path; public class FileUtilsTest { @@ -171,6 +172,38 @@ 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 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..858815d79e6b 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 @@ -417,10 +417,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( @@ -458,7 +463,7 @@ public void removeInfoFile(final DataSegment segment) private void deleteSegmentInfoFile(DataSegment segment) { - final File segmentInfoCacheFile = new File(getEffectiveInfoDir(), segment.getId().toString()); + final File segmentInfoCacheFile = getSegmentInfoFile(segment); if (!segmentInfoCacheFile.delete()) { log.warn("Unable to delete cache file[%s] for segment[%s].", segmentInfoCacheFile, segment.getId()); } @@ -473,7 +478,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 +535,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..42b41e9bbe6f 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}); + Assert.assertThrows(IAE.class, () -> manager.removeInfoFile(segment)); + Assert.assertTrue(unsafeInfoFile.exists()); + } + @Test public void testGetCachedSegmentsLegacyPathsMigrated() throws Exception { From 89a35d0149b7551ac07412fb9e01039e63a03311 Mon Sep 17 00:00:00 2001 From: Frank Chen Date: Fri, 31 Jul 2026 06:47:55 +0800 Subject: [PATCH 2/2] fix: handle invalid segment info paths --- .../apache/druid/java/util/common/FileUtils.java | 9 ++++++++- .../druid/java/util/common/FileUtilsTest.java | 16 ++++++++++++++++ .../loading/SegmentLocalCacheManager.java | 12 ++++++++++-- .../loading/SegmentLocalCacheManagerTest.java | 2 +- 4 files changed, 35 insertions(+), 4 deletions(-) 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 307c6d4fa895..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; @@ -453,7 +454,13 @@ public static Path getTempDir() public static File resolveFileWithinDirectory(final File directory, final String path) { final Path normalizedDirectory = directory.toPath().toAbsolutePath().normalize(); - final Path childPath = Path.of(path); + 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); } 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 561acdeead42..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,7 @@ 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 @@ -204,6 +205,21 @@ public void testResolveFileWithinDirectoryRejectsAbsolutePath() ); } + @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 858815d79e6b..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; @@ -461,9 +462,16 @@ public void removeInfoFile(final DataSegment segment) } } - private void deleteSegmentInfoFile(DataSegment segment) + private void deleteSegmentInfoFile(final DataSegment segment) { - final File segmentInfoCacheFile = getSegmentInfoFile(segment); + 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()); } 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 42b41e9bbe6f..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 @@ -265,7 +265,7 @@ public void testSegmentInfoFileRejectsPathTraversal() throws IOException Assert.assertFalse(unsafeInfoFile.exists()); Files.write(unsafeInfoFile.toPath(), new byte[]{1}); - Assert.assertThrows(IAE.class, () -> manager.removeInfoFile(segment)); + manager.removeInfoFile(segment); Assert.assertTrue(unsafeInfoFile.exists()); }