Skip to content

Commit

Permalink
Merge pull request #1265 from jeremylong/snyk_report
Browse files Browse the repository at this point in the history 
Fix Possible Path Traversal from Archive Files
  • Loading branch information
@jeremylong
jeremylong committed May 7, 2018
2 parents c541850 + cfa994d commit c106ca9
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 16 deletions.
17 changes: 17 additions & 0 deletions core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,11 @@ private void extractFiles(File archive, File destination, Engine engine) throws
final String uncompressedName = GzipUtils.getUncompressedFilename(archive.getName());
final File f = new File(destination, uncompressedName);
if (engine.accept(f)) {
final String destPath = destination.getCanonicalPath();
if (!f.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format("Archive (%s) contains a file that would be written outside of the destination directory", archive.getPath());
throw new AnalysisException(msg);
}
in = new BufferedInputStream(fis);
gin = new GzipCompressorInputStream(in);
decompressFile(gin, f);
Expand All @@ -420,6 +425,11 @@ private void extractFiles(File archive, File destination, Engine engine) throws
final String uncompressedName = BZip2Utils.getUncompressedFilename(archive.getName());
final File f = new File(destination, uncompressedName);
if (engine.accept(f)) {
final String destPath = destination.getCanonicalPath();
if (!f.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format("Archive (%s) contains a file that would be written outside of the destination directory", archive.getPath());
throw new AnalysisException(msg);
}
in = new BufferedInputStream(fis);
bzin = new BZip2CompressorInputStream(in);
decompressFile(bzin, f);
Expand Down Expand Up @@ -512,8 +522,15 @@ private void ensureReadableJar(final String archiveExt, BufferedInputStream in)
private void extractArchive(ArchiveInputStream input, File destination, Engine engine) throws ArchiveExtractionException {
ArchiveEntry entry;
try {
final String destPath = destination.getCanonicalPath();
while ((entry = input.getNextEntry()) != null) {
final File file = new File(destination, entry.getName());
if (!file.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format(
"Archive contains a file (%s) that would be extracted outside of the target directory.",
file.getName());
throw new ArchiveExtractionException(msg);
}
if (entry.isDirectory()) {
if (!file.exists() && !file.mkdirs()) {
final String msg = String.format("Unable to create directory '%s'.", file.getAbsolutePath());
Expand Down
65 changes: 49 additions & 16 deletions core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,21 +86,38 @@ public static void extractFiles(File archive, File extractTo, Engine engine) thr
if (archive == null || extractTo == null) {
return;
}

final String destPath;
try {
destPath = extractTo.getCanonicalPath();
} catch (IOException ex) {
throw new ExtractionException("Unable to extract files to destination path", ex);
}
ZipEntry entry;
try (FileInputStream fis = new FileInputStream(archive);
BufferedInputStream bis = new BufferedInputStream(fis);
ZipInputStream zis = new ZipInputStream(bis)) {
while ((entry = zis.getNextEntry()) != null) {
if (entry.isDirectory()) {
final File d = new File(extractTo, entry.getName());
if (!d.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format(
"Archive (%s) contains a path that would be extracted outside of the target directory.",
archive.getAbsolutePath());
throw new ExtractionException(msg);
}
if (!d.exists() && !d.mkdirs()) {
final String msg = String.format("Unable to create '%s'.", d.getAbsolutePath());
throw new ExtractionException(msg);
}
} else {
final File file = new File(extractTo, entry.getName());
if (engine == null || engine.accept(file)) {
if (!file.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format(
"Archive (%s) contains a file that would be extracted outside of the target directory.",
archive.getAbsolutePath());
throw new ExtractionException(msg);
}
try (FileOutputStream fos = new FileOutputStream(file)) {
IOUtils.copy(zis, fos);
} catch (FileNotFoundException ex) {
Expand Down Expand Up @@ -136,8 +153,7 @@ public static void extractFilesUsingFilter(File archive, File destination, Filen
}

try (FileInputStream fis = new FileInputStream(archive)) {
extractArchive(new ZipArchiveInputStream(new BufferedInputStream(
fis)), destination, filter);
extractArchive(new ZipArchiveInputStream(new BufferedInputStream(fis)), destination, filter);
} catch (FileNotFoundException ex) {
final String msg = String.format("Error extracting file `%s` with filter: %s", archive.getAbsolutePath(), ex.getMessage());
LOGGER.debug(msg, ex);
Expand All @@ -163,9 +179,17 @@ private static void extractArchive(ArchiveInputStream input,
throws ArchiveExtractionException {
ArchiveEntry entry;
try {
String destPath = destination.getCanonicalPath();

while ((entry = input.getNextEntry()) != null) {
if (entry.isDirectory()) {
final File dir = new File(destination, entry.getName());
if (!dir.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format(
"Archive contains a path (%s) that would be extracted outside of the target directory.",
dir.getAbsolutePath());
throw new AnalysisException(msg);
}
if (!dir.exists() && !dir.mkdirs()) {
final String msg = String.format(
"Unable to create directory '%s'.",
Expand Down Expand Up @@ -197,21 +221,30 @@ private static void extractArchive(ArchiveInputStream input,
private static void extractFile(ArchiveInputStream input, File destination,
FilenameFilter filter, ArchiveEntry entry) throws ExtractionException {
final File file = new File(destination, entry.getName());
if (filter.accept(file.getParentFile(), file.getName())) {
LOGGER.debug("Extracting '{}'", file.getPath());
createParentFile(file);
try {
if (filter.accept(file.getParentFile(), file.getName())) {
final String destPath = destination.getCanonicalPath();
if (!file.getCanonicalPath().startsWith(destPath)) {
final String msg = String.format(
"Archive contains a file (%s) that would be extracted outside of the target directory.",
file.getAbsolutePath());
throw new ExtractionException(msg);
}
LOGGER.debug("Extracting '{}'", file.getPath());
createParentFile(file);

try (FileOutputStream fos = new FileOutputStream(file)) {
IOUtils.copy(input, fos);
} catch (FileNotFoundException ex) {
LOGGER.debug("", ex);
final String msg = String.format("Unable to find file '%s'.", file.getName());
throw new ExtractionException(msg, ex);
} catch (IOException ex) {
LOGGER.debug("", ex);
final String msg = String.format("IO Exception while parsing file '%s'.", file.getName());
throw new ExtractionException(msg, ex);
try (FileOutputStream fos = new FileOutputStream(file)) {
IOUtils.copy(input, fos);
} catch (FileNotFoundException ex) {
LOGGER.debug("", ex);
final String msg = String.format("Unable to find file '%s'.", file.getName());
throw new ExtractionException(msg, ex);
}
}
} catch (IOException ex) {
LOGGER.debug("", ex);
final String msg = String.format("IO Exception while parsing file '%s'.", file.getName());
throw new ExtractionException(msg, ex);
}
}

Expand Down
65 changes: 65 additions & 0 deletions core/src/test/java/org/owasp/dependencycheck/utils/ExtractionUtilTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* This file is part of dependency-check-core.
*
* 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.
*
* Copyright (c) 2018 Jeremy Long. All Rights Reserved.
*/
package org.owasp.dependencycheck.utils;

import java.io.File;
import java.io.FilenameFilter;
import org.apache.commons.io.filefilter.NameFileFilter;
import org.junit.Test;
import org.owasp.dependencycheck.BaseTest;
import org.owasp.dependencycheck.Engine;

/**
*
* @author Jeremy Long
*/
public class ExtractionUtilTest extends BaseTest {

/**
* Test of extractFiles method, of class ExtractionUtil.
*/
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
public void testExtractFiles_File_File() throws Exception {
File destination = getSettings().getTempDirectory();
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
ExtractionUtil.extractFiles(archive, destination);
}

/**
* Test of extractFiles method, of class ExtractionUtil.
*/
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
public void testExtractFiles_3args() throws Exception {
File destination = getSettings().getTempDirectory();
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
Engine engine = null;
ExtractionUtil.extractFiles(archive, destination, engine);
}

/**
* Test of extractFilesUsingFilter method, of class ExtractionUtil.
*/
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
public void testExtractFilesUsingFilter() throws Exception {
File destination = getSettings().getTempDirectory();
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
ExtractionUtil.extractFiles(archive, destination);
FilenameFilter filter = new NameFileFilter("evil.txt");
ExtractionUtil.extractFilesUsingFilter(archive, destination, filter);
}
}
Binary file added core/src/test/resources/evil.zip
View file
Binary file not shown.

0 comments on commit c106ca9

Please sign in to comment.