Merge commit from fork

Perform the IJackcessOpenerInterface type check before calling
getConstructor().newInstance() when resolving a custom jackcessOpener
class. Previously the class was instantiated first and checked
afterwards, so the no-arg constructor of any named class would run
even if it didn't implement the required interface.

Also document that jackcessOpener must only be set from trusted
configuration, and add regression tests.

Refs: GHSA-5898-r52v-fprf
Reported-by: Fushuling (https://github.com/Fushuling)

Author: spannm

src/main/java/net/ucanaccess/converters/Metadata.java

@@ -48,6 +48,18 @@ public enum Property {
         ignoreCase(Boolean.class, true, 10),
         immediatelyReleaseResources(Boolean.class, false, 10),
         inactivityTimeout(Integer.class, 2, 10),
+        /**
+         * Fully qualified class name of a custom {@code IJackcessOpenerInterface} implementation
+         * to use for opening/decrypting the database file.
+         * <p>
+         * <strong>Security note:</strong> the value of this property is instantiated via reflection
+         * using its public no-arg constructor. It must therefore only ever be set from trusted,
+         * static application configuration. Never populate this property from untrusted or
+         * user-supplied input (e.g. HTTP request parameters, values forwarded unchecked from an
+         * external system), as doing so allows an attacker to trigger the construction of an
+         * arbitrary class implementing {@code IJackcessOpenerInterface} that is reachable on the
+         * application's classpath.
+         */
         jackcessOpener(String.class, null, 500),
         keepMirror(String.class, null, 500),
         lobScale(Integer.class, 2, 2, 1, 2, 4, 8, 16, 32),

src/main/java/net/ucanaccess/jdbc/UcanaccessDriver.java

@@ -280,13 +280,31 @@ public boolean jdbcCompliant() {
         return true;
     }
 
+    /**
+     * Loads the class with the given name and instantiates it via its public no-arg constructor,
+     * provided it implements {@link IJackcessOpenerInterface}.
+     * <p>
+     * <strong>Security note:</strong> {@code className} must only ever originate from trusted,
+     * static application configuration (see the {@code jackcessOpener} connection property).
+     * It must never be built from untrusted or user-supplied input, since this method executes
+     * the no-arg constructor of whatever class is named, and any class implementing
+     * {@link IJackcessOpenerInterface} that is reachable on the classpath will pass the type
+     * check regardless of what its constructor does.
+     *
+     * @param className fully qualified name of a class implementing {@link IJackcessOpenerInterface}
+     * @return a new instance of the given class
+     * @throws UcanaccessSQLException if the class cannot be loaded, does not implement
+     *         {@link IJackcessOpenerInterface}, or cannot be instantiated via its no-arg constructor
+     */
     private IJackcessOpenerInterface newJackcessOpenerInstance(String className) throws UcanaccessSQLException {
-        Object instance = Try.catching(() -> Class.forName(className).getConstructor().newInstance()).orThrow(ex -> new UcanaccessSQLException("Failed to instantiate " + className, ex));
-
-        if (instance instanceof IJackcessOpenerInterface) {
-            return (IJackcessOpenerInterface) instance;
+        Class<?> clazz = Try.catching(() -> Class.forName(className))
+            .orThrow(ex -> new UcanaccessSQLException("Failed to load class " + className, ex));
+        if (!IJackcessOpenerInterface.class.isAssignableFrom(clazz)) {
+            throw new UcanaccessSQLException("Jackess Opener class must implement " + IJackcessOpenerInterface.class.getName());
         }
-        throw new UcanaccessSQLException("Jackess Opener class must implement " + IJackcessOpenerInterface.class.getName());
+        Object instance = Try.catching(() -> clazz.getConstructor().newInstance())
+            .orThrow(ex -> new UcanaccessSQLException("Failed to instantiate " + className, ex));
+        return (IJackcessOpenerInterface) instance;
     }
 
     /**

src/site/xhtml/20-getting-started.xhtml

@@ -21,7 +21,7 @@
 <dependency>
     <groupId>io.github.spannm</groupId>
     <artifactId>ucanaccess</artifactId>
-    <version>5.1.3</version>
+    <version>5.1.7</version>
 &lt;/dependency&gt;</pre>
 
         <h2>Establishing a UCanAccess connection</h2>
@@ -178,6 +178,15 @@ Connection connExample = DriverManager.getConnection(&quot;jdbc:ucanaccess://c:/
             for UCanAccess2. Notice that you must use UCanAccess 2.x.x with jackcess-encrypt-2.x.x and all related
             dependencies</strong><br/>
         </p>
+        <p>
+            <strong>Security note:</strong> the class name passed to <code>jackcessOpener</code> is loaded and
+            instantiated via reflection using its public no-arg constructor. This parameter must therefore only ever
+            be set from trusted, static application configuration &#8211; never from untrusted or user-supplied input
+            (e.g. values taken from an HTTP request, or forwarded unchecked from an external system). Populating
+            <code>jackcessOpener</code> from untrusted input could allow an attacker to trigger the construction of
+            an arbitrary class implementing <i>IJackcessOpenerInterface</i> that is reachable on the application's
+            classpath.
+        </p>
         <pre>
 package yourPackage.example;
 

src/test/java/net/ucanaccess/jdbc/UcanaccessDriverTest.java

@@ -1,12 +1,20 @@
 package net.ucanaccess.jdbc;
 
-import static net.ucanaccess.converters.Metadata.Property.*;
+import static net.ucanaccess.converters.Metadata.Property.columnOrder;
+import static net.ucanaccess.converters.Metadata.Property.concatNulls;
+import static net.ucanaccess.converters.Metadata.Property.encrypt;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import io.github.spannm.jackcess.Database;
 import net.ucanaccess.converters.Metadata.Property;
+import net.ucanaccess.exception.UcanaccessSQLException;
 import net.ucanaccess.test.UcanaccessBaseTest;
 import org.junit.jupiter.api.Test;
 
+import java.io.File;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Properties;
@@ -42,4 +50,68 @@ void testVersion() {
         assertThat(driver.getMinorVersion()).isGreaterThanOrEqualTo(1);
     }
 
+    /**
+     * A class name passed via the {@code jackcessOpener}
+     * property must be rejected if it does not implement {@code IJackcessOpenerInterface} -
+     * and its constructor must never execute in that case, since constructor side effects
+     * cannot be undone once the class has been instantiated.
+     */
+    @Test
+    void testNewJackcessOpenerInstance_rejectsNonImplementingClass_beforeConstruction() throws Exception {
+        NonOpenerWithSideEffect.instantiated = false;
+
+        assertThatThrownBy(() -> invokeNewJackcessOpenerInstance(NonOpenerWithSideEffect.class.getName()))
+            .isInstanceOf(UcanaccessSQLException.class)
+            .hasMessageContaining("must implement");
+
+        assertThat(NonOpenerWithSideEffect.instantiated)
+            .as("constructor of a class not implementing IJackcessOpenerInterface must never run")
+            .isFalse();
+    }
+
+    @Test
+    void testNewJackcessOpenerInstance_acceptsImplementingClass() throws Exception {
+        Object instance = invokeNewJackcessOpenerInstance(ValidTestOpener.class.getName());
+
+        assertThat(instance).isInstanceOf(IJackcessOpenerInterface.class);
+    }
+
+    @Test
+    void testNewJackcessOpenerInstance_wrapsUnknownClassNameInSqlException() {
+        assertThatThrownBy(() -> invokeNewJackcessOpenerInstance("does.not.Exist"))
+            .isInstanceOf(UcanaccessSQLException.class);
+    }
+
+    private Object invokeNewJackcessOpenerInstance(String className) throws Exception {
+        Method method = UcanaccessDriver.class.getDeclaredMethod("newJackcessOpenerInstance", String.class);
+        method.setAccessible(true);
+        try {
+            return method.invoke(new UcanaccessDriver(), className);
+        } catch (InvocationTargetException _ex) {
+            if (_ex.getCause() instanceof Exception) {
+                throw (Exception) _ex.getCause();
+            }
+            throw _ex;
+        }
+    }
+
+    /** Simulates a malicious class unrelated to IJackcessOpenerInterface with a dangerous constructor side effect. */
+    static final class NonOpenerWithSideEffect {
+        static boolean instantiated = false;
+
+        public NonOpenerWithSideEffect() {
+            instantiated = true;
+        }
+    }
+
+    static final class ValidTestOpener implements IJackcessOpenerInterface {
+        public ValidTestOpener() {
+        }
+
+        @Override
+        public Database open(File _file, String _password) {
+            return null;
+        }
+    }
+
 }