From b77c75d33d37e96fc9da25c535f65879cf66fef9 Mon Sep 17 00:00:00 2001 From: Jaikiran Pai Date: Thu, 28 Jun 2018 11:12:19 +0530 Subject: [PATCH 1/8] Checking to see if our Jenkins setup has issues with HTTPS cert of apache.org for older Java runtime versions --- src/tests/antunit/taskdefs/get-test.xml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/tests/antunit/taskdefs/get-test.xml b/src/tests/antunit/taskdefs/get-test.xml index b4543f579..44d5af925 100644 --- a/src/tests/antunit/taskdefs/get-test.xml +++ b/src/tests/antunit/taskdefs/get-test.xml @@ -20,8 +20,12 @@ - - + + + + + + @@ -124,7 +128,7 @@ + from HTTP to HTTPS works without an error. See bugzilla-62499 for details" if="java.atleast.1_8"> From 29d13b436a34bb95d3d48b67b75fb9d0854280c5 Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Thu, 28 Jun 2018 17:16:18 +0200 Subject: [PATCH 2/8] clarify handling of symbolic links --- src/main/org/apache/tools/ant/util/FileUtils.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/main/org/apache/tools/ant/util/FileUtils.java b/src/main/org/apache/tools/ant/util/FileUtils.java index c36eae7ec..9b5ff4f9e 100644 --- a/src/main/org/apache/tools/ant/util/FileUtils.java +++ b/src/main/org/apache/tools/ant/util/FileUtils.java @@ -1147,6 +1147,9 @@ public class FileUtils { /** * Removes a leading path from a second path. * + *

