245 lines
9.6 KiB
Diff
245 lines
9.6 KiB
Diff
From 6a0f7b1514cf6163aea998bef6367127abcf413b Mon Sep 17 00:00:00 2001
|
||
From: Stefan Bodewig <bodewig@apache.org>
|
||
Date: Sat, 21 Apr 2018 19:55:02 +0200
|
||
Subject: [PATCH] Fix arbitrary file write vulnerability
|
||
|
||
Original commit messages:
|
||
|
||
unzip and friends could monitor where they write more closely
|
||
|
||
forgot to update the manual
|
||
|
||
and forgot two words, oh my
|
||
|
||
change stripAbsolutePathSpec's default, credit Snyk
|
||
---
|
||
WHATSNEW | 15 ++++++
|
||
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, 106 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 11e5babf6..472aaa21a 100644
|
||
--- a/WHATSNEW
|
||
+++ b/WHATSNEW
|
||
@@ -1,6 +1,21 @@
|
||
Changes from Ant 1.10.0 TO Ant 1.10.1
|
||
=====================================
|
||
|
||
+Changes that could break older environments:
|
||
+-------------------------------------------
|
||
+
|
||
+ * <unzip>, <unjar> and <untar> 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.
|
||
+
|
||
Fixed bugs:
|
||
-----------
|
||
|
||
diff --git a/manual/Tasks/unzip.html b/manual/Tasks/unzip.html
|
||
index 8d93b7041..856b50f09 100644
|
||
--- a/manual/Tasks/unzip.html
|
||
+++ b/manual/Tasks/unzip.html
|
||
@@ -126,7 +126,8 @@ <h3>Parameters</h3>
|
||
Note that this changes the entry's name before applying
|
||
include/exclude patterns and before using the nested mappers (if
|
||
any). <em>since Ant 1.8.0</em></td>
|
||
- <td valign="top" align="center">No, defaults to false</td>
|
||
+ <td valign="top" align="center">No, defaults to true since 1.9.12
|
||
+ (used to defaukt to false prior to that)</td>
|
||
</tr>
|
||
<tr>
|
||
<td valign="top">scanForUnicodeExtraFields</td>
|
||
@@ -138,6 +139,15 @@ <h3>Parameters</h3>
|
||
zip task page</a></td>
|
||
<td align="center" valign="top">No, defaults to true</td>
|
||
</tr>
|
||
+ <tr>
|
||
+ <td valign="top">allowFilesToEscapeDest</td>
|
||
+ <td valign="top">Whether to allow the extracted file or directory
|
||
+ to be outside of the dest directory.
|
||
+ <em>since Ant 1.9.12</em></td>
|
||
+ <td valign="top" align="center">No, defaults to false unless
|
||
+ stripAbsolutePathSpec is true and the entry's name starts with a leading
|
||
+ path spec.</td>
|
||
+ </tr>
|
||
</table>
|
||
<h3>Examples</h3>
|
||
<pre>
|
||
diff --git a/src/main/org/apache/tools/ant/taskdefs/Expand.java b/src/main/org/apache/tools/ant/taskdefs/Expand.java
|
||
index 0ec233308..744ef63a2 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";
|
||
|
||
@@ -256,14 +257,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)
|
||
@@ -329,6 +333,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()) {
|
||
@@ -524,4 +534,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 @@
|
||
<mkdir dir="${output}" />
|
||
</target>
|
||
|
||
+ <target name="tearDown" depends="antunit-base.tearDown">
|
||
+ <delete dir="/tmp/testdir"/>
|
||
+ </target>
|
||
+
|
||
<target name="testFailureOnBrokenCentralDirectoryStructure">
|
||
<au:expectfailure
|
||
expectedmessage="central directory is empty, can't expand corrupt archive.">
|
||
@@ -67,4 +71,46 @@
|
||
<!-- failed on Windows and other OSes with implicit file locking -->
|
||
<au:assertFileDoesntExist file="${input}/test.zip"/>
|
||
</target>
|
||
+
|
||
+ <target name="testEntriesDontEscapeDestByDefault">
|
||
+ <mkdir dir="${input}/"/>
|
||
+ <mkdir dir="${output}/"/>
|
||
+ <unzip src="zip/direscape.zip" dest="${output}"/>
|
||
+ <au:assertFileDoesntExist file="${input}/a"/>
|
||
+ </target>
|
||
+
|
||
+ <target name="testEntriesCanEscapeDestIfRequested">
|
||
+ <mkdir dir="${input}/"/>
|
||
+ <mkdir dir="${output}/"/>
|
||
+ <unzip src="zip/direscape.zip" dest="${output}" allowFilesToEscapeDest="true"/>
|
||
+ <au:assertFileExists file="${input}/a"/>
|
||
+ </target>
|
||
+
|
||
+ <target name="-can-write-to-tmp?">
|
||
+ <mkdir dir="${input}"/>
|
||
+ <echo file="${input}/A.java"><![CDATA[
|
||
+public class A {
|
||
+ public static void main(String[] args) {
|
||
+ new java.io.File("/tmp/testdir/").mkdirs();
|
||
+ }
|
||
+}
|
||
+]]></echo>
|
||
+ <mkdir dir="${output}"/>
|
||
+ <javac srcdir="${input}" destdir="${output}"/>
|
||
+ <java classname="A" classpath="${output}"/>
|
||
+ <available property="can-write-to-tmp!" file="/tmp/testdir/"/>
|
||
+ </target>
|
||
+
|
||
+ <target name="testEntriesCanEscapeDestViaAbsolutePathIfPermitted"
|
||
+ depends="-can-write-to-tmp?" if="can-write-to-tmp!">
|
||
+ <unzip src="zip/direscape-absolute.zip" dest="${output}"
|
||
+ stripAbsolutePathSpec="false"/>
|
||
+ <au:assertFileExists file="/tmp/testdir/a"/>
|
||
+ </target>
|
||
+
|
||
+ <target name="testEntriesDontEscapeDestViaAbsolutePathByDefault"
|
||
+ depends="-can-write-to-tmp?" if="can-write-to-tmp!">
|
||
+ <unzip src="zip/direscape-absolute.zip" dest="${output}"/>
|
||
+ <au:assertFileDoesntExist file="/tmp/testdir/a"/>
|
||
+ </target>
|
||
</project>
|
||
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
|
||
+ <00><><EFBFBD>L
|
||
/tmp/testdir/UT 7l<37>Zn<>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L /tmp/testdir/aUT 7l<37>ZJl<4A>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L
|
||
<00>A /tmp/testdir/UT 7l<37>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L <00><>G /tmp/testdir/aUT 7l<37>Zux <04> <04> PK <00> <00>
|
||
\ 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
|
||
+ <00><><EFBFBD>L
|
||
../testinput/UT 7l<37>Zn<>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L ../testinput/aUT 7l<37>ZJl<4A>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L
|
||
<00>A ../testinput/UT 7l<37>Zux <04> <04> PK
|
||
+ <00><><EFBFBD>L <00><>G ../testinput/aUT 7l<37>Zux <04> <04> PK <00> <00>
|
||
\ No newline at end of file
|
||
--
|
||
2.17.1
|
||
|