From 5f2bbf24f12b04670e991c5b2a046e7b1b3b80cb Mon Sep 17 00:00:00 2001 From: Emanuela Epure <67077116+emanuelaepure10@users.noreply.github.com> Date: Wed, 26 Jun 2024 14:49:59 +0200 Subject: [PATCH] fix: avoid arbitrary file access during archive extraction ("Zip Slip") Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations. ING-4369 --- .../io/project/impl/ArchiveProjectImport.java | 62 +++++---- .../Servicemerger/src/Servicemerger.java | 124 +++++++++--------- .../util/reflection/ReflectionHelper.java | 48 ++++--- 3 files changed, 122 insertions(+), 112 deletions(-) diff --git a/common/plugins/eu.esdihumboldt.hale.common.core/src/eu/esdihumboldt/hale/common/core/io/project/impl/ArchiveProjectImport.java b/common/plugins/eu.esdihumboldt.hale.common.core/src/eu/esdihumboldt/hale/common/core/io/project/impl/ArchiveProjectImport.java index 44c337ca7b..9c2fe78840 100644 --- a/common/plugins/eu.esdihumboldt.hale.common.core/src/eu/esdihumboldt/hale/common/core/io/project/impl/ArchiveProjectImport.java +++ b/common/plugins/eu.esdihumboldt.hale.common.core/src/eu/esdihumboldt/hale/common/core/io/project/impl/ArchiveProjectImport.java @@ -87,42 +87,48 @@ public URI getProjectLocation() { // destination file has to be a directory private void getZipFiles(InputStream file, File destination) throws IOException { byte[] buf = new byte[1024]; - ZipInputStream zipinputstream = null; - ZipEntry zipentry; - zipinputstream = new ZipInputStream(file); - - try { - while ((zipentry = zipinputstream.getNextEntry()) != null) { - String entryName = zipentry.getName(); - int n; - FileOutputStream fileoutputstream; - File newFile = new File(entryName); - String directory = newFile.getParent(); - - if (directory != null) { - File newF = new File(destination, directory); - try { - newF.mkdirs(); - } catch (SecurityException e) { - log.debug("Can not create directories because of SecurityManager", e); - } - } + try (ZipInputStream zipInputStream = new ZipInputStream(file)) { + ZipEntry zipEntry; - File nf = new File(destination, entryName); - fileoutputstream = new FileOutputStream(nf); + while ((zipEntry = zipInputStream.getNextEntry()) != null) { + String entryName = zipEntry.getName(); - while ((n = zipinputstream.read(buf, 0, 1024)) > -1) - fileoutputstream.write(buf, 0, n); + // Normalize the entry name and ensure it doesn't point outside + // the destination directory + File newFile = new File(destination, entryName).getCanonicalFile(); - fileoutputstream.close(); - zipinputstream.closeEntry(); + if (!newFile.getPath() + .startsWith(destination.getCanonicalPath() + File.separator)) { + throw new IOException( + "Entry is outside of the target dir: " + zipEntry.getName()); + } + if (zipEntry.isDirectory()) { + if (!newFile.isDirectory() && !newFile.mkdirs()) { + throw new IOException("Failed to create directory " + newFile); + } + } + else { + // Ensure parent directories are created + File parent = newFile.getParentFile(); + if (!parent.isDirectory() && !parent.mkdirs()) { + throw new IOException("Failed to create directory " + parent); + } + + // Write the file content + try (FileOutputStream fos = new FileOutputStream(newFile)) { + int len; + while ((len = zipInputStream.read(buf)) > 0) { + fos.write(buf, 0, len); + } + } + } + + zipInputStream.closeEntry(); } } catch (FileNotFoundException e) { throw new IOException("Destination directory not found", e); } - - zipinputstream.close(); } @Override diff --git a/platform/tools/Servicemerger/src/Servicemerger.java b/platform/tools/Servicemerger/src/Servicemerger.java index b925f47aa2..31d1d13d50 100644 --- a/platform/tools/Servicemerger/src/Servicemerger.java +++ b/platform/tools/Servicemerger/src/Servicemerger.java @@ -281,72 +281,66 @@ private void writeMergedFile(File file, String text) throws IOException{ * @param dir the directory of the jar files * @throws IOException */ - private void merge(File dir) throws IOException{ - - File[] files = dir.listFiles(); - Map visited = new HashMap(); - //are jar files in the given directory? - if (files != null) { - for (int i = 0; i < files.length; i++) { - - //we only work with .jar files - JarFile jar = null; - - if(this.bndFlag == false){ - if(files[i].getName().endsWith(".jar")){ - jar = new JarFile(files[i]); - } - } - - else if(this.bndFlag == true){ - if(files[i].getName().endsWith(".jar") && bndSpecifications.contains(files[i].getName())){ - jar = new JarFile(files[i]); - } - } - - if(jar != null){ - - Enumeration jarenu = jar.entries(); - //lets look at the entry of a specific jar file - while (jarenu.hasMoreElements()){ - - JarEntry entry = jarenu.nextElement(); - //do the jar file contains a servicefile? - if(entry.getName().contains("META-INF") - && entry.getName().contains("services") - && !entry.isDirectory()){ - - if(visited.containsKey(entry.getName())){ - - File mergedFile = visited.get(entry.getName()); - writeMergedFile(mergedFile, readFile(jar.getInputStream(entry))); - - } - - else { - File mergedFile = new File(dest + java.io.File.separator + entry.getName()); - writeMergedFile(mergedFile, readFile(jar.getInputStream(entry))); - visited.put(entry.getName(), mergedFile); - } - - } - - - - } - - } - - - - - - } - } + private void merge(File dir) throws IOException { + File[] files = dir.listFiles(); + Map visited = new HashMap<>(); + + // Are jar files in the given directory? + if (files != null) { + for (File file : files) { + if (file.getName().endsWith(".jar") && shouldProcessJar(file)) { + try (JarFile jar = new JarFile(file)) { + processJarFile(jar, visited); + } + } + } + } + } + + private boolean shouldProcessJar(File file) { + return !this.bndFlag || bndSpecifications.contains(file.getName()); + } + + private void processJarFile(JarFile jar, Map visited) throws IOException { + Enumeration jarEntries = jar.entries(); + while (jarEntries.hasMoreElements()) { + JarEntry entry = jarEntries.nextElement(); + if (isServiceFile(entry)) { + File mergedFile = visited.get(entry.getName()); + if (mergedFile == null) { + mergedFile = new File(dest, entry.getName()); + if (isInsideDestinationDirectory(mergedFile)) { + visited.put(entry.getName(), mergedFile); + } else { + throw new IOException("Entry is outside of the target dir: " + entry.getName()); + } + } + writeMergedFile(mergedFile, readJarFile(jar.getInputStream(entry))); + } + } + } + + private boolean isServiceFile(JarEntry entry) { + return entry.getName().contains("META-INF") && entry.getName().contains("services") && !entry.isDirectory(); + } + + private boolean isInsideDestinationDirectory(File file) throws IOException { + Path destDirPath = Paths.get(dest).toAbsolutePath(); + Path filePath = file.toPath().toAbsolutePath(); + return filePath.startsWith(destDirPath); + } + + private byte[] readJarFile(InputStream inputStream) throws IOException { + return inputStream.readAllBytes(); + } + + private void writeMergedFile(File file, byte[] content) throws IOException { + // Append content to the file, creating directories if necessary + if (!file.getParentFile().exists()) { + file.getParentFile().mkdirs(); + } + Files.write(file.toPath(), content, java.nio.file.StandardOpenOption.CREATE, java.nio.file.StandardOpenOption.APPEND); } - - - /** * main method to start the Servicemerger diff --git a/util/plugins/eu.esdihumboldt.util/src/eu/esdihumboldt/util/reflection/ReflectionHelper.java b/util/plugins/eu.esdihumboldt.util/src/eu/esdihumboldt/util/reflection/ReflectionHelper.java index e9f3254ff4..accfd555d4 100644 --- a/util/plugins/eu.esdihumboldt.util/src/eu/esdihumboldt/util/reflection/ReflectionHelper.java +++ b/util/plugins/eu.esdihumboldt.util/src/eu/esdihumboldt/util/reflection/ReflectionHelper.java @@ -74,10 +74,9 @@ public static Method findSetter(Class c, String property, Class propertyTy Method result = null; Method[] methods = c.getMethods(); for (Method m : methods) { - if (m.getName().equals(setterName) - && m.getParameterTypes().length == 1 - && (propertyType == null || m.getParameterTypes()[0] - .isAssignableFrom(propertyType))) { + if (m.getName().equals(setterName) && m.getParameterTypes().length == 1 + && (propertyType == null + || m.getParameterTypes()[0].isAssignableFrom(propertyType))) { result = m; break; } @@ -109,10 +108,9 @@ public static Method findDeepSetter(Class c, String property, Class proper // search visible and hidden methods in this class Method[] methods = c.getDeclaredMethods(); for (Method m : methods) { - if (m.getName().equals(setterName) - && m.getParameterTypes().length == 1 - && (propertyType == null || m.getParameterTypes()[0] - .isAssignableFrom(propertyType))) { + if (m.getName().equals(setterName) && m.getParameterTypes().length == 1 + && (propertyType == null + || m.getParameterTypes()[0].isAssignableFrom(propertyType))) { result = m; break; } @@ -355,8 +353,8 @@ public static void setDeepProperty(Object bean, String propertyName, Object valu * @throws InvocationTargetException if the getter throws an exception */ @SuppressWarnings("unchecked") - private static T invokeGetter(Object bean, Method getter) throws IllegalArgumentException, - IllegalAccessException, InvocationTargetException { + private static T invokeGetter(Object bean, Method getter) + throws IllegalArgumentException, IllegalAccessException, InvocationTargetException { boolean accessible = getter.isAccessible(); getter.setAccessible(true); T result = null; @@ -432,8 +430,8 @@ public static T getDeepProperty(Object bean, String propertyName) * @throws IllegalStateException is the field could be found or accessed */ @SuppressWarnings("unchecked") - public static T getDeepPropertyOrField(Object bean, String propertyName, Class valueClass) - throws IllegalArgumentException, InvocationTargetException { + public static T getDeepPropertyOrField(Object bean, String propertyName, + Class valueClass) throws IllegalArgumentException, InvocationTargetException { try { return ReflectionHelper.getDeepProperty(bean, propertyName); } catch (NoSuchMethodException e) {/* ignore */ @@ -443,8 +441,8 @@ public static T getDeepPropertyOrField(Object bean, String propertyName, Cla // there is no getter for the property. try to get the field directly Field f = findDeepField(bean.getClass(), propertyName, valueClass); if (f == null) { - throw new IllegalStateException("Could not find " + "field for property " - + propertyName + " in class " + bean.getClass().getCanonicalName()); + throw new IllegalStateException("Could not find " + "field for property " + propertyName + + " in class " + bean.getClass().getCanonicalName()); } boolean access = f.isAccessible(); @@ -562,12 +560,24 @@ public static synchronized File[] getFilesFromPackage(String pkg) throws IOExcep } while (entries.hasMoreElements()) { JarEntry j = entries.nextElement(); - if (j.getName().matches("^" + package_path + ".+\\..+")) { //$NON-NLS-1$ //$NON-NLS-2$ + String entryName = j.getName(); + + // Ensure the entry name is properly normalized and checked + File targetFile = new File(package_path, entryName); + String canonicalTargetPath = targetFile.getCanonicalPath(); + String canonicalBasePath = new File(package_path).getCanonicalPath(); + + if (!canonicalTargetPath.startsWith(canonicalBasePath + File.separator)) { + throw new IOException( + "Entry is outside of the target directory: " + entryName); + } + + if (entryName.matches("^" + package_path + ".+\\..+")) { //$NON-NLS-1$ //$NON-NLS-2$ if (slashed) { - file_names.add("/" + j.getName()); //$NON-NLS-1$ + file_names.add("/" + entryName); //$NON-NLS-1$ } else { - file_names.add(j.getName()); + file_names.add(entryName); } } } @@ -664,8 +674,8 @@ private static void getSubPackagesFromPackage(String pkg, List l, boolea } } - private static void getClassesFromPackage(String pkg, List> l, - ClassLoader classLoader, boolean recursive) throws IOException { + private static void getClassesFromPackage(String pkg, List> l, ClassLoader classLoader, + boolean recursive) throws IOException { File[] files = getFilesFromPackage(pkg); for (File f : files) { String name = f.getName();