This is an automated email from the ASF dual-hosted git repository. bodewig pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/ant-ivy.git
commit 03b6b8c3ae27406fadb3b3539b51294af246aafa Author: Stefan Bodewig <[email protected]> AuthorDate: Sun Aug 7 21:01:12 2022 +0200 CVE-2022-37865 ZipPacking allows overwriting arbitrary files --- src/java/org/apache/ivy/core/pack/ZipPacking.java | 11 +++- src/java/org/apache/ivy/util/FileUtil.java | 62 ++++++++++++++++++ test/java/org/apache/ivy/ant/FileUtilTest.java | 72 +++++++++++++++++++++ .../org/apache/ivy/core/pack/ZipPackingTest.java | 72 +++++++++++++++++++++ test/zip/test.zip | Bin 0 -> 554 bytes 5 files changed, 215 insertions(+), 2 deletions(-) diff --git a/src/java/org/apache/ivy/core/pack/ZipPacking.java b/src/java/org/apache/ivy/core/pack/ZipPacking.java index 18a010ec..197463c4 100644 --- a/src/java/org/apache/ivy/core/pack/ZipPacking.java +++ b/src/java/org/apache/ivy/core/pack/ZipPacking.java @@ -52,8 +52,15 @@ public class ZipPacking extends ArchivePacking { try (ZipInputStream zip = new ZipInputStream(packed)) { ZipEntry entry = null; while (((entry = zip.getNextEntry()) != null)) { - File f = new File(dest, entry.getName()); - Message.verbose("\t\texpanding " + entry.getName() + " to " + f); + String entryName = entry.getName(); + File f = FileUtil.resolveFile(dest, entryName); + if (!FileUtil.isLeadingPath(dest, f, true)) { + Message.verbose("\t\tskipping " + entryName + " as its target " + + f.getCanonicalPath() + + " is outside of " + dest.getCanonicalPath() + "."); + continue; + } + Message.verbose("\t\texpanding " + entryName + " to " + f); // create intermediary directories - sometimes zip don't add them File dirF = f.getParentFile(); diff --git a/src/java/org/apache/ivy/util/FileUtil.java b/src/java/org/apache/ivy/util/FileUtil.java index e5e31d5a..4d387412 100644 --- a/src/java/org/apache/ivy/util/FileUtil.java +++ b/src/java/org/apache/ivy/util/FileUtil.java @@ -610,6 +610,68 @@ public final class FileUtil { return new DissectedPath(File.separator, pathToDissect); } + /** + * Learn whether one path "leads" another. + * + * <p>This method uses {@link #normalize} under the covers and + * does not resolve symbolic links.</p> + * + * <p>If either path tries to go beyond the file system root + * (i.e. it contains more ".." segments than can be travelled up) + * the method will return false.</p> + * + * @param leading The leading path, must not be null, must be absolute. + * @param path The path to check, must not be null, must be absolute. + * @return true if path starts with leading; false otherwise. + * @since Ant 1.7 + */ + public static boolean isLeadingPath(File leading, File path) { + String l = normalize(leading.getAbsolutePath()).getAbsolutePath(); + String p = normalize(path.getAbsolutePath()).getAbsolutePath(); + if (l.equals(p)) { + return true; + } + // ensure that l ends with a / + // so we never think /foo was a parent directory of /foobar + if (!l.endsWith(File.separator)) { + l += File.separator; + } + // ensure "/foo/" is not considered a parent of "/foo/../../bar" + String up = File.separator + ".." + File.separator; + if (l.contains(up) || p.contains(up) || (p + File.separator).contains(up)) { + return false; + } + return p.startsWith(l); + } + + /** + * Learn whether one path "leads" another. + * + * @param leading The leading path, must not be null, must be absolute. + * @param path The path to check, must not be null, must be absolute. + * @param resolveSymlinks whether symbolic links shall be resolved + * prior to comparing the paths. + * @return true if path starts with leading; false otherwise. + * @since Ant 1.9.13 + * @throws IOException if resolveSymlinks is true and invoking + * getCanonicaPath on either argument throws an exception + */ + public static boolean isLeadingPath(File leading, File path, boolean resolveSymlinks) + throws IOException { + if (!resolveSymlinks) { + return isLeadingPath(leading, path); + } + final File l = leading.getCanonicalFile(); + File p = path.getCanonicalFile(); + do { + if (l.equals(p)) { + return true; + } + p = p.getParentFile(); + } while (p != null); + return false; + } + /** * Get the length of the file, or the sum of the children lengths if it is a directory * diff --git a/test/java/org/apache/ivy/ant/FileUtilTest.java b/test/java/org/apache/ivy/ant/FileUtilTest.java index ca96483c..dd1131c1 100644 --- a/test/java/org/apache/ivy/ant/FileUtilTest.java +++ b/test/java/org/apache/ivy/ant/FileUtilTest.java @@ -45,6 +45,7 @@ import static org.junit.Assert.assertTrue; public class FileUtilTest { private static boolean symlinkCapable = false; + private static final String PATH_SEP = System.getProperty("path.separator"); @BeforeClass public static void beforeClass() { @@ -151,4 +152,75 @@ public class FileUtilTest { Assert.assertTrue("Unexpected content in dest file " + destFile, Arrays.equals(fileContent, Files.readAllBytes(destFile))); } + /** + * @see "https://bz.apache.org/bugzilla/show_bug.cgi?id=62502"; + */ + @Test + public void isLeadingPathCannotBeFooledByTooManyDoubleDots() { + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/../../bar"))); + Assert.assertFalse(FileUtil.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\..\\..\\bar"))); + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/../.."))); + } + + /** + * @see "https://bz.apache.org/bugzilla/show_bug.cgi?id=62502"; + */ + @Test + public void isLeadingPathCanonicalVersionCannotBeFooledByTooManyDoubleDots() throws IOException { + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/../../bar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\..\\..\\bar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/../.."), true)); + } + + @Test + public void isLeadingPathCanonicalVersionWorksAsExpectedOnUnix() throws IOException { + Assume.assumeFalse("Test doesn't run on DOS", PATH_SEP.equals(";")); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/baz/../bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/../foo/bar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/foobar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("/foo"), new File("/bar"), true)); + } + + @Test + public void isLeadingPathAndTrailingSlashesOnUnix() throws IOException { + Assume.assumeFalse("Test doesn't run on DOS", PATH_SEP.equals(";")); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/bar/"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/"), true)); + + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/bar"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/bar/"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo/"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo/"), new File("/foo"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("/foo"), new File("/foo/"), false)); + } + + @Test + public void isLeadingPathCanonicalVersionWorksAsExpectedOnDos() throws IOException { + Assume.assumeTrue("Test only runs on DOS", PATH_SEP.equals(";")); + Assert.assertTrue(FileUtil.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\baz\\..\\bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\..\\foo\\bar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("C:\\foo"), new File("C:\\foobar"), true)); + Assert.assertFalse(FileUtil.isLeadingPath(new File("C:\\foo"), new File("C:\\bar"), true)); + } + + @Test + public void isLeadingPathAndTrailingSlashesOnDos() throws IOException { + Assume.assumeTrue("Test only runs on DOS", PATH_SEP.equals(";")); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\bar"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\bar\\"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo"), true)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\"), true)); + + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\bar"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\bar\\"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo\\"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo\\"), new File("c:\\foo"), false)); + Assert.assertTrue(FileUtil.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\"), false)); + } } diff --git a/test/java/org/apache/ivy/core/pack/ZipPackingTest.java b/test/java/org/apache/ivy/core/pack/ZipPackingTest.java new file mode 100644 index 00000000..5435dff4 --- /dev/null +++ b/test/java/org/apache/ivy/core/pack/ZipPackingTest.java @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package org.apache.ivy.core.pack; + +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; + +import org.apache.ivy.TestHelper; +import org.apache.ivy.util.DefaultMessageLogger; +import org.apache.ivy.util.FileUtil; +import org.apache.ivy.util.Message; +import org.apache.tools.ant.Project; +import org.apache.tools.ant.taskdefs.Mkdir; +import org.apache.tools.ant.taskdefs.Delete; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class ZipPackingTest { + + private static final Project PROJECT = TestHelper.newProject(); + private static final File TEST_DIR = PROJECT.resolveFile("build/test/pack"); + + @Before + public void setUp() { + Mkdir mkdir = new Mkdir(); + mkdir.setProject(PROJECT); + mkdir.setDir(TEST_DIR); + mkdir.execute(); + Message.setDefaultLogger(new DefaultMessageLogger(Message.MSG_INFO)); + } + + @After + public void tearDown() { + Delete del = new Delete(); + del.setProject(PROJECT); + del.setDir(TEST_DIR); + del.execute(); + } + + @Test + public void zipPackingExtractsArchive() throws IOException { + try (InputStream zip = new FileInputStream(PROJECT.resolveFile("test/zip/test.zip"))) { + new ZipPacking().unpack(zip, TEST_DIR); + } + assertTrue("Expecting file a", FileUtil.resolveFile(TEST_DIR, "a").isFile()); + assertTrue("Expecting directory b", FileUtil.resolveFile(TEST_DIR, "b").isDirectory()); + assertTrue("Expecting file b/c", FileUtil.resolveFile(TEST_DIR, "b/c").isFile()); + assertTrue("Expecting directory d", FileUtil.resolveFile(TEST_DIR, "d").isDirectory()); + assertFalse("Not expecting file e", PROJECT.resolveFile("build/test/e").exists()); + } +} diff --git a/test/zip/test.zip b/test/zip/test.zip new file mode 100644 index 00000000..b1b653a7 Binary files /dev/null and b/test/zip/test.zip differ
