Skip to content

Commit

Permalink
Merge pull request from GHSA-v7wg-cpwc-24m4
Browse files Browse the repository at this point in the history
* test: Add some SSL helpers to TestUtil

* security: verify plugin classes adhere to the expected interface before instantiating the class

This ensures arbitrary classes can't be passed instead of
SocketFactory, SSLSocketFactory, CallbackHandler, HostnameVerifier

Co-authored-by: Sehrope Sarkuni <sehrope@jackdb.com>
  • Loading branch information
vlsi and sehrope committed Feb 1, 2022
1 parent 51c70a9 commit 8a363a7
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static SocketFactory getSocketFactory(Properties info) throws PSQLExcepti
return SocketFactory.getDefault();
}
try {
return (SocketFactory) ObjectFactory.instantiate(socketFactoryClassName, info, true,
return ObjectFactory.instantiate(SocketFactory.class, socketFactoryClassName, info, true,
PGProperty.SOCKET_FACTORY_ARG.get(info));
} catch (Exception e) {
throw new PSQLException(
Expand All @@ -61,7 +61,7 @@ public static SSLSocketFactory getSslSocketFactory(Properties info) throws PSQLE
return new LibPQFactory(info);
}
try {
return (SSLSocketFactory) ObjectFactory.instantiate(classname, info, true,
return ObjectFactory.instantiate(SSLSocketFactory.class, classname, info, true,
PGProperty.SSL_FACTORY_ARG.get(info));
} catch (Exception e) {
throw new PSQLException(
Expand Down
2 changes: 1 addition & 1 deletion pgjdbc/src/main/java/org/postgresql/ssl/LibPQFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ private CallbackHandler getCallbackHandler(
String sslpasswordcallback = PGProperty.SSL_PASSWORD_CALLBACK.get(info);
if (sslpasswordcallback != null) {
try {
cbh = (CallbackHandler) ObjectFactory.instantiate(sslpasswordcallback, info, false, null);
cbh = ObjectFactory.instantiate(CallbackHandler.class, sslpasswordcallback, info, false, null);
} catch (Exception e) {
throw new PSQLException(
GT.tr("The password callback class provided {0} could not be instantiated.",
Expand Down
2 changes: 1 addition & 1 deletion pgjdbc/src/main/java/org/postgresql/ssl/MakeSSL.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private static void verifyPeerName(PGStream stream, Properties info, SSLSocket n
sslhostnameverifier = "PgjdbcHostnameVerifier";
} else {
try {
hvn = (HostnameVerifier) instantiate(sslhostnameverifier, info, false, null);
hvn = instantiate(HostnameVerifier.class, sslhostnameverifier, info, false, null);
} catch (Exception e) {
throw new PSQLException(
GT.tr("The HostnameVerifier class provided {0} could not be instantiated.",
Expand Down
7 changes: 4 additions & 3 deletions pgjdbc/src/main/java/org/postgresql/util/ObjectFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ public class ObjectFactory {
* @throws IllegalAccessException if something goes wrong
* @throws InvocationTargetException if something goes wrong
*/
public static Object instantiate(String classname, Properties info, boolean tryString,
public static <T> T instantiate(Class<T> expectedClass, String classname, Properties info,
boolean tryString,
@Nullable String stringarg)
throws ClassNotFoundException, SecurityException, NoSuchMethodException,
IllegalArgumentException, InstantiationException, IllegalAccessException,
InvocationTargetException {
@Nullable Object[] args = {info};
Constructor<?> ctor = null;
Class<?> cls = Class.forName(classname);
Constructor<? extends T> ctor = null;
Class<? extends T> cls = Class.forName(classname).asSubclass(expectedClass);
try {
ctor = cls.getConstructor(Properties.class);
} catch (NoSuchMethodException ignored) {
Expand Down
23 changes: 23 additions & 0 deletions pgjdbc/src/test/java/org/postgresql/test/TestUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import org.checkerframework.checker.nullness.qual.Nullable;
import org.junit.Assert;
import org.junit.Assume;

import java.io.Closeable;
import java.io.File;
Expand Down Expand Up @@ -265,6 +266,28 @@ public static Properties loadPropertyFiles(String... names) {
return p;
}

private static Properties sslTestProperties = null;

private static synchronized void initSslTestProperties() {
if (sslTestProperties == null) {
sslTestProperties = TestUtil.loadPropertyFiles("ssltest.properties");
}
}

private static String getSslTestProperty(String name) {
initSslTestProperties();
return sslTestProperties.getProperty(name);
}

public static void assumeSslTestsEnabled() {
Assume.assumeTrue(Boolean.parseBoolean(getSslTestProperty("enable_ssl_tests")));
}

public static String getSslTestCertPath(String name) {
File certdir = TestUtil.getFile(getSslTestProperty("certdir"));
return new File(certdir, name).getAbsolutePath();
}

public static void initDriver() {
synchronized (TestUtil.class) {
if (initialized) {
Expand Down
100 changes: 100 additions & 0 deletions pgjdbc/src/test/java/org/postgresql/test/util/ObjectFactoryTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright (c) 2022, PostgreSQL Global Development Group
* See the LICENSE file in the project root for more information.
*/

package org.postgresql.test.util;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;

import org.postgresql.PGProperty;
import org.postgresql.jdbc.SslMode;
import org.postgresql.test.TestUtil;
import org.postgresql.util.ObjectFactory;
import org.postgresql.util.PSQLState;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Assertions;
import org.opentest4j.MultipleFailuresError;

import java.sql.SQLException;
import java.util.Properties;

import javax.net.SocketFactory;

public class ObjectFactoryTest {
Properties props = new Properties();

static class BadObject {
static boolean wasInstantiated = false;

BadObject() {
wasInstantiated = true;
throw new RuntimeException("I should not be instantiated");
}
}

private void testInvalidInstantiation(PGProperty prop, PSQLState expectedSqlState) {
prop.set(props, BadObject.class.getName());

BadObject.wasInstantiated = false;
SQLException ex = assertThrows(SQLException.class, () -> {
TestUtil.openDB(props);
});

try {
Assertions.assertAll(
() -> assertFalse(BadObject.wasInstantiated, "ObjectFactory should not have "
+ "instantiated bad object for " + prop),
() -> assertEquals(expectedSqlState.getState(), ex.getSQLState(), () -> "#getSQLState()"),
() -> {
assertThrows(
ClassCastException.class,
() -> {
throw ex.getCause();
},
() -> "Wrong class specified for " + prop.name()
+ " => ClassCastException is expected in SQLException#getCause()"
);
}
);
} catch (MultipleFailuresError e) {
// Add the original exception so it is easier to understand the reason for the test to fail
e.addSuppressed(ex);
throw e;
}
}

@Test
public void testInvalidSocketFactory() {
testInvalidInstantiation(PGProperty.SOCKET_FACTORY, PSQLState.CONNECTION_FAILURE);
}

@Test
public void testInvalidSSLFactory() {
TestUtil.assumeSslTestsEnabled();
// We need at least "require" to trigger SslSockerFactory instantiation
PGProperty.SSL_MODE.set(props, SslMode.REQUIRE.value);
testInvalidInstantiation(PGProperty.SSL_FACTORY, PSQLState.CONNECTION_FAILURE);
}

@Test
public void testInvalidSslHostnameVerifier() {
TestUtil.assumeSslTestsEnabled();
// Hostname verification is done at verify-full level only
PGProperty.SSL_MODE.set(props, SslMode.VERIFY_FULL.value);
PGProperty.SSL_ROOT_CERT.set(props, TestUtil.getSslTestCertPath("goodroot.crt"));
testInvalidInstantiation(PGProperty.SSL_HOSTNAME_VERIFIER, PSQLState.CONNECTION_FAILURE);
}

@Test
public void testInstantiateInvalidSocketFactory() {
Properties props = new Properties();
assertThrows(ClassCastException.class, () -> {
ObjectFactory.instantiate(SocketFactory.class, BadObject.class.getName(), props,
false, null);
});
}
}

0 comments on commit 8a363a7

Please sign in to comment.