Skip to content
This repository was archived by the owner on Sep 27, 2025. It is now read-only.

Commit c106ca9

Browse files
jeremylongjeremylong
authored
Merge pull request #1265 from jeremylong/snyk_report
Fix Possible Path Traversal from Archive Files
2 parents c541850 + cfa994d commit c106ca9

File tree

4 files changed

+131
-16
lines changed

4 files changed

+131
-16
lines changed

‎core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,11 @@ private void extractFiles(File archive, File destination, Engine engine) throws
412412
final String uncompressedName = GzipUtils.getUncompressedFilename(archive.getName());
413413
final File f = new File(destination, uncompressedName);
414414
if (engine.accept(f)) {
415+
final String destPath = destination.getCanonicalPath();
416+
if (!f.getCanonicalPath().startsWith(destPath)) {
417+
final String msg = String.format("Archive (%s) contains a file that would be written outside of the destination directory", archive.getPath());
418+
throw new AnalysisException(msg);
419+
}
415420
in = new BufferedInputStream(fis);
416421
gin = new GzipCompressorInputStream(in);
417422
decompressFile(gin, f);
@@ -420,6 +425,11 @@ private void extractFiles(File archive, File destination, Engine engine) throws
420425
final String uncompressedName = BZip2Utils.getUncompressedFilename(archive.getName());
421426
final File f = new File(destination, uncompressedName);
422427
if (engine.accept(f)) {
428+
final String destPath = destination.getCanonicalPath();
429+
if (!f.getCanonicalPath().startsWith(destPath)) {
430+
final String msg = String.format("Archive (%s) contains a file that would be written outside of the destination directory", archive.getPath());
431+
throw new AnalysisException(msg);
432+
}
423433
in = new BufferedInputStream(fis);
424434
bzin = new BZip2CompressorInputStream(in);
425435
decompressFile(bzin, f);
@@ -512,8 +522,15 @@ private void ensureReadableJar(final String archiveExt, BufferedInputStream in)
512522
private void extractArchive(ArchiveInputStream input, File destination, Engine engine) throws ArchiveExtractionException {
513523
ArchiveEntry entry;
514524
try {
525+
final String destPath = destination.getCanonicalPath();
515526
while ((entry = input.getNextEntry()) != null) {
516527
final File file = new File(destination, entry.getName());
528+
if (!file.getCanonicalPath().startsWith(destPath)) {
529+
final String msg = String.format(
530+
"Archive contains a file (%s) that would be extracted outside of the target directory.",
531+
file.getName());
532+
throw new ArchiveExtractionException(msg);
533+
}
517534
if (entry.isDirectory()) {
518535
if (!file.exists() && !file.mkdirs()) {
519536
final String msg = String.format("Unable to create directory '%s'.", file.getAbsolutePath());

‎core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java‎

Lines changed: 49 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -86,21 +86,38 @@ public static void extractFiles(File archive, File extractTo, Engine engine) thr
8686
if (archive == null || extractTo == null) {
8787
return;
8888
}
89-
89+
final String destPath;
90+
try {
91+
destPath = extractTo.getCanonicalPath();
92+
} catch (IOException ex) {
93+
throw new ExtractionException("Unable to extract files to destination path", ex);
94+
}
9095
ZipEntry entry;
9196
try (FileInputStream fis = new FileInputStream(archive);
9297
BufferedInputStream bis = new BufferedInputStream(fis);
9398
ZipInputStream zis = new ZipInputStream(bis)) {
9499
while ((entry = zis.getNextEntry()) != null) {
95100
if (entry.isDirectory()) {
96101
final File d = new File(extractTo, entry.getName());
102+
if (!d.getCanonicalPath().startsWith(destPath)) {
103+
final String msg = String.format(
104+
"Archive (%s) contains a path that would be extracted outside of the target directory.",
105+
archive.getAbsolutePath());
106+
throw new ExtractionException(msg);
107+
}
97108
if (!d.exists() && !d.mkdirs()) {
98109
final String msg = String.format("Unable to create '%s'.", d.getAbsolutePath());
99110
throw new ExtractionException(msg);
100111
}
101112
} else {
102113
final File file = new File(extractTo, entry.getName());
103114
if (engine == null || engine.accept(file)) {
115+
if (!file.getCanonicalPath().startsWith(destPath)) {
116+
final String msg = String.format(
117+
"Archive (%s) contains a file that would be extracted outside of the target directory.",
118+
archive.getAbsolutePath());
119+
throw new ExtractionException(msg);
120+
}
104121
try (FileOutputStream fos = new FileOutputStream(file)) {
105122
IOUtils.copy(zis, fos);
106123
} catch (FileNotFoundException ex) {
@@ -136,8 +153,7 @@ public static void extractFilesUsingFilter(File archive, File destination, Filen
136153
}
137154

138155
try (FileInputStream fis = new FileInputStream(archive)) {
139-
extractArchive(new ZipArchiveInputStream(new BufferedInputStream(
140-
fis)), destination, filter);
156+
extractArchive(new ZipArchiveInputStream(new BufferedInputStream(fis)), destination, filter);
141157
} catch (FileNotFoundException ex) {
142158
final String msg = String.format("Error extracting file `%s` with filter: %s", archive.getAbsolutePath(), ex.getMessage());
143159
LOGGER.debug(msg, ex);
@@ -163,9 +179,17 @@ private static void extractArchive(ArchiveInputStream input,
163179
throws ArchiveExtractionException {
164180
ArchiveEntry entry;
165181
try {
182+
String destPath = destination.getCanonicalPath();
183+
166184
while ((entry = input.getNextEntry()) != null) {
167185
if (entry.isDirectory()) {
168186
final File dir = new File(destination, entry.getName());
187+
if (!dir.getCanonicalPath().startsWith(destPath)) {
188+
final String msg = String.format(
189+
"Archive contains a path (%s) that would be extracted outside of the target directory.",
190+
dir.getAbsolutePath());
191+
throw new AnalysisException(msg);
192+
}
169193
if (!dir.exists() && !dir.mkdirs()) {
170194
final String msg = String.format(
171195
"Unable to create directory '%s'.",
@@ -197,21 +221,30 @@ private static void extractArchive(ArchiveInputStream input,
197221
private static void extractFile(ArchiveInputStream input, File destination,
198222
FilenameFilter filter, ArchiveEntry entry) throws ExtractionException {
199223
final File file = new File(destination, entry.getName());
200-
if (filter.accept(file.getParentFile(), file.getName())) {
201-
LOGGER.debug("Extracting '{}'", file.getPath());
202-
createParentFile(file);
224+
try {
225+
if (filter.accept(file.getParentFile(), file.getName())) {
226+
final String destPath = destination.getCanonicalPath();
227+
if (!file.getCanonicalPath().startsWith(destPath)) {
228+
final String msg = String.format(
229+
"Archive contains a file (%s) that would be extracted outside of the target directory.",
230+
file.getAbsolutePath());
231+
throw new ExtractionException(msg);
232+
}
233+
LOGGER.debug("Extracting '{}'", file.getPath());
234+
createParentFile(file);
203235

204-
try (FileOutputStream fos = new FileOutputStream(file)) {
205-
IOUtils.copy(input, fos);
206-
} catch (FileNotFoundException ex) {
207-
LOGGER.debug("", ex);
208-
final String msg = String.format("Unable to find file '%s'.", file.getName());
209-
throw new ExtractionException(msg, ex);
210-
} catch (IOException ex) {
211-
LOGGER.debug("", ex);
212-
final String msg = String.format("IO Exception while parsing file '%s'.", file.getName());
213-
throw new ExtractionException(msg, ex);
236+
try (FileOutputStream fos = new FileOutputStream(file)) {
237+
IOUtils.copy(input, fos);
238+
} catch (FileNotFoundException ex) {
239+
LOGGER.debug("", ex);
240+
final String msg = String.format("Unable to find file '%s'.", file.getName());
241+
throw new ExtractionException(msg, ex);
242+
}
214243
}
244+
} catch (IOException ex) {
245+
LOGGER.debug("", ex);
246+
final String msg = String.format("IO Exception while parsing file '%s'.", file.getName());
247+
throw new ExtractionException(msg, ex);
215248
}
216249
}
217250

‎core/src/test/java/org/owasp/dependencycheck/utils/ExtractionUtilTest.java‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/*
2+
* This file is part of dependency-check-core.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*
16+
* Copyright (c) 2018 Jeremy Long. All Rights Reserved.
17+
*/
18+
package org.owasp.dependencycheck.utils;
19+
20+
import java.io.File;
21+
import java.io.FilenameFilter;
22+
import org.apache.commons.io.filefilter.NameFileFilter;
23+
import org.junit.Test;
24+
import org.owasp.dependencycheck.BaseTest;
25+
import org.owasp.dependencycheck.Engine;
26+
27+
/**
28+
*
29+
* @author Jeremy Long
30+
*/
31+
public class ExtractionUtilTest extends BaseTest {
32+
33+
/**
34+
* Test of extractFiles method, of class ExtractionUtil.
35+
*/
36+
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
37+
public void testExtractFiles_File_File() throws Exception {
38+
File destination = getSettings().getTempDirectory();
39+
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
40+
ExtractionUtil.extractFiles(archive, destination);
41+
}
42+
43+
/**
44+
* Test of extractFiles method, of class ExtractionUtil.
45+
*/
46+
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
47+
public void testExtractFiles_3args() throws Exception {
48+
File destination = getSettings().getTempDirectory();
49+
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
50+
Engine engine = null;
51+
ExtractionUtil.extractFiles(archive, destination, engine);
52+
}
53+
54+
/**
55+
* Test of extractFilesUsingFilter method, of class ExtractionUtil.
56+
*/
57+
@Test(expected = org.owasp.dependencycheck.utils.ExtractionException.class)
58+
public void testExtractFilesUsingFilter() throws Exception {
59+
File destination = getSettings().getTempDirectory();
60+
File archive = BaseTest.getResourceAsFile(this, "evil.zip");
61+
ExtractionUtil.extractFiles(archive, destination);
62+
FilenameFilter filter = new NameFileFilter("evil.txt");
63+
ExtractionUtil.extractFilesUsingFilter(archive, destination, filter);
64+
}
65+
}

‎core/src/test/resources/evil.zip‎

382 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)