From 3c714ca4cbedada24f53f710def0ec7aee74fbab Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 29 Jul 2022 19:15: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 --- .../core/provider/JarProvider.java | 282 +++++++------- .../core/provider/ZipProvider.java | 368 +++++++++--------- 2 files changed, 329 insertions(+), 321 deletions(-) diff --git a/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/JarProvider.java b/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/JarProvider.java index 4364c7b..b2868f2 100644 --- a/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/JarProvider.java +++ b/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/JarProvider.java @@ -17,142 +17,146 @@ */ package com.dscalzi.zipextractor.core.provider; - -import com.dscalzi.zipextractor.core.TaskInterruptedException; -import com.dscalzi.zipextractor.core.ZTask; -import com.dscalzi.zipextractor.core.managers.MessageManager; -import com.dscalzi.zipextractor.core.util.ICommandSender; - -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; -import java.io.IOException; -import java.nio.file.AccessDeniedException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.jar.JarEntry; -import java.util.jar.JarInputStream; -import java.util.regex.Pattern; -import java.util.zip.ZipException; - -public class JarProvider implements TypeProvider { - - // Shared pattern by JarProviders - public static final Pattern PATH_END = Pattern.compile("\\.jar$"); - protected static final List SUPPORTED = new ArrayList<>(Collections.singletonList("jar")); - - @Override - public List scanForExtractionConflicts(ICommandSender sender, File src, File dest, boolean silent) { - - List existing = new ArrayList<>(); - final MessageManager mm = MessageManager.inst(); - if(!silent) - mm.scanningForConflics(sender); - try (FileInputStream fis = new FileInputStream(src); JarInputStream jis = new JarInputStream(fis);) { - JarEntry je = jis.getNextJarEntry(); - - while (je != null) { - if (Thread.interrupted()) - throw new TaskInterruptedException(); - - File newFile = new File(dest + File.separator + je.getName()); - if (newFile.exists()) { - existing.add(je.getName()); - } - je = jis.getNextJarEntry(); - } - - jis.closeEntry(); - } catch (TaskInterruptedException e) { - mm.taskInterruption(sender, ZTask.EXTRACT); - } catch (IOException e) { - e.printStackTrace(); - } - - return existing; - } - - @Override - public boolean canDetectPipedConflicts() { - return false; - } - - @Override - public boolean extract(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { - final MessageManager mm = MessageManager.inst(); - byte[] buffer = new byte[1024]; - mm.startingProcess(sender, ZTask.EXTRACT, src.getName()); - try (FileInputStream fis = new FileInputStream(src); JarInputStream jis = new JarInputStream(fis);) { - JarEntry je = jis.getNextJarEntry(); - - while(je != null) { - if (Thread.interrupted()) - throw new TaskInterruptedException(); - - File newFile = new File(dest + File.separator + je.getName()); - if (log) - mm.info("Extracting : " + newFile.getAbsoluteFile()); - File parent = newFile.getParentFile(); - if (!parent.exists() && !parent.mkdirs()) { - throw new IllegalStateException("Couldn't create dir: " + parent); - } - if (je.isDirectory()) { - newFile.mkdir(); - je = jis.getNextJarEntry(); - continue; - } - try (FileOutputStream fos = new FileOutputStream(newFile)) { - int len; - while ((len = jis.read(buffer)) > 0) { - fos.write(buffer, 0, len); - } - } - je = jis.getNextJarEntry(); - } - jis.closeEntry(); - if(!pipe) - mm.extractionComplete(sender, dest); - return true; - } catch (AccessDeniedException e) { - mm.fileAccessDenied(sender, ZTask.EXTRACT, e.getMessage()); - return false; - } catch (ZipException e) { - mm.extractionFormatError(sender, src, "Jar"); - return false; - } catch (TaskInterruptedException e) { - mm.taskInterruption(sender, ZTask.EXTRACT); - return false; - } catch (IOException e) { - e.printStackTrace(); - mm.genericOperationError(sender, src, ZTask.EXTRACT); - return false; - } - } - - @Override - public boolean validForExtraction(File src) { - return PATH_END.matcher(src.getAbsolutePath()).find(); - } - - @Override - public boolean srcValidForCompression(File src) { - return false; // Compression to Jars is not supported. - } - - @Override - public boolean destValidForCompression(File dest) { - return false; // Compression to Jars is not supported. - } - - @Override - public List supportedExtractionTypes() { - return SUPPORTED; - } - - @Override - public List canCompressTo() { - return new ArrayList<>(); - } - -} + +import com.dscalzi.zipextractor.core.TaskInterruptedException; +import com.dscalzi.zipextractor.core.ZTask; +import com.dscalzi.zipextractor.core.managers.MessageManager; +import com.dscalzi.zipextractor.core.util.ICommandSender; + +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.jar.JarEntry; +import java.util.jar.JarInputStream; +import java.util.regex.Pattern; +import java.util.zip.ZipException; + +public class JarProvider implements TypeProvider { + + // Shared pattern by JarProviders + public static final Pattern PATH_END = Pattern.compile("\\.jar$"); + protected static final List SUPPORTED = new ArrayList<>(Collections.singletonList("jar")); + + @Override + public List scanForExtractionConflicts(ICommandSender sender, File src, File dest, boolean silent) { + + List existing = new ArrayList<>(); + final MessageManager mm = MessageManager.inst(); + if(!silent) + mm.scanningForConflics(sender); + try (FileInputStream fis = new FileInputStream(src); JarInputStream jis = new JarInputStream(fis);) { + JarEntry je = jis.getNextJarEntry(); + + while (je != null) { + if (Thread.interrupted()) + throw new TaskInterruptedException(); + + File newFile = new File(dest + File.separator + je.getName()); + if (newFile.exists()) { + existing.add(je.getName()); + } + je = jis.getNextJarEntry(); + } + + jis.closeEntry(); + } catch (TaskInterruptedException e) { + mm.taskInterruption(sender, ZTask.EXTRACT); + } catch (IOException e) { + e.printStackTrace(); + } + + return existing; + } + + @Override + public boolean canDetectPipedConflicts() { + return false; + } + + @Override + public boolean extract(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { + final MessageManager mm = MessageManager.inst(); + byte[] buffer = new byte[1024]; + mm.startingProcess(sender, ZTask.EXTRACT, src.getName()); + try (FileInputStream fis = new FileInputStream(src); JarInputStream jis = new JarInputStream(fis);) { + JarEntry je = jis.getNextJarEntry(); + + while(je != null) { + if (Thread.interrupted()) + throw new TaskInterruptedException(); + + File newFile = new File(dest, je.getName()); + + if (!newFile.toPath().normalize().startsWith(dest.toPath())) { + throw new RuntimeException("Bad zip entry"); + } + if (log) + mm.info("Extracting : " + newFile.getAbsoluteFile()); + File parent = newFile.getParentFile(); + if (!parent.exists() && !parent.mkdirs()) { + throw new IllegalStateException("Couldn't create dir: " + parent); + } + if (je.isDirectory()) { + newFile.mkdir(); + je = jis.getNextJarEntry(); + continue; + } + try (FileOutputStream fos = new FileOutputStream(newFile)) { + int len; + while ((len = jis.read(buffer)) > 0) { + fos.write(buffer, 0, len); + } + } + je = jis.getNextJarEntry(); + } + jis.closeEntry(); + if(!pipe) + mm.extractionComplete(sender, dest); + return true; + } catch (AccessDeniedException e) { + mm.fileAccessDenied(sender, ZTask.EXTRACT, e.getMessage()); + return false; + } catch (ZipException e) { + mm.extractionFormatError(sender, src, "Jar"); + return false; + } catch (TaskInterruptedException e) { + mm.taskInterruption(sender, ZTask.EXTRACT); + return false; + } catch (IOException e) { + e.printStackTrace(); + mm.genericOperationError(sender, src, ZTask.EXTRACT); + return false; + } + } + + @Override + public boolean validForExtraction(File src) { + return PATH_END.matcher(src.getAbsolutePath()).find(); + } + + @Override + public boolean srcValidForCompression(File src) { + return false; // Compression to Jars is not supported. + } + + @Override + public boolean destValidForCompression(File dest) { + return false; // Compression to Jars is not supported. + } + + @Override + public List supportedExtractionTypes() { + return SUPPORTED; + } + + @Override + public List canCompressTo() { + return new ArrayList<>(); + } + +} diff --git a/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/ZipProvider.java b/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/ZipProvider.java index c074818..4208405 100644 --- a/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/ZipProvider.java +++ b/ZipExtractor-Core/src/main/java/com/dscalzi/zipextractor/core/provider/ZipProvider.java @@ -17,185 +17,189 @@ */ package com.dscalzi.zipextractor.core.provider; - -import com.dscalzi.zipextractor.core.TaskInterruptedException; -import com.dscalzi.zipextractor.core.ZTask; -import com.dscalzi.zipextractor.core.managers.MessageManager; -import com.dscalzi.zipextractor.core.util.ICommandSender; - -import java.io.*; -import java.nio.file.AccessDeniedException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.regex.Pattern; -import java.util.stream.Stream; -import java.util.zip.ZipEntry; -import java.util.zip.ZipException; -import java.util.zip.ZipInputStream; -import java.util.zip.ZipOutputStream; - -public class ZipProvider implements TypeProvider { - - // Shared pattern by ZipProviders - public static final Pattern PATH_END = Pattern.compile("\\.zip$"); - protected static final List SUPPORTED = new ArrayList<>(Collections.singletonList("zip")); - - @Override - public List scanForExtractionConflicts(ICommandSender sender, File src, File dest, boolean silent) { - List existing = new ArrayList<>(); - final MessageManager mm = MessageManager.inst(); - if(!silent) - mm.scanningForConflics(sender); - try (FileInputStream fis = new FileInputStream(src); ZipInputStream zis = new ZipInputStream(fis);) { - ZipEntry ze = zis.getNextEntry(); - - while (ze != null) { - if (Thread.interrupted()) - throw new TaskInterruptedException(); - - File newFile = new File(dest + File.separator + ze.getName()); - if (newFile.exists()) { - existing.add(ze.getName()); - } - ze = zis.getNextEntry(); - } - - zis.closeEntry(); - } catch (TaskInterruptedException e) { - mm.taskInterruption(sender, ZTask.EXTRACT); - } catch (IOException e) { - e.printStackTrace(); - } - - return existing; - } - - @Override - public boolean canDetectPipedConflicts() { - return false; - } - - @Override - public boolean extract(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { - final MessageManager mm = MessageManager.inst(); - byte[] buffer = new byte[1024]; - mm.startingProcess(sender, ZTask.EXTRACT, src.getName()); - try (FileInputStream fis = new FileInputStream(src); ZipInputStream zis = new ZipInputStream(fis);) { - ZipEntry ze = zis.getNextEntry(); - - while (ze != null) { - if (Thread.interrupted()) - throw new TaskInterruptedException(); - - File newFile = new File(dest + File.separator + ze.getName()); - if (log) - mm.info("Extracting : " + newFile.getAbsoluteFile()); - File parent = newFile.getParentFile(); - if (!parent.exists() && !parent.mkdirs()) { - throw new IllegalStateException("Couldn't create dir: " + parent); - } - if (ze.isDirectory()) { - newFile.mkdir(); - ze = zis.getNextEntry(); - continue; - } - try (FileOutputStream fos = new FileOutputStream(newFile)) { - int len; - while ((len = zis.read(buffer)) > 0) { - fos.write(buffer, 0, len); - } - } - ze = zis.getNextEntry(); - } - zis.closeEntry(); - if(!pipe) - mm.extractionComplete(sender, dest); - return true; - } catch (AccessDeniedException e) { - mm.fileAccessDenied(sender, ZTask.EXTRACT, e.getMessage()); - return false; - } catch(ZipException e) { - mm.extractionFormatError(sender, src, "Zip"); - return false; - } catch (TaskInterruptedException e) { - mm.taskInterruption(sender, ZTask.EXTRACT); - return false; - } catch (IOException ex) { - ex.printStackTrace(); - mm.genericOperationError(sender, src, ZTask.EXTRACT); - return false; - } - } - - @Override - public boolean compress(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { - final MessageManager mm = MessageManager.inst(); - mm.startingProcess(sender, ZTask.COMPRESS, src.getName()); - try (OutputStream os = Files.newOutputStream(dest.toPath()); ZipOutputStream zs = new ZipOutputStream(os); Stream pathWalk = Files.walk(src.toPath())) { - Path pp = src.toPath(); - pathWalk.filter(path -> !path.toFile().isDirectory()).forEach(path -> { - if (Thread.interrupted()) - throw new TaskInterruptedException(); - // Prevent recursive compressions - if (path.equals(dest.toPath())) - return; - String sp = path.toAbsolutePath().toString().replace(pp.toAbsolutePath().toString(), ""); - if (sp.length() > 0) - sp = sp.substring(1); - ZipEntry zipEntry = new ZipEntry(pp.getFileName() + ((sp.length() > 0) ? (File.separator + sp) : "")); - try { - if (log) - mm.info("Compressing : " + zipEntry.toString()); - zs.putNextEntry(zipEntry); - zs.write(Files.readAllBytes(path)); - zs.closeEntry(); - } catch (Exception e) { - e.printStackTrace(); - } - }); - if(!pipe) - mm.compressionComplete(sender, dest); - return true; - } catch (AccessDeniedException e) { - mm.fileAccessDenied(sender, ZTask.COMPRESS, e.getMessage()); - return false; - } catch (TaskInterruptedException e) { - mm.taskInterruption(sender, ZTask.COMPRESS); - return false; - } catch (Throwable e) { - e.printStackTrace(); - mm.genericOperationError(sender, src, ZTask.COMPRESS); - return false; - } - } - - @Override - public boolean validForExtraction(File src) { - return PATH_END.matcher(src.getAbsolutePath()).find(); - } - - @Override - public boolean srcValidForCompression(File src) { - // Any source file can be compressed to a zip. - return true; - } - - @Override - public boolean destValidForCompression(File dest) { - return validForExtraction(dest); - } - - @Override - public List supportedExtractionTypes() { - return SUPPORTED; - } - - @Override - public List canCompressTo() { - return SUPPORTED; - } - -} + +import com.dscalzi.zipextractor.core.TaskInterruptedException; +import com.dscalzi.zipextractor.core.ZTask; +import com.dscalzi.zipextractor.core.managers.MessageManager; +import com.dscalzi.zipextractor.core.util.ICommandSender; + +import java.io.*; +import java.nio.file.AccessDeniedException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.regex.Pattern; +import java.util.stream.Stream; +import java.util.zip.ZipEntry; +import java.util.zip.ZipException; +import java.util.zip.ZipInputStream; +import java.util.zip.ZipOutputStream; + +public class ZipProvider implements TypeProvider { + + // Shared pattern by ZipProviders + public static final Pattern PATH_END = Pattern.compile("\\.zip$"); + protected static final List SUPPORTED = new ArrayList<>(Collections.singletonList("zip")); + + @Override + public List scanForExtractionConflicts(ICommandSender sender, File src, File dest, boolean silent) { + List existing = new ArrayList<>(); + final MessageManager mm = MessageManager.inst(); + if(!silent) + mm.scanningForConflics(sender); + try (FileInputStream fis = new FileInputStream(src); ZipInputStream zis = new ZipInputStream(fis);) { + ZipEntry ze = zis.getNextEntry(); + + while (ze != null) { + if (Thread.interrupted()) + throw new TaskInterruptedException(); + + File newFile = new File(dest + File.separator + ze.getName()); + if (newFile.exists()) { + existing.add(ze.getName()); + } + ze = zis.getNextEntry(); + } + + zis.closeEntry(); + } catch (TaskInterruptedException e) { + mm.taskInterruption(sender, ZTask.EXTRACT); + } catch (IOException e) { + e.printStackTrace(); + } + + return existing; + } + + @Override + public boolean canDetectPipedConflicts() { + return false; + } + + @Override + public boolean extract(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { + final MessageManager mm = MessageManager.inst(); + byte[] buffer = new byte[1024]; + mm.startingProcess(sender, ZTask.EXTRACT, src.getName()); + try (FileInputStream fis = new FileInputStream(src); ZipInputStream zis = new ZipInputStream(fis);) { + ZipEntry ze = zis.getNextEntry(); + + while (ze != null) { + if (Thread.interrupted()) + throw new TaskInterruptedException(); + + File newFile = new File(dest, ze.getName()); + + if (!newFile.toPath().normalize().startsWith(dest.toPath())) { + throw new RuntimeException("Bad zip entry"); + } + if (log) + mm.info("Extracting : " + newFile.getAbsoluteFile()); + File parent = newFile.getParentFile(); + if (!parent.exists() && !parent.mkdirs()) { + throw new IllegalStateException("Couldn't create dir: " + parent); + } + if (ze.isDirectory()) { + newFile.mkdir(); + ze = zis.getNextEntry(); + continue; + } + try (FileOutputStream fos = new FileOutputStream(newFile)) { + int len; + while ((len = zis.read(buffer)) > 0) { + fos.write(buffer, 0, len); + } + } + ze = zis.getNextEntry(); + } + zis.closeEntry(); + if(!pipe) + mm.extractionComplete(sender, dest); + return true; + } catch (AccessDeniedException e) { + mm.fileAccessDenied(sender, ZTask.EXTRACT, e.getMessage()); + return false; + } catch(ZipException e) { + mm.extractionFormatError(sender, src, "Zip"); + return false; + } catch (TaskInterruptedException e) { + mm.taskInterruption(sender, ZTask.EXTRACT); + return false; + } catch (IOException ex) { + ex.printStackTrace(); + mm.genericOperationError(sender, src, ZTask.EXTRACT); + return false; + } + } + + @Override + public boolean compress(ICommandSender sender, File src, File dest, boolean log, boolean pipe) { + final MessageManager mm = MessageManager.inst(); + mm.startingProcess(sender, ZTask.COMPRESS, src.getName()); + try (OutputStream os = Files.newOutputStream(dest.toPath()); ZipOutputStream zs = new ZipOutputStream(os); Stream pathWalk = Files.walk(src.toPath())) { + Path pp = src.toPath(); + pathWalk.filter(path -> !path.toFile().isDirectory()).forEach(path -> { + if (Thread.interrupted()) + throw new TaskInterruptedException(); + // Prevent recursive compressions + if (path.equals(dest.toPath())) + return; + String sp = path.toAbsolutePath().toString().replace(pp.toAbsolutePath().toString(), ""); + if (sp.length() > 0) + sp = sp.substring(1); + ZipEntry zipEntry = new ZipEntry(pp.getFileName() + ((sp.length() > 0) ? (File.separator + sp) : "")); + try { + if (log) + mm.info("Compressing : " + zipEntry.toString()); + zs.putNextEntry(zipEntry); + zs.write(Files.readAllBytes(path)); + zs.closeEntry(); + } catch (Exception e) { + e.printStackTrace(); + } + }); + if(!pipe) + mm.compressionComplete(sender, dest); + return true; + } catch (AccessDeniedException e) { + mm.fileAccessDenied(sender, ZTask.COMPRESS, e.getMessage()); + return false; + } catch (TaskInterruptedException e) { + mm.taskInterruption(sender, ZTask.COMPRESS); + return false; + } catch (Throwable e) { + e.printStackTrace(); + mm.genericOperationError(sender, src, ZTask.COMPRESS); + return false; + } + } + + @Override + public boolean validForExtraction(File src) { + return PATH_END.matcher(src.getAbsolutePath()).find(); + } + + @Override + public boolean srcValidForCompression(File src) { + // Any source file can be compressed to a zip. + return true; + } + + @Override + public boolean destValidForCompression(File dest) { + return validForExtraction(dest); + } + + @Override + public List supportedExtractionTypes() { + return SUPPORTED; + } + + @Override + public List canCompressTo() { + return SUPPORTED; + } + +}