This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new fc88ddc8d4 [#12035] improvement(catalog-jdbc): block DBCP2
class-loading properties in datasource config (#12036)
fc88ddc8d4 is described below
commit fc88ddc8d4e8c35bb0d06cebd125a4559e771084
Author: YangJie <[email protected]>
AuthorDate: Mon Jul 20 16:55:46 2026 +0800
[#12035] improvement(catalog-jdbc): block DBCP2 class-loading properties in
datasource config (#12036)
### What changes were proposed in this pull request?
Add `rejectUnsafePoolProperties` in `DataSourceUtils`, rejecting the
DBCP2 pool properties `connectionFactoryClassName`,
`evictionPolicyClassName`, `driverClassName`, and `initialSize`
(case-insensitive whole-key match) before the config map is handed to
`BasicDataSourceFactory`.
### Why are the changes needed?
DBCP2's factory loads and instantiates the class named by the first
three via reflection (`Class.forName` + `newInstance`), and
`initialSize` > 0 forces an eager connection — triggering that driver
load — during `createDataSource`, before Gravitino's explicit
`setDriverClassName` override. Reachable via
`gravitino.bypass.<key>=...`, this lets a catalog creator run arbitrary
classes on the server (RCE).
Fix: #12035
### Does this PR introduce _any_ user-facing change?
No. These properties are never used by a legitimate Gravitino JDBC
catalog (the driver comes from `jdbc-driver`; pool sizing from
`jdbc.pool.*`). Supplying one now fails fast with a clear message.
### How was this patch tested?
New unit tests in `TestDataSourceUrlValidation` covering each rejected
property (including case-insensitive variants) and a whole-key negative
control proving a similarly-named legitimate property still builds.
`./gradlew :catalogs:catalog-jdbc-common:test -PskipITs` and spotless
pass.
---
.../catalog/jdbc/utils/DataSourceUtils.java | 56 +++++
.../jdbc/utils/TestDataSourceUrlValidation.java | 257 ++++++++++++++++++++-
2 files changed, 307 insertions(+), 6 deletions(-)
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
index a2b8e12b80..e06f376c7b 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.catalog.jdbc.utils;
import java.sql.SQLException;
import java.time.Duration;
+import java.util.List;
import java.util.Map;
import java.util.Properties;
import javax.sql.DataSource;
@@ -39,6 +40,32 @@ public class DataSourceUtils {
/** SQL statements for database connection pool testing. */
private static final String POOL_TEST_QUERY = "SELECT 1";
+ // DBCP2 connection-pool properties that must never come from catalog
configuration. The whole
+ // config map is handed to BasicDataSourceFactory, so allowing these would
either run arbitrary
+ // code or let a raw property override the validated canonical connection
fields:
+ // - connectionFactoryClassName / evictionPolicyClassName /
driverClassName: their values are
+ // class names the factory loads and instantiates via reflection
(Class.forName + newInstance)
+ // when the pool creates connections (remote code execution). The
legitimate driver is set
+ // separately from the "jdbc-driver" property via an explicit setter, so
the raw
+ // "driverClassName" key is never needed.
+ // - url / username / password: the connection identity is validated
(JdbcUrlUtils, the H2
+ // guard) and applied from the canonical
"jdbc-url"/"jdbc-user"/"jdbc-password" properties via
+ // explicit setters. A raw DBCP key here would reach the factory
unvalidated and, once the
+ // pool initializes, take effect before those setters run — e.g. a raw
"url" could smuggle an
+ // H2 or otherwise-denied URL past the guards.
+ // - initialSize: defense in depth. A value > 0 makes the factory eagerly
open a connection
+ // during createDataSource; blocking it keeps pool creation lazy so our
explicit url/driver
+ // setters, not factory-time eager init, control how connections are
created.
+ private static final List<String> UNSAFE_POOL_PROPERTIES =
+ List.of(
+ "connectionFactoryClassName",
+ "evictionPolicyClassName",
+ "driverClassName",
+ "url",
+ "username",
+ "password",
+ "initialSize");
+
public static DataSource createDataSource(Map<String, String> properties) {
return createDataSource(new JdbcConfig(properties));
}
@@ -56,6 +83,10 @@ public class DataSourceUtils {
if (jdbcConfig.getJdbcDriver().toLowerCase().startsWith("org.h2.")) {
throw new GravitinoRuntimeException("H2 JDBC driver is not allowed in
catalog configuration");
}
+ // Reject DBCP2 pool properties that load arbitrary classes via reflection
before handing the
+ // config to the factory. Kept outside the try below so the specific
reason surfaces directly
+ // instead of being wrapped as "Error creating datasource".
+ rejectUnsafePoolProperties(jdbcConfig.getAllConfig());
try {
return createDBCPDataSource(jdbcConfig);
} catch (Exception exception) {
@@ -92,6 +123,31 @@ public class DataSourceUtils {
return properties;
}
+ /**
+ * Rejects DBCP2 connection-pool properties that would let catalog
configuration run arbitrary
+ * classes on the server ({@code connectionFactoryClassName}, {@code
evictionPolicyClassName},
+ * {@code driverClassName}) or override the validated canonical connection
identity ({@code url},
+ * {@code username}, {@code password}), plus {@code initialSize} (blocked as
defense in depth to
+ * keep pool creation lazy).
+ *
+ * @param config the JDBC configuration properties forwarded to the DBCP2
factory
+ * @throws GravitinoRuntimeException if an unsafe connection-pool property
is present
+ */
+ private static void rejectUnsafePoolProperties(Map<String, String> config) {
+ if (config == null) {
+ return;
+ }
+ for (String key : config.keySet()) {
+ for (String unsafe : UNSAFE_POOL_PROPERTIES) {
+ if (unsafe.equalsIgnoreCase(key)) {
+ throw new GravitinoRuntimeException(
+ "Unsafe JDBC connection pool property '%s' is not allowed in
catalog configuration",
+ key);
+ }
+ }
+ }
+ }
+
private static String recursiveDecode(String url) {
String prev;
String decoded = url;
diff --git
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
index e05e4f1062..41ac39cfe6 100644
---
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
+++
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java
@@ -44,6 +44,24 @@ public class TestDataSourceUrlValidation {
((BasicDataSource) dataSource).close();
}
+ @Test
+ public void testSimilarNamedPropertyIsNotRejected() throws SQLException {
+ // The unsafe-pool-property guard matches the whole key
(equalsIgnoreCase), so a legitimate key
+ // that merely contains an unsafe name as a substring must still build.
Guards against a
+ // regression to contains/startsWith matching.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.sqlite.JDBC");
+ properties.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("connectionFactoryClassNameHelper", "com.example.Fine");
+
+ DataSource dataSource =
+ Assertions.assertDoesNotThrow(() ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertTrue(dataSource instanceof BasicDataSource);
+ ((BasicDataSource) dataSource).close();
+ }
+
@Test
public void testRejectMysqlAllowLoadLocalInfile() {
HashMap<String, String> properties = Maps.newHashMap();
@@ -53,8 +71,13 @@ public class TestDataSourceUrlValidation {
properties.put(JdbcConfig.USERNAME.getKey(), "test");
properties.put(JdbcConfig.PASSWORD.getKey(), "test");
- Assertions.assertThrows(
- GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ // validateJdbcConfig throws inside the try, so the specific reason is the
wrapped cause.
+ Assertions.assertNotNull(gre.getCause());
+ Assertions.assertTrue(
+
gre.getCause().getMessage().toLowerCase().contains("allowloadlocalinfile"));
}
@Test
@@ -67,8 +90,11 @@ public class TestDataSourceUrlValidation {
properties.put(JdbcConfig.USERNAME.getKey(), "test");
properties.put(JdbcConfig.PASSWORD.getKey(), "test");
- Assertions.assertThrows(
- GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertNotNull(gre.getCause());
+
Assertions.assertTrue(gre.getCause().getMessage().contains("socketFactory"));
}
@Test
@@ -81,8 +107,12 @@ public class TestDataSourceUrlValidation {
properties.put(JdbcConfig.USERNAME.getKey(), "test");
properties.put(JdbcConfig.PASSWORD.getKey(), "test");
- Assertions.assertThrows(
- GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertNotNull(gre.getCause());
+ Assertions.assertTrue(
+
gre.getCause().getMessage().toLowerCase().contains("allowloadlocalinfile"));
}
@Test
@@ -147,4 +177,219 @@ public class TestDataSourceUrlValidation {
Assertions.assertEquals(
"H2 JDBC driver is not allowed in catalog configuration",
gre.getMessage());
}
+
+ @Test
+ public void testRejectConnectionFactoryClassName() {
+ // DBCP2's BasicDataSourceFactory loads "connectionFactoryClassName" via
Class.forName and
+ // instantiates it at pool init, which is a remote-code-execution vector.
A user can reach this
+ // key through the "gravitino.bypass." prefix (stripped before the config
map is handed here),
+ // so it must be rejected.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("connectionFactoryClassName", "com.example.Evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'connectionFactoryClassName' is
not allowed in"
+ + " catalog configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectEvictionPolicyClassName() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("evictionPolicyClassName", "com.example.Evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'evictionPolicyClassName' is not
allowed in catalog"
+ + " configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectConnectionFactoryClassNameCaseInsensitive() {
+ // The guard matches case-insensitively (broader than DBCP2's own
case-sensitive key lookup),
+ // so a mis-cased variant is rejected outright rather than silently
ignored. The error echoes
+ // the user-supplied key casing.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("ConnectionFactoryClassName", "com.example.Evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'ConnectionFactoryClassName' is
not allowed in"
+ + " catalog configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectEvictionPolicyClassNameCaseInsensitive() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("EVICTIONPOLICYCLASSNAME", "com.example.Evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'EVICTIONPOLICYCLASSNAME' is not
allowed in catalog"
+ + " configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectDriverClassName() {
+ // DBCP2's BasicDataSourceFactory loads the "driverClassName" value via
Class.forName and
+ // instantiates it (an RCE vector) when the pool creates a connection. The
legitimate driver is
+ // set separately from the "jdbc-driver" property, so the raw key must be
rejected.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("driverClassName", "com.example.Evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'driverClassName' is not allowed
in catalog"
+ + " configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectInitialSize() {
+ // initialSize > 0 makes the factory eagerly open a connection during
createDataSource; reject
+ // it (defense in depth) so pool creation stays lazy and our explicit
url/driver setters control
+ // how connections are created.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("initialSize", "1");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'initialSize' is not allowed in
catalog"
+ + " configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectRawUrlProperty() {
+ // A raw DBCP "url" key would reach the factory unvalidated and, once the
pool initializes, take
+ // effect before the canonical setUrl override — smuggling a denied URL
(e.g. H2) past the
+ // guards. The canonical URL is supplied via "jdbc-url", so the raw key
must be rejected.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("url", "jdbc:h2:mem:evil");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'url' is not allowed in catalog
configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectRawUsernameProperty() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("username", "root");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'username' is not allowed in
catalog configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRejectRawPasswordProperty() {
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(),
"com.mysql.cj.jdbc.Driver");
+ properties.put(JdbcConfig.JDBC_URL.getKey(),
"jdbc:mysql://localhost:3306/test");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("password", "secret");
+
+ GravitinoRuntimeException gre =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals(
+ "Unsafe JDBC connection pool property 'password' is not allowed in
catalog configuration",
+ gre.getMessage());
+ }
+
+ @Test
+ public void testRawUrlBypassCannotSmuggleH2ViaEagerInit() {
+ // Regression for the factory-init ordering issue: even with initialSize >
0 forcing eager pool
+ // init AND a raw "url" pointing at H2, the config must be rejected before
the factory runs, so
+ // the H2 URL never becomes the live connection.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.sqlite.JDBC");
+ properties.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+ properties.put("url", "jdbc:h2:mem:evil");
+ properties.put("initialSize", "1");
+
+ // Rejected on the first unsafe key encountered (url or initialSize); the
point is that
+ // creation fails before BasicDataSourceFactory initializes the pool with
the raw url.
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
DataSourceUtils.createDataSource(properties));
+ }
+
+ @Test
+ public void testValidConnectionUsesCanonicalUrl() throws SQLException {
+ // Positive control: with only canonical keys, the live connection uses
the canonical URL, not
+ // any bootstrap value — proving the ordering fix holds for legitimate
configs.
+ HashMap<String, String> properties = Maps.newHashMap();
+ properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.sqlite.JDBC");
+ properties.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:");
+ properties.put(JdbcConfig.USERNAME.getKey(), "test");
+ properties.put(JdbcConfig.PASSWORD.getKey(), "test");
+
+ BasicDataSource dataSource =
+ (BasicDataSource)
+ Assertions.assertDoesNotThrow(() ->
DataSourceUtils.createDataSource(properties));
+ Assertions.assertEquals("jdbc:sqlite::memory:", dataSource.getUrl());
+ try (java.sql.Connection connection = dataSource.getConnection()) {
+ Assertions.assertEquals("jdbc:sqlite::memory:",
connection.getMetaData().getURL());
+ } finally {
+ dataSource.close();
+ }
+ }
}
