From 45a3e16e7e9518ea8c7ecc85d91c8d37fd61bb39 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Wed, 23 Jul 2025 12:43:18 +0100 Subject: [PATCH] add IOUtils.newFile(parent, path) (#855) * add IOUtils.newFile(parent, path) * Update IOUtils.java * Update IOUtils.java --- .../org/apache/poi/poifs/dev/POIFSDump.java | 12 +++-- .../poi/poifs/macros/VBAMacroExtractor.java | 3 +- .../java/org/apache/poi/util/IOUtils.java | 23 +++++++- .../java/org/apache/poi/util/TestIOUtils.java | 54 +++++++++++++++++++ 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/poi/src/main/java/org/apache/poi/poifs/dev/POIFSDump.java b/poi/src/main/java/org/apache/poi/poifs/dev/POIFSDump.java index d29745ea82..fdf6bb85f4 100644 --- a/poi/src/main/java/org/apache/poi/poifs/dev/POIFSDump.java +++ b/poi/src/main/java/org/apache/poi/poifs/dev/POIFSDump.java @@ -71,7 +71,7 @@ public final class POIFSDump { DirectoryEntry root = fs.getRoot(); String filenameWithoutPath = new File(filename).getName(); File dumpDir = new File(filenameWithoutPath + "_dump"); - File file = new File(dumpDir, root.getName()); + File file = IOUtils.newFile(dumpDir, root.getName()); if (!file.exists() && !file.mkdirs()) { throw new IOException("Could not create directory " + file); } @@ -104,13 +104,14 @@ public final class POIFSDump { try (DocumentInputStream is = new DocumentInputStream(node)) { bytes = IOUtils.toByteArray(is); } - try (OutputStream out = Files.newOutputStream(new File(parent, node.getName().trim()).toPath())) { + try (OutputStream out = Files.newOutputStream( + IOUtils.newFile(parent, node.getName().trim()).toPath())) { out.write(bytes); } } else if (entry instanceof DirectoryEntry){ DirectoryEntry dir = (DirectoryEntry)entry; - File file = new File(parent, entry.getName()); - if(!file.exists() && !file.mkdirs()) { + File file = IOUtils.newFile(parent, entry.getName()); + if (!file.exists() && !file.mkdirs()) { throw new IOException("Could not create directory " + file); } dump(dir, file); @@ -119,8 +120,9 @@ public final class POIFSDump { } } } + public static void dump(POIFSFileSystem fs, int startBlock, String name, File parent) throws IOException { - File file = new File(parent, name); + File file = IOUtils.newFile(parent, name); try (OutputStream out = Files.newOutputStream(file.toPath())) { POIFSStream stream = new POIFSStream(fs, startBlock); diff --git a/poi/src/main/java/org/apache/poi/poifs/macros/VBAMacroExtractor.java b/poi/src/main/java/org/apache/poi/poifs/macros/VBAMacroExtractor.java index c0db47e5fb..e5a80e9034 100644 --- a/poi/src/main/java/org/apache/poi/poifs/macros/VBAMacroExtractor.java +++ b/poi/src/main/java/org/apache/poi/poifs/macros/VBAMacroExtractor.java @@ -26,6 +26,7 @@ import java.nio.file.Files; import java.util.Map; import java.util.Map.Entry; +import org.apache.poi.util.IOUtils; import org.apache.poi.util.StringUtil; /** @@ -95,7 +96,7 @@ public class VBAMacroExtractor { System.out.println(); System.out.println(moduleCode); } else { - File out = new File(outputDir, moduleName + extension); + File out = IOUtils.newFile(outputDir, moduleName + extension); try (OutputStream fout = Files.newOutputStream(out.toPath()); OutputStreamWriter fwriter = new OutputStreamWriter(fout, StringUtil.UTF8)) { fwriter.write(moduleCode); diff --git a/poi/src/main/java/org/apache/poi/util/IOUtils.java b/poi/src/main/java/org/apache/poi/util/IOUtils.java index ff86043a54..b913a9bf03 100644 --- a/poi/src/main/java/org/apache/poi/util/IOUtils.java +++ b/poi/src/main/java/org/apache/poi/util/IOUtils.java @@ -601,7 +601,6 @@ public final class IOUtils { return Arrays.copyOfRange(src, offset, offset+realLength); } - /** * Simple utility function to check that you haven't hit EOF * when reading a byte. @@ -618,6 +617,28 @@ public final class IOUtils { return b; } + /** + * Creates a new file in the given parent directory with the given name. + * There is a check to prevent path traversal attacks. Only path traversal + * that would lead to a file outside the parent directory is regarded as an issue. + * + * @param parent The parent directory where the file should be created. + * @param name The name of the file to create. + * @return The created file. + * @throws IOException If path traversal is detected. + * @since POI 5.4.2 + */ + public static File newFile(final File parent, final String name) throws IOException { + final File file = new File(parent, name); + if (!file.toPath().toAbsolutePath().normalize().startsWith( + parent.toPath().toAbsolutePath().normalize() + )) { + throw new IOException(String.format( + Locale.ROOT, "Failing due to path traversal in `%s`", name)); + } + return file; + } + private static void throwRFE(long length, int maxLength) { throw new RecordFormatException(String.format(Locale.ROOT, "Tried to allocate an array of length %,d" + ", but the maximum length for this record type is %,d.%n" + diff --git a/poi/src/test/java/org/apache/poi/util/TestIOUtils.java b/poi/src/test/java/org/apache/poi/util/TestIOUtils.java index 7f026adc74..094e138eef 100644 --- a/poi/src/test/java/org/apache/poi/util/TestIOUtils.java +++ b/poi/src/test/java/org/apache/poi/util/TestIOUtils.java @@ -41,6 +41,7 @@ import java.nio.charset.StandardCharsets; import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream; import org.apache.poi.EmptyFileException; import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Isolated; @@ -575,6 +576,59 @@ final class TestIOUtils { assertEquals(4, ret.length); } + @Test + void testNewFile() throws IOException { + final File parent = TempFile.createTempDirectory("create-file-test"); + try { + final String path0 = "path/to/file.txt"; + final File outFile = IOUtils.newFile(parent, path0); + assertTrue(outFile.getAbsolutePath().endsWith(path0), + "unexpected path: " + outFile.getAbsolutePath()); + } finally { + assertTrue(parent.delete()); + } + } + + @Test + void testAllowedPathTraversal() throws IOException { + final File parent = TempFile.createTempDirectory("path-traversal-test"); + try { + // this path is ok because it doesn't walk out of the parent directory + final String path0 = "a/b/c/../d/e/../../f/g/./h"; + File outFile = IOUtils.newFile(parent, path0); + assertTrue(outFile.getAbsolutePath().endsWith(path0), + "unexpected path: " + outFile.getAbsolutePath()); + } finally { + assertTrue(parent.delete()); + } + } + + @Test + void testAllowedPathTraversal2() throws IOException { + final File parent = TempFile.createTempDirectory("path-traversal-test"); + try { + // this path is ok because it doesn't walk out of the parent directory + // the initial slash is ignored and the generated path is relative to the parent directory + final String path0 = "/a/b/c.txt"; + File outFile = IOUtils.newFile(parent, path0); + assertTrue(outFile.getAbsolutePath().endsWith(path0), + "unexpected path: " + outFile.getAbsolutePath()); + } finally { + assertTrue(parent.delete()); + } + } + + @Test + void testDisallowedPathTraversal() throws IOException { + final File parent = TempFile.createTempDirectory("path-traversal-test"); + try { + final String path0 = "../a/b/c.txt"; + Assertions.assertThrows(IOException.class, () -> IOUtils.newFile(parent, path0)); + } finally { + assertTrue(parent.delete()); + } + } + /** * This returns 0 for the first call to skip and then reads * as requested. This tests that the fallback to read() works.