From 89a75b024e45f687b1152fbb89ea47f7cbad75ce Mon Sep 17 00:00:00 2001 From: Michael Simacek Date: Tue, 26 Jun 2018 15:17:33 +0200 Subject: [PATCH] Fix arbitrary file write vulnerability --- WHATSNEW | 18 +++++++ manual/Tasks/unzip.html | 12 ++++- .../org/apache/tools/ant/taskdefs/Expand.java | 37 ++++++++++++-- src/tests/antunit/taskdefs/unzip-test.xml | 46 ++++++++++++++++++ .../taskdefs/zip/direscape-absolute.zip | Bin 0 -> 332 bytes src/tests/antunit/taskdefs/zip/direscape.zip | Bin 0 -> 332 bytes 6 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 src/tests/antunit/taskdefs/zip/direscape-absolute.zip create mode 100644 src/tests/antunit/taskdefs/zip/direscape.zip diff --git a/WHATSNEW b/WHATSNEW index 1e9b3398b..1ff99fc98 100644 --- a/WHATSNEW +++ b/WHATSNEW @@ -1,3 +1,21 @@ +Backported changes +================== + +Changes that could break older environments: +------------------------------------------- + + * , and will no longer extract entries whose + names would make the created files be placed outside of the + destination directory anymore by default. A new attribute + allowFilesToEscapeDest can be used to override the behavior. + Another special case is when stripAbsolutePathSpec is false (which + no longer is the default) and the entry's name starts with a + (back)slash and allowFilesToEscapeDest hasn't been specified + explicitly, in this case the file may be created outside of the + dest directory as well. + In addition stripAbsolutePathSpec is now true by default. + Based on a recommendation by the Snyk Security Research Team. + Changes from Ant 1.9.3 TO Ant 1.9.4 =================================== diff --git a/manual/Tasks/unzip.html b/manual/Tasks/unzip.html index 02df7acf9..a5fc40fd3 100644 --- a/manual/Tasks/unzip.html +++ b/manual/Tasks/unzip.html @@ -125,7 +125,8 @@ archive.

Note that this changes the entry's name before applying include/exclude patterns and before using the nested mappers (if any). since Ant 1.8.0 - No, defaults to false + No, defaults to true since 1.9.12 + (used to defaukt to false prior to that) scanForUnicodeExtraFields @@ -137,6 +138,15 @@ archive.

zip task page No, defaults to true + + allowFilesToEscapeDest + Whether to allow the extracted file or directory + to be outside of the dest directory. + since Ant 1.9.12 + No, defaults to false unless + stripAbsolutePathSpec is true and the entry's name starts with a leading + path spec. +

Examples

diff --git a/src/main/org/apache/tools/ant/taskdefs/Expand.java b/src/main/org/apache/tools/ant/taskdefs/Expand.java
index 8722e2402..ac5287cb8 100644
--- a/src/main/org/apache/tools/ant/taskdefs/Expand.java
+++ b/src/main/org/apache/tools/ant/taskdefs/Expand.java
@@ -67,8 +67,9 @@ public class Expand extends Task {
     private Union resources = new Union();
     private boolean resourcesSpecified = false;
     private boolean failOnEmptyArchive = false;
-    private boolean stripAbsolutePathSpec = false;
+    private boolean stripAbsolutePathSpec = true;
     private boolean scanForUnicodeExtraFields = true;
+    private Boolean allowFilesToEscapeDest = null;
 
     public static final String NATIVE_ENCODING = "native-encoding";
 
@@ -240,14 +241,17 @@ public class Expand extends Task {
                                boolean isDirectory, FileNameMapper mapper)
                                throws IOException {
 
-        if (stripAbsolutePathSpec && entryName.length() > 0
+        final boolean entryNameStartsWithPathSpec = entryName.length() > 0
             && (entryName.charAt(0) == File.separatorChar
                 || entryName.charAt(0) == '/'
-                || entryName.charAt(0) == '\\')) {
+                || entryName.charAt(0) == '\\');
+        if (stripAbsolutePathSpec && entryNameStartsWithPathSpec) {
             log("stripped absolute path spec from " + entryName,
                 Project.MSG_VERBOSE);
             entryName = entryName.substring(1);
         }
+        boolean allowedOutsideOfDest = Boolean.TRUE == getAllowFilesToEscapeDest()
+            || null == getAllowFilesToEscapeDest() && !stripAbsolutePathSpec && entryNameStartsWithPathSpec;
 
         if (patternsets != null && patternsets.size() > 0) {
             String name = entryName.replace('/', File.separatorChar)
@@ -313,6 +317,12 @@ 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);
+                return;
+        }
+
         try {
             if (!overwrite && f.exists()
                 && f.lastModified() >= entryDate.getTime()) {
@@ -508,4 +518,25 @@ public class Expand extends Task {
         return scanForUnicodeExtraFields;
     }
 
+    /**
+     * Whether to allow the extracted file or directory to be outside of the dest directory.
+     *
+     * @param b the flag
+     * @since Ant 1.9.12
+     */
+    public void setAllowFilesToEscapeDest(boolean b) {
+        allowFilesToEscapeDest = b;
+    }
+
+    /**
+     * Whether to allow the extracted file or directory to be outside of the dest directory.
+     *
+     * @return {@code null} if the flag hasn't been set explicitly,
+     * otherwise the value set by the user.
+     * @since Ant 1.9.12
+     */
+    public Boolean getAllowFilesToEscapeDest() {
+        return allowFilesToEscapeDest;
+    }
+
 }
diff --git a/src/tests/antunit/taskdefs/unzip-test.xml b/src/tests/antunit/taskdefs/unzip-test.xml
index b2c2105dd..bdf5f61e1 100644
--- a/src/tests/antunit/taskdefs/unzip-test.xml
+++ b/src/tests/antunit/taskdefs/unzip-test.xml
@@ -24,6 +24,10 @@
     
   
 
+  
+    
+  
+
   
     
@@ -67,4 +71,46 @@
     
     
   
+
+  
+    
+    
+    
+    
+  
+
+  
+    
+    
+    
+    
+  
+
+  
+    
+    
+    
+    
+    
+    
+  
+
+  
+    
+    
+  
+
+  
+    
+    
+  
 
diff --git a/src/tests/antunit/taskdefs/zip/direscape-absolute.zip b/src/tests/antunit/taskdefs/zip/direscape-absolute.zip
new file mode 100644
index 000000000..0bae4aaf1
--- /dev/null
+++ b/src/tests/antunit/taskdefs/zip/direscape-absolute.zip
@@ -0,0 +1,5 @@
+PK
+L
/tmp/testdir/UT	7lZnZuxPK
+L/tmp/testdir/aUT	7lZJlZuxPK
+L
A/tmp/testdir/UT7lZuxPK
+LG/tmp/testdir/aUT7lZuxPK
\ No newline at end of file
diff --git a/src/tests/antunit/taskdefs/zip/direscape.zip b/src/tests/antunit/taskdefs/zip/direscape.zip
new file mode 100644
index 000000000..63cefd2d8
--- /dev/null
+++ b/src/tests/antunit/taskdefs/zip/direscape.zip
@@ -0,0 +1,5 @@
+PK
+L
../testinput/UT	7lZnZuxPK
+L../testinput/aUT	7lZJlZuxPK
+L
A../testinput/UT7lZuxPK
+LG../testinput/aUT7lZuxPK
\ No newline at end of file
-- 
2.17.1