This method uses {@link #normalize} under the covers and + * does not resolve symbolic links.

+ * * @param leading The leading path, must not be null, must be absolute. * @param path The path to remove from, must not be null, must be absolute. * @@ -1171,8 +1174,12 @@ public class FileUtils { /** * Learn whether one path "leads" another. + * + *

This method uses {@link #normalize} under the covers and + * does not resolve symbolic links.

+ * * @param leading The leading path, must not be null, must be absolute. - * @param path The path to remove from, 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 */ From 8cfee71c77e69876a898b40605082f75a6e0420f Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 10:28:10 +0200 Subject: [PATCH 3/8] document what FileUtils#normalize does in a certain edge case --- src/main/org/apache/tools/ant/util/FileUtils.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/org/apache/tools/ant/util/FileUtils.java b/src/main/org/apache/tools/ant/util/FileUtils.java index 9b5ff4f9e..5e5b94611 100644 --- a/src/main/org/apache/tools/ant/util/FileUtils.java +++ b/src/main/org/apache/tools/ant/util/FileUtils.java @@ -723,8 +723,12 @@ public class FileUtils { *
  • DOS style paths that start with a drive letter will have * \ as the separator.
    • * - * Unlike {@link File#getCanonicalPath()} this method - * specifically does not resolve symbolic links. + *

    Unlike {@link File#getCanonicalPath()} this method + * specifically does not resolve symbolic links.

    + * + *

    If the path tries to go beyond the file system root (i.e. it + * contains more ".." segments than can be travelled up) the + * method will return the original path unchanged.

    * * @param path the path to be normalized. * @return the normalized version of the path. From d064f5f7d3e42026b6c03ce24f403eb9c5ba0a58 Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 10:36:21 +0200 Subject: [PATCH 4/8] ensure isLeadingPath cannot be subverted by too many double-dots https://bz.apache.org/bugzilla/show_bug.cgi?id=62502 --- src/main/org/apache/tools/ant/util/FileUtils.java | 9 +++++++++ .../junit/org/apache/tools/ant/util/FileUtilsTest.java | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/main/org/apache/tools/ant/util/FileUtils.java b/src/main/org/apache/tools/ant/util/FileUtils.java index 5e5b94611..1f3aba51d 100644 --- a/src/main/org/apache/tools/ant/util/FileUtils.java +++ b/src/main/org/apache/tools/ant/util/FileUtils.java @@ -1182,6 +1182,10 @@ public class FileUtils { *

    This method uses {@link #normalize} under the covers and * does not resolve symbolic links.

    * + *

    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.

    + * * @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. @@ -1198,6 +1202,11 @@ public class FileUtils { 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); } diff --git a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java index c52015654..0e99f630e 100644 --- a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java +++ b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java @@ -591,6 +591,16 @@ public class FileUtilsTest { FILE_UTILS.getDefaultEncoding(); } + /** + * @see "https://bz.apache.org/bugzilla/show_bug.cgi?id=62502" + */ + @Test + public void isLeadingPathCannotBeFooledByTooManyDoubleDots() { + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../../bar"))); + assertFalse(FILE_UTILS.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\..\\..\\bar"))); + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../.."))); + } + /** * adapt file separators to local conventions */ From 31f0b01804286bc64f94bfe0c85a40f878dda650 Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 10:36:52 +0200 Subject: [PATCH 5/8] enable forgotten test --- src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java index 0e99f630e..fef72716f 100644 --- a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java +++ b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java @@ -586,6 +586,8 @@ public class FileUtilsTest { } } + + @Test public void testGetDefaultEncoding() { // This just tests that the function does not blow up FILE_UTILS.getDefaultEncoding(); From 6a41d62cb9ab4e640b72cb4de42a6c211dea645d Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 11:03:01 +0200 Subject: [PATCH 6/8] add additional isLeadingPath method that resolves symlinks --- .../org/apache/tools/ant/util/FileUtils.java | 30 ++++++++++++++++++ .../apache/tools/ant/util/FileUtilsTest.java | 31 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/main/org/apache/tools/ant/util/FileUtils.java b/src/main/org/apache/tools/ant/util/FileUtils.java index 1f3aba51d..900b65b51 100644 --- a/src/main/org/apache/tools/ant/util/FileUtils.java +++ b/src/main/org/apache/tools/ant/util/FileUtils.java @@ -1210,6 +1210,36 @@ public class FileUtils { 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 boolean isLeadingPath(File leading, File path, boolean resolveSymlinks) + throws IOException { + if (!resolveSymlinks) { + return isLeadingPath(leading, path); + } + String l = leading.getCanonicalPath(); + String p = path.getCanonicalPath(); + 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; + } + return p.startsWith(l); + } + /** * Constructs a file: URI that represents the * external form of the given pathname. diff --git a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java index fef72716f..716751d4d 100644 --- a/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java +++ b/src/tests/junit/org/apache/tools/ant/util/FileUtilsTest.java @@ -32,6 +32,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; /** @@ -603,6 +604,36 @@ public class FileUtilsTest { assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../.."))); } + /** + * @see "https://bz.apache.org/bugzilla/show_bug.cgi?id=62502" + */ + @Test + public void isLeadingPathCanonicalVersionCannotBeFooledByTooManyDoubleDots() throws IOException { + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../../bar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("c:\\foo"), new File("c:\\foo\\..\\..\\bar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../.."), true)); + } + + @Test + public void isLeadingPathCanonicalVersionWorksAsExpectedOnUnix() throws IOException { + assumeFalse("Test doesn't run on DOS", Os.isFamily("dos")); + assertTrue(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/bar"), true)); + assertTrue(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/baz/../bar"), true)); + assertTrue(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foo/../foo/bar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/foobar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("/foo"), new File("/bar"), true)); + } + + @Test + public void isLeadingPathCanonicalVersionWorksAsExpectedOnDos() throws IOException { + assumeTrue("Test only runs on DOS", Os.isFamily("dos")); + assertTrue(FILE_UTILS.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\bar"), true)); + assertTrue(FILE_UTILS.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\baz\\..\\bar"), true)); + assertTrue(FILE_UTILS.isLeadingPath(new File("C:\\foo"), new File("C:\\foo\\..\\foo\\bar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("C:\\foo"), new File("C:\\foobar"), true)); + assertFalse(FILE_UTILS.isLeadingPath(new File("C:\\foo"), new File("C:\\bar"), true)); + } + /** * adapt file separators to local conventions */ From 5a8c37b271677587046bfd0fea18c1675d5a6300 Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 11:03:28 +0200 Subject: [PATCH 7/8] take symlinks into account when expanding archives and checking entries --- src/main/org/apache/tools/ant/taskdefs/Expand.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/org/apache/tools/ant/taskdefs/Expand.java b/src/main/org/apache/tools/ant/taskdefs/Expand.java index b3897efc6..039f203ff 100644 --- a/src/main/org/apache/tools/ant/taskdefs/Expand.java +++ b/src/main/org/apache/tools/ant/taskdefs/Expand.java @@ -333,9 +333,9 @@ public class Expand extends Task { mappedNames = new String[] {entryName}; } File f = fileUtils.resolveFile(dir, mappedNames[0]); - if (!allowedOutsideOfDest && !fileUtils.isLeadingPath(dir, f)) { - log("skipping " + entryName + " as its target " + f + " is outside of " - + dir + ".", Project.MSG_VERBOSE); + if (!allowedOutsideOfDest && !fileUtils.isLeadingPath(dir, f, true)) { + log("skipping " + entryName + " as its target " + f.getCanonicalPath() + + " is outside of " + dir.getCanonicalPath() + ".", Project.MSG_VERBOSE); return; } From 52655511902ff59b9bc49c743bd93dd8ddb088e8 Mon Sep 17 00:00:00 2001 From: Stefan Bodewig Date: Sun, 1 Jul 2018 11:20:48 +0200 Subject: [PATCH 8/8] record fix of https://bz.apache.org/bugzilla/show_bug.cgi?id=62502 --- WHATSNEW | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/WHATSNEW b/WHATSNEW index 527d0e291..e65f3233b 100644 --- a/WHATSNEW +++ b/WHATSNEW @@ -9,6 +9,12 @@ Fixed bugs: an exception. Bugzilla Report 62499 + * the new allowFilesToEscapeDest didn't work when set to false and + archive entries contained relative paths with so many ".." + segnments that the resulting path would go beyond the file system + root. + Bugzilla Report 62502 + Changes from Ant 1.9.11 TO Ant 1.9.12 =====================================