From cc0c6c8e765704bfad9be10bb7964e1058ce5a88 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 14:56:18 +0300 Subject: [PATCH 1/2] Refuse archive entries that escape their target directory and drop dead ECIES code Entries of connector bundles (lib/*, native/*), of IOUtil.unjar and of the maven plugin's own resources were resolved with new File(dir, name), so an entry such as lib/../../x was written outside of the target directory (CodeQL java/zipslip). IOUtil.resolveEntry now rejects any entry whose canonical path leaves the directory, and the four extraction sites use it. ECIESEncryptor (AES/CBC with an IV taken from the ECDH secret) was only reachable through OpenICFServerAdapter.initialiseEncryptor(), which nothing calls and which dereferences a null HandshakeMessage; both are removed. Also widens the contract tests' loop counters to long to match the long MAX_ITERATIONS parameter, and makes the ObjectPool wait loop's exits explicit - it only ever left by returning or throwing. --- .../test/AuthenticationApiOpTests.java | 9 +- .../local/LocalConnectorInfoManagerImpl.java | 4 +- .../framework/impl/api/local/ObjectPool.java | 6 +- .../api/LocalConnectorInfoManagerTests.java | 53 ++++++++++++ .../remote/OpenICFServerAdapter.java | 14 ---- .../remote/security/ECIESEncryptor.java | 82 ------------------- .../framework/remote/SecurityUtilTest.java | 16 +--- .../org/identityconnectors/common/IOUtil.java | 26 +++++- .../common/IOUtilsTests.java | 58 +++++++++++++ .../maven/ConnectorInfoReportMojo.java | 4 +- .../openicf/maven/DocBookResourceMojo.java | 4 +- 11 files changed, 155 insertions(+), 121 deletions(-) delete mode 100644 OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/security/ECIESEncryptor.java diff --git a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/AuthenticationApiOpTests.java b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/AuthenticationApiOpTests.java index 0d6b0c7c..e0a502fd 100644 --- a/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/AuthenticationApiOpTests.java +++ b/OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/AuthenticationApiOpTests.java @@ -21,6 +21,7 @@ * ==================== * * Portions Copyrighted 2012 ForgeRock AS + * Portions Copyrighted 2026 3A Systems, LLC * */ package org.identityconnectors.contract.test; @@ -526,7 +527,7 @@ private void sleepIngoringInterruption(long sleepTime) { private boolean authenticateExpectingRuntimeException(ObjectClass objectClass, String name, GuardedString password) { boolean authenticateFailed = false; - for(int i=0;i 0); + } } finally { lock.unlock(); } diff --git a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java index 881c07d2..e5a8467a 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java +++ b/OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/LocalConnectorInfoManagerTests.java @@ -19,14 +19,22 @@ * enclosed by brackets [] replaced by your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" * ==================== + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.framework.impl.api; +import java.io.File; +import java.io.FileOutputStream; import java.net.URL; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.jar.Attributes; +import java.util.jar.JarEntry; +import java.util.jar.JarOutputStream; +import java.util.jar.Manifest; import org.identityconnectors.common.Version; import org.identityconnectors.common.logging.Log; @@ -86,6 +94,51 @@ public void testCheckVersion() throws Exception { } } + /** + * A bundle entry such as {@code lib/../../x.jar} must not be written + * outside of the bundle's temporary directory (zip slip). + */ + @Test + public void testRejectsBundleEntryEscapingTempDirectory() throws Exception { + // bundles are expanded into java.io.tmpdir/bundle-/, so the + // entry below points two levels up, straight into java.io.tmpdir + String escapedName = "escaped-" + UUID.randomUUID() + ".jar"; + File escaped = new File(System.getProperty("java.io.tmpdir"), escapedName); + + File bundle = File.createTempFile("evil-bundle", ".jar"); + Manifest manifest = new Manifest(); + manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0"); + manifest.getMainAttributes().putValue("ConnectorBundle-FrameworkVersion", "1.0"); + manifest.getMainAttributes().putValue("ConnectorBundle-Name", "evil"); + manifest.getMainAttributes().putValue("ConnectorBundle-Version", "1.0"); + JarOutputStream out = new JarOutputStream(new FileOutputStream(bundle), manifest); + try { + // a regular entry first, so that lib/ exists when the escaping + // entry is expanded and lib/../.. resolves + out.putNextEntry(new JarEntry("lib/ok.jar")); + out.write(new byte[] { 0 }); + out.closeEntry(); + out.putNextEntry(new JarEntry("lib/../../" + escapedName)); + out.write(new byte[] { 0 }); + out.closeEntry(); + } finally { + out.close(); + } + try { + ConnectorInfoManagerFactory.getInstance().getLocalManager(bundle.toURI().toURL()); + Assert.fail("Expected the bundle to be refused"); + } catch (ConfigurationException expected) { + assertFalse(escaped.exists(), "bundle entry written outside of its temp directory: " + + escaped); + } finally { + // nothing to evict: a bundle that fails to load is never cached, + // and clearing the local cache here would leave pooled connector + // instances of the other tests behind with a stale class loader + escaped.delete(); + bundle.delete(); + } + } + /** * To be overridden by subclasses to get different ConnectorInfoManagers * diff --git a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/OpenICFServerAdapter.java b/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/OpenICFServerAdapter.java index 3c3a5ea7..dc7811fa 100644 --- a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/OpenICFServerAdapter.java +++ b/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/OpenICFServerAdapter.java @@ -17,7 +17,6 @@ package org.forgerock.openicf.framework.remote; import java.security.KeyPair; -import java.security.PublicKey; import java.util.ArrayList; import java.util.List; import java.util.Locale; @@ -56,11 +55,9 @@ import org.forgerock.openicf.framework.async.impl.ValidateAsyncApiOpImpl; import org.forgerock.openicf.framework.remote.rpc.OperationMessageListener; import org.forgerock.openicf.framework.remote.rpc.WebSocketConnectionHolder; -import org.forgerock.openicf.framework.remote.security.ECIESEncryptor; import org.identityconnectors.common.Assertions; import org.identityconnectors.common.l10n.CurrentLocale; import org.identityconnectors.common.logging.Log; -import org.identityconnectors.common.security.Encryptor; import org.identityconnectors.framework.api.ConfigurationProperty; import org.identityconnectors.framework.api.ConfigurationPropertyChangeListener; import org.identityconnectors.framework.api.ConnectorFacade; @@ -491,17 +488,6 @@ public void processCancelOpRequest(final WebSocketConnectionHolder socket, long messageId); } - protected Encryptor initialiseEncryptor() { - HandshakeMessage message = null; - // Create Encryptor - if (!message.getPublicKey().isEmpty()) { - PublicKey publicKey = - SecurityUtil.createPublicKey(message.getPublicKey().toByteArray()); - Encryptor encryptor = new ECIESEncryptor(keyPair, publicKey); - } - return null; - } - protected String loggerName() { return isClient() ? "Client" : "Server"; } diff --git a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/security/ECIESEncryptor.java b/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/security/ECIESEncryptor.java deleted file mode 100644 index 463240eb..00000000 --- a/OpenICF-java-framework/connector-framework-server/src/main/java/org/forgerock/openicf/framework/remote/security/ECIESEncryptor.java +++ /dev/null @@ -1,82 +0,0 @@ -/* - * ==================== - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER. - * - * Copyright 2008-2009 Sun Microsystems, Inc. All rights reserved. - * - * The contents of this file are subject to the terms of the Common Development - * and Distribution License("CDDL") (the "License"). You may not use this file - * except in compliance with the License. - * - * You can obtain a copy of the License at - * http://opensource.org/licenses/cddl1.php - * See the License for the specific language governing permissions and limitations - * under the License. - * - * When distributing the Covered Code, include this CDDL Header Notice in each file - * and include the License file at http://opensource.org/licenses/cddl1.php. - * If applicable, add the following below this CDDL Header, with the fields - * enclosed by brackets [] replaced by your own identifying information: - * "Portions Copyrighted [year] [name of copyright owner]" - * ==================== - * Portions Copyrighted 2016 ForgeRock AS. - */ - -package org.forgerock.openicf.framework.remote.security; - -import java.security.Key; -import java.security.KeyPair; -import java.security.PublicKey; - -import org.forgerock.openicf.framework.remote.SecurityUtil; -import org.identityconnectors.common.security.Encryptor; - -import javax.crypto.Cipher; -import javax.crypto.spec.IvParameterSpec; -import javax.crypto.spec.SecretKeySpec; - -public class ECIESEncryptor implements Encryptor { - - public static final String ALGORITHM = "AES"; - public static final String FULL_ALGORITHM = "AES/CBC/PKCS5Padding"; - - private final Key key; - private final IvParameterSpec iv; - - public ECIESEncryptor(KeyPair privateKey, PublicKey publicKey) { - byte[] bytes = SecurityUtil.doECDH(privateKey, publicKey); - byte[] secret = new byte[16]; - byte[] vector = new byte[16]; - System.arraycopy(bytes,0, secret, 0, 16); - System.arraycopy(bytes,16, vector, 0, 16); - - key = new SecretKeySpec(secret, ALGORITHM); - iv = new IvParameterSpec(vector); - } - - @Override - public byte[] decrypt(byte[] bytes) { - try { - Cipher cipher = Cipher.getInstance(FULL_ALGORITHM); - cipher.init(Cipher.DECRYPT_MODE, key, iv); - return cipher.doFinal(bytes); - } catch (RuntimeException e) { - throw e; - } catch (Exception e) { - throw new RuntimeException(e); - } - } - - @Override - public byte[] encrypt(byte[] bytes) { - try { - Cipher cipher = Cipher.getInstance(FULL_ALGORITHM); - cipher.init(Cipher.ENCRYPT_MODE, key, iv); - return cipher.doFinal(bytes); - } catch (RuntimeException e) { - throw e; - } catch (Exception e) { - throw new RuntimeException(e); - } - } -} diff --git a/OpenICF-java-framework/connector-framework-server/src/test/java/org/forgerock/openicf/framework/remote/SecurityUtilTest.java b/OpenICF-java-framework/connector-framework-server/src/test/java/org/forgerock/openicf/framework/remote/SecurityUtilTest.java index c41eaa29..5451363d 100644 --- a/OpenICF-java-framework/connector-framework-server/src/test/java/org/forgerock/openicf/framework/remote/SecurityUtilTest.java +++ b/OpenICF-java-framework/connector-framework-server/src/test/java/org/forgerock/openicf/framework/remote/SecurityUtilTest.java @@ -20,33 +20,19 @@ * with the fields enclosed by brackets [] replaced by * your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" + * Portions Copyrighted 2026 3A Systems, LLC */ package org.forgerock.openicf.framework.remote; -import org.forgerock.openicf.framework.remote.security.ECIESEncryptor; import org.identityconnectors.common.Pair; import org.testng.Assert; import org.testng.annotations.Test; -import java.security.KeyPair; import java.security.SecureRandom; public class SecurityUtilTest { - @Test - public void testECIESEncryptor() throws Exception { - KeyPair client = SecurityUtil.generateKeyPair(); - KeyPair server = SecurityUtil.generateKeyPair(); - - ECIESEncryptor clientEncryptor = new ECIESEncryptor(client, server.getPublic()); - ECIESEncryptor serverEncryptor = new ECIESEncryptor(server, client.getPublic()); - - byte[] expected = "password".getBytes(); - byte[] secure = clientEncryptor.encrypt(expected); - Assert.assertEquals(serverEncryptor.decrypt(secure), expected); - } - @Test public void testCheckMutualVerification() throws Exception { SecureRandom random = new SecureRandom(); diff --git a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java index 68a1a3d9..2555018e 100644 --- a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java +++ b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java @@ -617,6 +617,30 @@ public static void extractResourceToFile(final Class clazz, final String path } } + /** + * Resolves an archive entry name against a directory, refusing names that + * would land outside of it, such as {@code ../etc/passwd} (zip slip). + * + * @param dir + * The directory the entry is extracted into. + * @param entryName + * The entry name as recorded in the archive. + * @return The file the entry maps to: {@code dir} itself or a file below + * it. + * @throws IOException + * If the entry would escape {@code dir}. + */ + public static File resolveEntry(final File dir, final String entryName) throws IOException { + final File file = new File(dir, entryName); + final String dirPath = dir.getCanonicalPath(); + final String filePath = file.getCanonicalPath(); + final String prefix = dirPath.endsWith(File.separator) ? dirPath : dirPath + File.separator; + if (!filePath.equals(dirPath) && !filePath.startsWith(prefix)) { + throw new IOException("Archive entry " + entryName + " is outside of " + dir); + } + return file; + } + /** * Unjars the given file to the given directory. Does not close the JarFile * when finished. @@ -630,7 +654,7 @@ public static void unjar(final JarFile jarFile, final File toDir) throws IOExcep final Enumeration entries = jarFile.entries(); while (entries.hasMoreElements()) { final JarEntry entry = entries.nextElement(); - final File outFile = new File(toDir, entry.getName()); + final File outFile = resolveEntry(toDir, entry.getName()); FileOutputStream fos = null; try { fos = new FileOutputStream(outFile); diff --git a/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/IOUtilsTests.java b/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/IOUtilsTests.java index 2e9eac7d..20ca4c17 100644 --- a/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/IOUtilsTests.java +++ b/OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/IOUtilsTests.java @@ -19,16 +19,26 @@ * enclosed by brackets [] replaced by your own identifying information: * "Portions Copyrighted [year] [name of copyright owner]" * ==================== + * Portions Copyrighted 2026 3A Systems, LLC */ package org.identityconnectors.common; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; +import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.io.Reader; import java.io.Writer; +import java.nio.file.Files; +import java.util.jar.JarEntry; +import java.util.jar.JarFile; +import java.util.jar.JarOutputStream; import org.testng.annotations.Test; @@ -62,6 +72,54 @@ public void resourcePath() { // test resource path returns the right thing.. } + @Test + public void resolveEntryReturnsFileInsideDirectory() throws IOException { + File dir = Files.createTempDirectory("IOUtilsTests").toFile(); + assertEquals(IOUtil.resolveEntry(dir, "lib/a.jar").getCanonicalFile(), new File(dir, + "lib/a.jar").getCanonicalFile()); + } + + @Test + public void resolveEntryAcceptsTheDirectoryItself() throws IOException { + // the directory entry of an archive root, e.g. "shared/" once its + // prefix is stripped, maps to the directory itself + File dir = Files.createTempDirectory("IOUtilsTests").toFile(); + assertEquals(IOUtil.resolveEntry(dir, "").getCanonicalFile(), dir.getCanonicalFile()); + } + + @Test(expectedExceptions = IOException.class) + public void resolveEntryRejectsEntryEscapingDirectory() throws IOException { + File dir = Files.createTempDirectory("IOUtilsTests").toFile(); + IOUtil.resolveEntry(dir, "lib/../../escaped.jar"); + } + + @Test + public void unjarRefusesEntryEscapingTargetDirectory() throws IOException { + File root = Files.createTempDirectory("IOUtilsTests").toFile(); + File toDir = new File(root, "out"); + assertTrue(toDir.mkdir()); + File jar = new File(root, "evil.jar"); + JarOutputStream out = new JarOutputStream(new FileOutputStream(jar)); + try { + out.putNextEntry(new JarEntry("../escaped.txt")); + out.write("escaped".getBytes(UTF8_NAME)); + out.closeEntry(); + } finally { + out.close(); + } + JarFile jarFile = new JarFile(jar); + try { + IOUtil.unjar(jarFile, toDir); + fail("Expected the entry escaping " + toDir + " to be refused"); + } catch (IOException expected) { + assertFalse(new File(root, "escaped.txt").exists(), "entry written outside " + toDir); + } finally { + jarFile.close(); + } + } + + private static final String UTF8_NAME = "UTF-8"; + // public static String getResourcePath(Class c, String res) { // public static InputStream getResourceAsStream(Class clazz, String res) // { diff --git a/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/ConnectorInfoReportMojo.java b/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/ConnectorInfoReportMojo.java index e306b8f9..62ff4eda 100644 --- a/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/ConnectorInfoReportMojo.java +++ b/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/ConnectorInfoReportMojo.java @@ -343,7 +343,9 @@ protected void executeReport(Locale locale) throws MavenReportException { String name = entry.getName(); if (name.startsWith("shared")) { - File destination = new File(outputDirectory, name.substring(7)); + File destination = + org.identityconnectors.common.IOUtil.resolveEntry( + outputDirectory, name.substring(7)); if (entry.isDirectory()) { if (!destination.exists()) { destination.mkdirs(); diff --git a/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/DocBookResourceMojo.java b/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/DocBookResourceMojo.java index d7f527cd..4e258c80 100644 --- a/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/DocBookResourceMojo.java +++ b/OpenICF-maven-plugin/src/main/java/org/forgerock/openicf/maven/DocBookResourceMojo.java @@ -414,7 +414,9 @@ public boolean accept(File pathname) { String name = entry.getName(); if (entry.getName().startsWith("shared")) { - File destination = new File(sharedRoot, name); + File destination = + org.identityconnectors.common.IOUtil.resolveEntry( + sharedRoot, name); if (entry.isDirectory()) { if (!destination.exists()) { destination.mkdirs(); From 3febbbe23c92a6b59b1fc12575eb82d1a8abc83f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 16:46:22 +0300 Subject: [PATCH 2/2] Shape the archive entry check so that CodeQL recognises it as a sanitizer The check compared the canonical path with "equals the directory or starts with its prefix"; CodeQL's path-injection sanitizer only credits a lone startsWith on a normalised path that guards the use. A trailing separator on both sides makes one startsWith cover the directory itself as well. resolveEntry now returns the canonical file, so the bundle temp directory is canonicalised too before its parent directories are walked. --- .../api/local/LocalConnectorInfoManagerImpl.java | 5 ++++- .../org/identityconnectors/common/IOUtil.java | 16 +++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java index c1e8968b..18c5701d 100644 --- a/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java +++ b/OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/LocalConnectorInfoManagerImpl.java @@ -455,7 +455,10 @@ public File copyStreamToFile(final InputStream stream) throws IOException { public File copyStreamToFile(final InputStream stream, final String name) throws IOException { - final File bundleDir = getBundleTempDir(); + // canonical, like the file resolveEntry returns, so that the + // parent walk below ends at bundleDir even when java.io.tmpdir + // goes through a symbolic link + final File bundleDir = getBundleTempDir().getCanonicalFile(); // refuses entries such as lib/../../x that would leave bundleDir final File newFile = IOUtil.resolveEntry(bundleDir, name); if (newFile.exists()) { diff --git a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java index 2555018e..9a992171 100644 --- a/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java +++ b/OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/IOUtil.java @@ -625,17 +625,19 @@ public static void extractResourceToFile(final Class clazz, final String path * The directory the entry is extracted into. * @param entryName * The entry name as recorded in the archive. - * @return The file the entry maps to: {@code dir} itself or a file below - * it. + * @return The canonical file the entry maps to: {@code dir} itself or a + * file below it. * @throws IOException * If the entry would escape {@code dir}. */ public static File resolveEntry(final File dir, final String entryName) throws IOException { - final File file = new File(dir, entryName); - final String dirPath = dir.getCanonicalPath(); - final String filePath = file.getCanonicalPath(); - final String prefix = dirPath.endsWith(File.separator) ? dirPath : dirPath + File.separator; - if (!filePath.equals(dirPath) && !filePath.startsWith(prefix)) { + final File root = dir.getCanonicalFile(); + final File file = new File(root, entryName).getCanonicalFile(); + final String rootPath = root.getPath(); + final String prefix = rootPath.endsWith(File.separator) ? rootPath : rootPath + File.separator; + // the trailing separator lets the directory itself pass ("dir/") + // and keeps a sibling such as "dir2" out + if (!(file.getPath() + File.separator).startsWith(prefix)) { throw new IOException("Archive entry " + entryName + " is outside of " + dir); } return file;