diff --git a/src/main/java/org/zeroturnaround/zip/ZipUtil.java b/src/main/java/org/zeroturnaround/zip/ZipUtil.java index 7ebc8dd..604866e 100644 --- a/src/main/java/org/zeroturnaround/zip/ZipUtil.java +++ b/src/main/java/org/zeroturnaround/zip/ZipUtil.java @@ -1150,6 +1150,15 @@ public void process(InputStream in, ZipEntry zipEntry) throws IOException { String name = mapper.map(zipEntry.getName()); if (name != null) { File file = new File(outputDir, name); + + /* If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. See + * DirectoryTraversalMaliciousTest for details. + */ + if (name.indexOf("..") != -1 && !file.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) { + throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file."); + } + if (zipEntry.isDirectory()) { FileUtils.forceMkdir(file); } @@ -1218,11 +1227,29 @@ public void process(InputStream in, ZipEntry zipEntry) throws IOException { parentDirectory = file; } File destFile = new File(parentDirectory, dirs[dirs.length - 1]); + + /* If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. See + * DirectoryTraversalMaliciousTest for details. + */ + if (name.indexOf("..") != -1 && !destFile.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) { + throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file."); + } + FileUtils.copy(in, destFile); } // it could be that there are just top level files that the unpacker is used for else { File destFile = new File(outputDir, name); + + /* If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. See + * DirectoryTraversalMaliciousTest for details. + */ + if (name.indexOf("..") != -1 && !destFile.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) { + throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file."); + } + FileUtils.copy(in, destFile); } } @@ -1258,6 +1285,15 @@ else if (!rootDir.equals(root)) { String name = mapper.map(getUnrootedName(root, zipEntry.getName())); if (name != null) { File file = new File(outputDir, name); + + /* If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. See + * DirectoryTraversalMaliciousTest for details. + */ + if (name.indexOf("..") != -1 && !file.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) { + throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file."); + } + if (zipEntry.isDirectory()) { FileUtils.forceMkdir(file); } diff --git a/src/test/java/org/zeroturnaround/zip/DirectoryTraversalMaliciousTest.java b/src/test/java/org/zeroturnaround/zip/DirectoryTraversalMaliciousTest.java new file mode 100644 index 0000000..8eede3d --- /dev/null +++ b/src/test/java/org/zeroturnaround/zip/DirectoryTraversalMaliciousTest.java @@ -0,0 +1,61 @@ +package org.zeroturnaround.zip; + +/** + * Copyright (C) 2012 ZeroTurnaround LLC + * + * Licensed 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 + * + * http://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. + */ +import java.io.File; + +import junit.framework.TestCase; + +public class DirectoryTraversalMaliciousTest extends TestCase { + /* + * This is the contents of the file. There is one evil file that tries to get out of the + * target. + * + * $ unzip -t zip-slip.zip + * Archive: zip-slip.zip + * testing: good.txt OK + * testing: ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt OK + * No errors detected in compressed data of zip-slip.zip. + */ + private static final File badFile = new File("src/test/resources/zip-malicious-traversal.zip"); + private static final File badFileBackslashes = new File("src/test/resources/zip-malicious-traversal-backslashes.zip"); + + public void testUnpackDoesntLeaveTarget() throws Exception { + File file = File.createTempFile("temp", null); + File tmpDir = file.getParentFile(); + + try { + ZipUtil.unpack(badFile, tmpDir); + fail(); + } + catch (ZipException e) { + assertTrue(true); + } + } + + public void testUnwrapDoesntLeaveTarget() throws Exception { + File file = File.createTempFile("temp", null); + File tmpDir = file.getParentFile(); + + try { + ZipUtil.iterate(badFileBackslashes, new ZipUtil.BackslashUnpacker(tmpDir)); + fail(); + } + catch (ZipException e) { + assertTrue(true); + } + } +} diff --git a/src/test/resources/zip-malicious-traversal-backslashes.zip b/src/test/resources/zip-malicious-traversal-backslashes.zip new file mode 100644 index 0000000..a67ff8b Binary files /dev/null and b/src/test/resources/zip-malicious-traversal-backslashes.zip differ diff --git a/src/test/resources/zip-malicious-traversal.zip b/src/test/resources/zip-malicious-traversal.zip new file mode 100644 index 0000000..38b3f49 Binary files /dev/null and b/src/test/resources/zip-malicious-traversal.zip differ