Skip to content

Commit

Permalink
Merge pull request from GHSA-v7wg-cpwc-24m4
Browse files Browse the repository at this point in the history
This ensures arbitrary classes can't be passed instead of
AuthenticationPlugin, SocketFactory, SSLSocketFactory, CallbackHandler, HostnameVerifier
  • Loading branch information
vlsi committed Feb 1, 2022
1 parent d6e5312 commit f4d0ed6
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 11 deletions.
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
Expand Up @@ -66,11 +66,12 @@ public static <T> T withPassword(AuthenticationRequestType type, Properties info
} else {
AuthenticationPlugin authPlugin;
try {
authPlugin = (AuthenticationPlugin) ObjectFactory.instantiate(authPluginClassName, info,
authPlugin = ObjectFactory.instantiate(AuthenticationPlugin.class, authPluginClassName, info,
false, null);
} catch (Exception ex) {
LOGGER.log(Level.FINE, "Unable to load Authentication Plugin " + ex.toString());
throw new PSQLException(ex.getMessage(), PSQLState.UNEXPECTED_ERROR);
String msg = GT.tr("Unable to load Authentication Plugin {0}", authPluginClassName);
LOGGER.log(Level.FINE, msg, ex);
throw new PSQLException(msg, PSQLState.INVALID_PARAMETER_VALUE, ex);
}

password = authPlugin.getPassword(type);
Expand Down Expand Up @@ -106,7 +107,8 @@ public static <T> T withEncodedPassword(AuthenticationRequestType type, Properti
byte[] encodedPassword = withPassword(type, info, password -> {
if (password == null) {
throw new PSQLException(
GT.tr("The server requested password-based authentication, but no password was provided."),
GT.tr("The server requested password-based authentication, but no password was provided by plugin {0}",
PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.get(info)),
PSQLState.CONNECTION_REJECTED);
}
ByteBuffer buf = StandardCharsets.UTF_8.encode(CharBuffer.wrap(password));
Expand Down
2 changes: 1 addition & 1 deletion pgjdbc/src/main/java/org/postgresql/ssl/LibPQFactory.java
Expand Up @@ -56,7 +56,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
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
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);

This comment has been minimized.

Copy link
@tbroyer

tbroyer Feb 4, 2022

Correct me if I'm wrong but this Class.forName will still initialize the class, which means executing the static initializers, and static fields' initializers (https://docs.oracle.com/javase/specs/jls/se11/html/jls-12.html#jls-12.4), so in effect you've only guarded the execution of the constructor, but not of all possible code execution paths.

To fully protect against (some) RCE, you would have to initialize the class after you've ensured it's of a correct type, i.e. load the class with forName(classname, false, ObjectFactory.class.getClassLoader()).

This comment has been minimized.

Copy link
@davecramer

davecramer via email Feb 4, 2022

Member

This comment has been minimized.

Copy link
@vlsi

vlsi Feb 4, 2022

Author Member

I agree it is worth using forName(classname, false, however, the set of available gadgets for <clinit> exploits is way less than the number of vulnerable (String) constructors.

This comment has been minimized.

Copy link
@Sanne

Sanne Feb 16, 2022

FWIW, I agree with @davecramer that this doesn't look like an RCE. I'd even go as far as not considering it a vulnerability at all: you already throw a ClassCastException, which is a reasonable reaction to a configuration mistake.

As to the loaded code to include malicious instructions.. it's not your projects responsibility that users included malicious code on the classpath. The advisory seems to suggest that the plugin could potentially connect to a different system and then do who-knows-what with the return - clearly that's a problem of lack of safeguards in such a plugin implementation.

This comment has been minimized.

Copy link
@tbroyer

tbroyer Feb 16, 2022

So either there was no vulnerability and this commit and the advisory were useless (and I'd even say harmful, crying wolf erodes confidence in the whole advisory process), or there's possibly a vulnerability (unchecked class instantiation) and this commit only partially fixes the problem as it leaves open "unchecked class initialisation".

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 16, 2022

Member

Well I had a long discussion with the security folks at github and we negotiated a lower score. They still felt it was a vulnerability. I have changed the description. It is no longer an RCE

This comment has been minimized.

Copy link
@vlsi

vlsi Feb 16, 2022

Author Member

@Sanne ,

you already throw a ClassCastException, which is a reasonable reaction to a configuration mistake.

The previous implementation was:

  1. Instantiate class first (call constructor)
  2. Cast the instance and only then throw ClassCastException

There are classes that might perform remote code execution when their constructor is called.

That is why the fix is to call the constructor only for the classes that implement the required pgjdbc interface. That significantly reduces the set of classes an attacker can use.

This comment has been minimized.

Copy link
@Sanne

Sanne Feb 16, 2022

Yes I understand the rationale. So this limits the options of a potential attaker that would want to take advantage of an existing class in the application to perform something malicious - without adding new classes - AND also has enough privileges to change the configuration. This limits the set of classes one might find potentially useful to do such a thing.

That makes sense and I have no problems with the proposed patch - the new check is certainly better: to be clear I wasn't complaining about the fix :)
Thanks everyone!

Still I was surprised this is considered an issue - IMO if an untrusted individual can change the database configuration the organization has bigger issues - but we can certainly move on.

This comment has been minimized.

Copy link
@davecramer

davecramer Feb 17, 2022

Member

@Sanne I learned a lot during this.

try {
ctor = cls.getConstructor(Properties.class);
} catch (NoSuchMethodException ignored) {
Expand Down
106 changes: 106 additions & 0 deletions pgjdbc/src/test/java/org/postgresql/test/util/ObjectFactoryTest.java
@@ -0,0 +1,106 @@
/*
* 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 testInvalidAuthenticationPlugin() {
testInvalidInstantiation(PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME,
PSQLState.INVALID_PARAMETER_VALUE);
}

@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 f4d0ed6

Please sign in to comment.