From bc501a96bf4b8f597f9cc210595f32f417b18670 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Mon, 3 Oct 2022 21:32:42 +0000 Subject: [PATCH] vuln-fix: Zip Slip Vulnerability This fixes a Zip-Slip vulnerability. This change does one of two things. This change either 1. Inserts a guard to protect against Zip Slip. OR 2. Replaces `dir.getCanonicalPath().startsWith(parent.getCanonicalPath())`, which is vulnerable to partial path traversal attacks, with the more secure `dir.getCanonicalFile().toPath().startsWith(parent.getCanonicalFile().toPath())`. For number 2, consider `"/usr/outnot".startsWith("/usr/out")`. The check is bypassed although `/outnot` is not under the `/out` directory. It's important to understand that the terminating slash may be removed when using various `String` representations of the `File` object. For example, on Linux, `println(new File("/var"))` will print `/var`, but `println(new File("/var", "/")` will print `/var/`; however, `println(new File("/var", "/").getCanonicalPath())` will print `/var`. Weakness: CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') Severity: High CVSSS: 7.4 Detection: CodeQL (https://codeql.github.com/codeql-query-help/java/java-zipslip/) & OpenRewrite (https://public.moderne.io/recipes/org.openrewrite.java.security.ZipSlip) Reported-by: Jonathan Leitschuh Signed-off-by: Jonathan Leitschuh Bug-tracker: https://github.com/JLLeitschuh/security-research/issues/16 Co-authored-by: Moderne --- .../org/owasp/dependencycheck/utils/ExtractionUtil.java | 3 +++ .../dependencycheck/data/nvdcve/BaseDBTestCase.java | 9 ++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java index 6aed21164b1..47254781f27 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/utils/ExtractionUtil.java @@ -107,6 +107,9 @@ public static void extractFiles(File archive, File extractTo, Engine engine) thr } } else { final File file = new File(extractTo, entry.getName()); + if (!file.toPath().normalize().startsWith(extractTo.toPath().normalize())) { + throw new RuntimeException("Bad zip entry"); + } if (engine == null || engine.accept(file)) { BufferedOutputStream bos = null; FileOutputStream fos; diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/nvdcve/BaseDBTestCase.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/nvdcve/BaseDBTestCase.java index 652dc6e6023..d3d6056ead8 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/nvdcve/BaseDBTestCase.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/data/nvdcve/BaseDBTestCase.java @@ -17,11 +17,7 @@ */ package org.owasp.dependencycheck.data.nvdcve; -import java.io.BufferedInputStream; -import java.io.BufferedOutputStream; -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; +import java.io.*; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import org.junit.Before; @@ -72,6 +68,9 @@ public static void ensureDBExists() throws Exception { BufferedOutputStream dest = null; try { File o = new File(dataPath, entry.getName()); + if (!o.toPath().normalize().startsWith(dataPath.toPath().normalize())) { + throw new IOException("Bad zip entry"); + } o.createNewFile(); fos = new FileOutputStream(o, false); dest = new BufferedOutputStream(fos, BUFFER_SIZE);