diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java index bc845660834..1775ce6c467 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java @@ -39,6 +39,11 @@ public class AlertExpressionEvalVisitor extends AlertExpressionBaseVisitor> visitSqlExpr(AlertExpressionParser.SqlExprConte @Override public List> visitSqlCallExpr(AlertExpressionParser.SqlCallExprContext ctx) { - return callSqlOrPromql(tokens.getText(ctx.string())); + return executor.execute(unquote(tokens.getText(ctx.string()))); } @Override public List> visitPromqlCallExpr(AlertExpressionParser.PromqlCallExprContext ctx) { - return callSqlOrPromql(tokens.getText(ctx.string())); + return executor.execute(unquote(tokens.getText(ctx.string()))); } - private List> callSqlOrPromql(String text) { - String script = text.substring(1, text.length() - 1); - return executor.execute(script); + private String unquote(String text) { + return text.substring(1, text.length() - 1); } /** diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java index 136777c085b..6698c9ac603 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java @@ -31,7 +31,6 @@ import org.apache.hertzbeat.alert.expr.AlertExpressionParser; import org.apache.hertzbeat.alert.service.DataSourceService; import org.apache.hertzbeat.common.support.exception.AlertExpressionException; -import org.apache.hertzbeat.common.support.valid.SqlSecurityException; import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; import org.apache.hertzbeat.common.util.ResourceBundleUtil; import org.apache.hertzbeat.warehouse.constants.WarehouseConstants; @@ -58,6 +57,13 @@ public class DataSourceServiceImpl implements DataSourceService { */ private static final List DEFAULT_ALLOWED_TABLES = List.of(WarehouseConstants.LOG_TABLE_NAME); + /** + * The policy for an alert expression is read only and nothing narrower: which tables it + * may read is not constrained, because metric tables are created per metric on demand and + * a whitelist would reject every legitimate metric query. + */ + private static final SqlSecurityValidator EXPRESSION_SQL_VALIDATOR = SqlSecurityValidator.selectOnly(); + protected ResourceBundle bundle = ResourceBundleUtil.getBundle("alerter"); @Setter @@ -93,7 +99,7 @@ public List> calculate(String datasource, String expr) { // replace all white space expr = expr.replaceAll("\\s+", " "); try { - return evaluate(expr, executor); + return evaluate(expr, guardSql(executor, EXPRESSION_SQL_VALIDATOR)); } catch (AlertExpressionException ae) { log.error("Calculate query parse error, datasource: {}, expr: {}, msg: {}", datasource, expr, ae.getMessage(), ae); throw ae; @@ -119,13 +125,11 @@ public List> query(String datasource, String expr) { // replace all white space expr = expr.replaceAll("\\s+", " "); - // SQL security validation for SQL-based datasources - if (isSqlDatasource(datasource)) { - validateSqlSecurity(expr); - } - try { - return executor.execute(expr); + return guardSql(executor, sqlSecurityValidator).execute(expr); + } catch (AlertExpressionException ae) { + // A statement the policy rejected, whose message names the part it broke. + throw ae; } catch (Exception e) { log.error("Error executing query on datasource {}: {}", datasource, e.getMessage()); throw new AlertExpressionException(e.getMessage()); @@ -133,22 +137,22 @@ public List> query(String datasource, String expr) { } /** - * Check if the datasource is SQL-based - */ - private boolean isSqlDatasource(String datasource) { - return datasource != null && datasource.equalsIgnoreCase(WarehouseConstants.SQL); - } - - /** - * Validate SQL statement for security + * Wraps an executor that speaks sql so that nothing runs on it unvalidated. + * + *

The decision is made from the executor rather than from the datasource string the + * caller passed, because the executor is what actually holds the database credentials. + * A datasource that does not speak sql is handed back untouched: a promql endpoint takes + * a query string, not a statement, and running it through a sql parser would only reject + * valid promql. + * @param executor Executor chosen for this datasource + * @param validator Policy to enforce, read only for expressions and whitelisting for raw log queries + * @return The executor, guarded when it speaks sql */ - private void validateSqlSecurity(String sql) { - try { - sqlSecurityValidator.validate(sql); - } catch (SqlSecurityException e) { - log.warn("SQL security validation failed: {}", e.getMessage()); - throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); + private QueryExecutor guardSql(QueryExecutor executor, SqlSecurityValidator validator) { + if (!executor.support(WarehouseConstants.SQL)) { + return executor; } + return new SqlValidatingQueryExecutor(executor, validator); } private List> evaluate(String expr, QueryExecutor executor) { diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java new file mode 100644 index 00000000000..020669594c1 --- /dev/null +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java @@ -0,0 +1,83 @@ +/* + * 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.alert.service.impl; + +import org.apache.hertzbeat.common.entity.dto.query.DatasourceQuery; +import org.apache.hertzbeat.common.entity.dto.query.DatasourceQueryData; +import org.apache.hertzbeat.common.support.exception.AlertExpressionException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; +import org.apache.hertzbeat.warehouse.db.QueryExecutor; + +import java.util.List; +import java.util.Map; + +/** + * A sql executor that validates before it runs anything. + * + *

An alert expression reaches a query executor by several routes: the {@code sql("...")} + * and {@code promql("...")} spellings both carry an arbitrary string, a bare select is + * parsed by the expression grammar itself, and each of them is evaluated for the preview + * endpoint and for the periodic evaluation loop alike. All of them end at + * {@link QueryExecutor#execute(String)}, which runs the string with the server side database + * credentials. + * + *

Guarding that one method rather than each route is what makes the check complete: the + * spelling an expression happens to use does not decide whether the statement is checked, + * the database it lands on does. It also means a route added later is covered without anyone + * remembering to add a call. + */ +public class SqlValidatingQueryExecutor implements QueryExecutor { + + private final QueryExecutor delegate; + + private final SqlSecurityValidator validator; + + public SqlValidatingQueryExecutor(QueryExecutor delegate, SqlSecurityValidator validator) { + this.delegate = delegate; + this.validator = validator; + } + + @Override + public List> execute(String query) { + try { + validator.validate(query); + } catch (SqlSecurityException e) { + // AlertExpressionException rather than a generic failure: it is the type + // DataSourceServiceImpl rethrows untouched and the preview endpoint turns into a + // 400, so the author of the rule sees which part of the policy the statement broke + throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); + } + return delegate.execute(query); + } + + @Override + public DatasourceQueryData query(DatasourceQuery datasourceQuery) { + return delegate.query(datasourceQuery); + } + + @Override + public String getDatasource() { + return delegate.getDatasource(); + } + + @Override + public boolean support(String queryLanguage) { + return delegate.support(queryLanguage); + } +} diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java index c12e23e80b1..f384d501af3 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java @@ -864,10 +864,55 @@ void testComparisonExpr() { assertEquals(0, result.get(0).get("__value__")); } + @Test + void testSqlCallRunsPlainRead() { + when(mockExecutor.execute("select value from cpu where host = 'server1'")) + .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + final List> result = + evaluate("sql(\"select value from cpu where host = 'server1'\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + + /** + * Subqueries and nested aggregation are supported in alert expressions, and with every + * metric table already readable, rejecting them would cost features without denying an + * attacker anything. The policy stops at read only on purpose. + */ + @Test + void testSqlCallKeepsSubqueriesWorking() { + final String sql = "select value from cpu where host = (select host from hosts limit 1)"; + when(mockExecutor.execute(sql)).thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + final List> result = evaluate("sql(\"" + sql + "\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + + /** + * The visitor hands both spellings to the executor as written. Whether a statement is + * allowed to run is decided by the executor it lands on, see + * {@code DataSourceServiceTest}, so promql keeps working for text that is not valid sql + * at all. + */ + @Test + void testBothCallSpellingsReachTheExecutorAsWritten() { + when(mockExecutor.execute("rate(http_requests_total[5m])")) + .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + final List> result = evaluate("promql(\"rate(http_requests_total[5m])\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + private List> evaluate(String expression) { AlertExpressionLexer lexer = new AlertExpressionLexer(CharStreams.fromString(expression)); CommonTokenStream tokens = new CommonTokenStream(lexer); AlertExpressionParser parser = new AlertExpressionParser(tokens); return new AlertExpressionEvalVisitor(mockExecutor, tokens).visit(parser.expression()); } -} \ No newline at end of file +} diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java index e9b098b1b08..b513d035049 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java @@ -787,4 +787,64 @@ void query12() { () -> dataSourceService.query("sql", "SELEC * FORM hertzbeat_logs")); verify(mockExecutor, never()).execute(anyString()); } + + /** + * The expression grammar offers three ways to reach the executor, and the datasource the + * caller names is what picks that executor. So a statement written with the + * {@code promql("...")} spelling still lands on the sql executor, with the server side + * database credentials behind it, whenever the caller names the sql datasource: whether a + * statement may run cannot be decided from the spelling. + */ + @Test + void calculateRejectsWritesWhateverSpellingTheyArrivedIn() { + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("sql")).thenReturn(true); + dataSourceService.setExecutors(List.of(mockExecutor)); + + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "sql(\"drop table cpu\") > 0")); + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "promql(\"drop table cpu\") > 0")); + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "sql(\"select 1; drop table cpu\") > 0")); + verify(mockExecutor, never()).execute(anyString()); + } + + /** + * Only a datasource that speaks sql is guarded. A promql endpoint takes a query string + * rather than a statement, so running it through a sql parser would reject valid promql + * without denying an attacker anything. + */ + @Test + void calculateLeavesPromqlDatasourcesAlone() { + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("promql")).thenReturn(true); + when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); + dataSourceService.setExecutors(List.of(mockExecutor)); + + final List> result = dataSourceService.calculate( + "promql", "promql(\"rate(http_requests_total[5m])\") > 70"); + + assertEquals(1, result.size()); + verify(mockExecutor).execute("rate(http_requests_total[5m])"); + } + + /** + * A read still has to run, including the GreptimeDB range query syntax that the sql + * parser cannot read. + */ + @Test + void calculateStillRunsReadsOnSqlDatasources() { + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("sql")).thenReturn(true); + when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); + dataSourceService.setExecutors(List.of(mockExecutor)); + + final String rangeQuery = "select avg(value) RANGE '10s' from cpu ALIGN '5s'"; + final List> result = + dataSourceService.calculate("sql", "sql(\"" + rangeQuery + "\") > 70"); + + assertEquals(1, result.size()); + verify(mockExecutor).execute(rangeQuery); + } } diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index 6d58dff18b4..5d926a82622 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -21,32 +21,69 @@ import net.sf.jsqlparser.JSQLParserException; import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.Statements; +import net.sf.jsqlparser.statement.delete.Delete; +import net.sf.jsqlparser.statement.insert.Insert; +import net.sf.jsqlparser.statement.merge.Merge; import net.sf.jsqlparser.statement.select.LateralSubSelect; import net.sf.jsqlparser.statement.select.ParenthesedSelect; +import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.statement.select.WithItem; +import net.sf.jsqlparser.statement.update.Update; import net.sf.jsqlparser.util.TablesNamesFinder; import org.springframework.util.CollectionUtils; import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.stream.Collectors; /** * SQL Security Validator using JSqlParser 5.1+. - * Security Policy: - * 1. Only SELECT statements are allowed. - * 2. All referenced tables must be in the whitelist. - * 3. Subqueries, UNION, CTE, LATERAL are blocked. + * + *

Two modes, see {@link #SqlSecurityValidator(Collection)} and {@link #selectOnly()}: + *

    + *
  • whitelisting: only SELECT, every referenced table must be whitelisted, and + * subqueries, UNION, CTE and LATERAL are blocked because they are the ways a statement + * can reach a table the whitelist never mentions.
    • + *
  • read only: only SELECT, any table, nested reads kept.
    • + *
+ * + *

Both modes reject a string that carries more than one statement, and neither lets a + * write through at any depth of the statement. */ @Slf4j public class SqlSecurityValidator { + private static final String SELECT_KEYWORD = "SELECT"; + + private static final String WITH_KEYWORD = "WITH"; + + /** + * Words that no read contains, matched as whole words outside literals and comments. + * + *

This is the check that holds when the parser cannot read the dialect and there is no + * tree to walk, so it has to catch a write wherever it sits, including nested in a cte or + * a subquery. It is deliberately coarse; the tree walk is the precise one. + * + *

Statements that can only stand alone, {@code TRUNCATE} and {@code CALL} among them, + * are absent: the leading keyword already rejects those, and several of them double as + * ordinary functions, {@code TRUNCATE(value, 2)} and {@code REPLACE(msg, 'a', 'b')} being + * the ones a metric query really does use. An identifier that collides with a word listed + * here can still be quoted, which the scan skips over. + */ + private static final Set WRITE_KEYWORDS = Set.of( + "DELETE", "INSERT", "UPDATE", "MERGE", "INTO", "DROP", "ALTER", + "CREATE", "GRANT", "REVOKE", "COPY", "RENAME", "ATTACH", "DETACH"); + private final Set allowedTables; + private final ValidationPolicy validationPolicy; + public SqlSecurityValidator(Collection allowedTables) { if (CollectionUtils.isEmpty(allowedTables)) { this.allowedTables = new HashSet<>(); @@ -55,18 +92,120 @@ public SqlSecurityValidator(Collection allowedTables) { .map(this::normalizeIdentifier) .collect(Collectors.toSet()); } + this.validationPolicy = ValidationPolicy.WHITELISTED_SELECT; + } + + private SqlSecurityValidator() { + this.allowedTables = new HashSet<>(); + this.validationPolicy = ValidationPolicy.READ_ONLY; + } + + /** + * A validator whose whole policy is "this statement may only read". + * + *

Use it where there is no table list to validate against: metric tables are created + * on demand, one per metric, so the whitelisting constructor would reject every + * legitimate metric query. + * + *

It deliberately keeps subqueries, unions and ctes, unlike the whitelisting mode. + * Those structures are blocked there because they are the ways a statement can reach a + * table the whitelist never mentions; with every table already readable they buy no + * protection, while alert expressions do use subqueries and nested aggregation. + * @return A validator that accepts only statements proven to be read-only + */ + public static SqlSecurityValidator selectOnly() { + return new SqlSecurityValidator(); } public void validate(String sql) throws SqlSecurityException { if (sql == null || sql.trim().isEmpty()) { throw new SqlSecurityException("SQL statement cannot be empty"); } + switch (validationPolicy) { + case WHITELISTED_SELECT -> validateAgainstWhitelist(sql); + case READ_ONLY -> validateReadOnly(sql); + // not dead: a policy added later that nobody wired up here must refuse the + // statement rather than fall through this switch having validated nothing + default -> throw new SqlSecurityException("Unknown SQL validation policy."); + } + } - Statement statement; + /** + * Read only mode, which runs against a time series database whose sql dialect JSqlParser + * does not fully cover: GreptimeDB range queries such as + * {@code SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s'} are rejected by the parser + * although they are ordinary reads. + * + *

So the properties this mode has to guarantee are established without the parser: + * statements are counted by scanning outside string literals and comments, the leading + * keyword decides whether the statement reads, and no word that only a write contains may + * appear anywhere. All three are dialect independent, and the last one is what covers a + * write nested where the scan has no structure to reason about, as in + * {@code WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x}. + * + *

The parser then runs as a second and precise opinion over the whole tree. A statement + * it cannot parse is still accepted on the scan alone rather than failing a user whose + * dialect is merely richer than the parser. + * @param sql Statement to validate + * @throws SqlSecurityException If the statement writes, or carries more than one statement + */ + private void validateReadOnly(String sql) throws SqlSecurityException { + final StatementShape shape = scan(sql); + if (shape.statementCount() != 1) { + throw new SqlSecurityException("Only a single statement is allowed."); + } + if (shape.writeKeyword() != null) { + throw new SqlSecurityException("'" + shape.writeKeyword() + "' is not allowed, only reads are."); + } + if (!SELECT_KEYWORD.equals(shape.leadingKeyword()) && !WITH_KEYWORD.equals(shape.leadingKeyword())) { + throw new SqlSecurityException("Only SELECT statements are allowed."); + } + + final Statement statement; try { - statement = CCJSqlParserUtil.parse(sql); + statement = parseSingleStatement(sql); } catch (JSQLParserException e) { - log.warn("Failed to parse SQL: {}", sql, e); + // Debug, not warn: a dialect the parser does not cover is the expected case here. + // This validation runs on every evaluation of every rule that uses one. + log.debug("SQL not understood by the parser, accepted as a read on the statement scan: {}", sql, e); + return; + } + + if (!(statement instanceof Select)) { + throw new SqlSecurityException("Only SELECT statements are allowed."); + } + assertNothingWrites(statement); + } + + /** + * Walks the whole statement rather than its outermost node, because a write hides at any + * depth: {@code SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu} puts the write in a + * branch of a set operation, and {@code SELECT * FROM (SELECT * INTO backup FROM cpu) t} + * puts it in a subquery, so an outermost node that is a plain select proves nothing. + * + *

Any other failure of the walk is a rejection too. A data modifying cte makes + * JSqlParser's own finder cast a {@code ParenthesedDelete} to a {@code ParenthesedSelect}, + * and a walk that ended in an exception established nothing about the statement. + * @param statement Parsed statement to walk + * @throws SqlSecurityException If any part of the statement writes, or could not be walked + */ + private void assertNothingWrites(Statement statement) throws SqlSecurityException { + try { + new ReadOnlyStatementFinder().getTableList(statement); + } catch (SecurityViolationException e) { + throw new SqlSecurityException(e.getMessage()); + } catch (RuntimeException e) { + log.debug("Failed to walk SQL, so nothing about it is established: {}", statement, e); + throw new SqlSecurityException("SQL structure could not be verified as a read."); + } + } + + private void validateAgainstWhitelist(String sql) throws SqlSecurityException { + final Statement statement; + try { + statement = parseSingleStatement(sql); + } catch (JSQLParserException e) { + log.debug("Failed to parse SQL: {}", sql, e); throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); } @@ -74,14 +213,18 @@ public void validate(String sql) throws SqlSecurityException { throw new SqlSecurityException("Only SELECT statements are allowed."); } + // The whitelist is about which tables a statement may touch, so on its own it lets + // "select * into backup from hertzbeat_logs" through: every table it names is allowed + assertNothingWrites(statement); + // Check for CTE at top level if (select.getWithItemsList() != null && !select.getWithItemsList().isEmpty()) { throw new SqlSecurityException("CTE (WITH clause) is not allowed"); } // Use custom TablesNamesFinder that throws on dangerous structures - SecurityTablesNamesFinder finder = new SecurityTablesNamesFinder(); - List tables; + final SecurityTablesNamesFinder finder = new SecurityTablesNamesFinder(); + final List tables; try { tables = finder.getTableList(statement); } catch (SecurityViolationException e) { @@ -91,6 +234,179 @@ public void validate(String sql) throws SqlSecurityException { validateTables(tables); } + /** + * @param sql Statement to parse + * @return The only statement the string carries + * @throws JSQLParserException If the string does not parse + * @throws SqlSecurityException If the string carries more than one statement + */ + private Statement parseSingleStatement(String sql) throws JSQLParserException, SqlSecurityException { + final Statements statements = CCJSqlParserUtil.parseStatements(sql); + // ParseStatements rather than parse: parse() returns only the first statement. + // Otherwise, "select 1; drop table x" would validate as a plain select. + // The caller would still hand the whole string to the database. + if (statements.getStatements().size() != 1) { + throw new SqlSecurityException("Only a single statement is allowed."); + } + return statements.getStatements().get(0); + } + + /** + * What a statement string looks like from outside any sql dialect. + * @param statementCount Statements the string carries, a trailing semicolon not counting as one + * @param leadingKeyword First word of the first statement, upper cased, empty when it does not start with a word + * @param writeKeyword First word from {@link #WRITE_KEYWORDS} found anywhere, null when there is none + */ + private record StatementShape(int statementCount, String leadingKeyword, String writeKeyword) { + } + + /** + * Counts the statements a string carries and reads the word it opens with, skipping over + * string literals, quoted identifiers and comments so that a semicolon inside them is not + * mistaken for a statement separator. + * + *

A backslash is not treated as an escape, because assuming it escapes the closing + * quote in a dialect where it does not would let {@code 'a\'; DROP TABLE t} hide a second + * statement inside what this scan thinks is one literal. Not assuming it costs at worst a + * rejection of a statement that uses backslash escapes, which errs the safe way. + * @param sql Statement string to scan + * @return The shape of the string + * @throws SqlSecurityException If a literal or a block comment is left open + */ + private StatementShape scan(String sql) throws SqlSecurityException { + int statementCount = 0; + boolean statementHasContent = false; + String leadingKeyword = ""; + String writeKeyword = null; + int index = 0; + while (index < sql.length()) { + final char current = sql.charAt(index); + if (current == '-' && index + 1 < sql.length() && sql.charAt(index + 1) == '-') { + final int lineEnd = sql.indexOf('\n', index); + index = lineEnd < 0 ? sql.length() : lineEnd + 1; + } else if (current == '/' && index + 1 < sql.length() && sql.charAt(index + 1) == '*') { + final int commentEnd = sql.indexOf("*/", index + 2); + if (commentEnd < 0) { + throw new SqlSecurityException("Unterminated block comment."); + } + index = commentEnd + 2; + } else if (current == '$') { + rejectDollarQuote(sql, index); + statementHasContent = true; + index++; + } else if (current == '\'' || current == '"' || current == '`') { + index = skipQuoted(sql, index, current); + statementHasContent = true; + } else if (current == ';') { + if (statementHasContent) { + statementCount++; + } + statementHasContent = false; + index++; + } else if (Character.isLetter(current) || current == '_') { + // Read the whole word and step past it. + // Match a write keyword only on its own, never inside an identifier like delete_count. + final int wordEnd = wordEnd(sql, index); + final String word = sql.substring(index, wordEnd).toUpperCase(Locale.ROOT); + if (statementCount == 0 && !statementHasContent) { + leadingKeyword = word; + } + if (writeKeyword == null && WRITE_KEYWORDS.contains(word)) { + writeKeyword = word; + } + statementHasContent = true; + index = wordEnd; + } else { + if (!Character.isWhitespace(current)) { + statementHasContent = true; + } + index++; + } + } + if (statementHasContent) { + statementCount++; + } + return new StatementShape(statementCount, leadingKeyword, writeKeyword); + } + + /** + * @param sql Statement string being scanned + * @param start Index of the opening quote + * @param quote Quote character to close on, a doubled one being an escaped quote rather than the close + * @return Index just past the closing quote + * @throws SqlSecurityException If the quote is never closed + */ + private int skipQuoted(String sql, int start, char quote) throws SqlSecurityException { + int index = start + 1; + while (index < sql.length()) { + if (sql.charAt(index) == quote) { + if (index + 1 < sql.length() && sql.charAt(index + 1) == quote) { + index += 2; + continue; + } + return index + 1; + } + index++; + } + throw new SqlSecurityException("Unterminated quoted literal."); + } + + /** + * Rejects a PostgreSQL dollar-quoted literal, {@code $$...$$} or {@code $tag$...$tag$}. + * + *

Comment markers and semicolons inside such a literal are data, so a scan that reads + * them as syntax loses the rest of the input: {@code SELECT $$--$$; DROP TABLE cpu} looks + * like one statement once the {@code --} is taken for a comment. + * + *

Rejecting rather than skipping, because skipping would be the same assumption in + * reverse. A dialect without dollar quoting means the semicolon inside one really does + * separate statements, and a scan that skipped the literal would be the thing hiding + * them. Rejecting needs no assumption either way: a dialect that has dollar quoting is + * refused, and one that does not was going to fail on the syntax regardless. No read of a + * metric table spells anything this way. + * @param sql Statement string being scanned + * @param start Index of the dollar sign + * @throws SqlSecurityException If a dollar-quoted literal opens here + */ + private void rejectDollarQuote(String sql, int start) throws SqlSecurityException { + int tagEnd = start + 1; + if (tagEnd >= sql.length()) { + return; + } + final char firstTagCharacter = sql.charAt(tagEnd); + if (firstTagCharacter != '$' + && !Character.isLetter(firstTagCharacter) + && firstTagCharacter != '_') { + // a positional parameter such as $1, or a dollar sign that is just a character + return; + } + while (tagEnd < sql.length() + && (Character.isLetterOrDigit(sql.charAt(tagEnd)) || sql.charAt(tagEnd) == '_')) { + tagEnd++; + } + if (tagEnd >= sql.length() || sql.charAt(tagEnd) != '$') { + return; + } + throw new SqlSecurityException("Dollar-quoted literals are not allowed."); + } + + /** + * @param sql Statement string being scanned + * @param start Index of the first character of a word + * @return Index just past the word, digits and underscores counting as part of it so that + * {@code delete_count} is one word rather than a {@code delete} followed by a remainder. + * A dollar sign ends the word, so that a literal opening right after an identifier is + * still seen by {@link #rejectDollarQuote} + */ + private int wordEnd(String sql, int start) { + int index = start; + while (index < sql.length() + && (Character.isLetterOrDigit(sql.charAt(index)) || sql.charAt(index) == '_')) { + index++; + } + return index; + } + private void validateTables(List tables) throws SqlSecurityException { if (CollectionUtils.isEmpty(tables)) { return; @@ -121,6 +437,46 @@ private static class SecurityViolationException extends RuntimeException { } } + private enum ValidationPolicy { + WHITELISTED_SELECT, + READ_ONLY + } + + /** + * Walks a statement and throws as soon as it finds a part of it that writes, at any depth. + */ + private static class ReadOnlyStatementFinder extends TablesNamesFinder { + + @Override + public Void visit(PlainSelect plainSelect, Object context) { + // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read. + if (!CollectionUtils.isEmpty(plainSelect.getIntoTables())) { + throw new SecurityViolationException("SELECT ... INTO is not allowed."); + } + return super.visit(plainSelect, context); + } + + @Override + public Void visit(Delete delete, Object context) { + throw new SecurityViolationException("DELETE is not allowed, only reads are."); + } + + @Override + public Void visit(Insert insert, Object context) { + throw new SecurityViolationException("INSERT is not allowed, only reads are."); + } + + @Override + public Void visit(Update update, Object context) { + throw new SecurityViolationException("UPDATE is not allowed, only reads are."); + } + + @Override + public Void visit(Merge merge, Object context) { + throw new SecurityViolationException("MERGE is not allowed, only reads are."); + } + } + /** * Custom TablesNamesFinder that throws exceptions on dangerous SQL structures. * Extends TablesNamesFinder with proper generic type to avoid raw type warnings. diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index 782ee313833..de4500bc064 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -264,4 +264,219 @@ void testBypassWithMathOperations() { assertThrows(SqlSecurityException.class, () -> validator.validate( "SELECT * FROM hertzbeat_logs WHERE id = 1 + (SELECT id FROM secret_table)")); } + + /** + * `CCJSqlParserUtil.parse` returns the first statement and silently discards the rest, + * so a stacked statement used to validate as a plain select while the caller still + * handed the whole string to the database. + */ + @Test + void testStackedStatementIsRejected() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT * FROM hertzbeat_logs; DROP TABLE hertzbeat_logs")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT 1; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT $$--$$; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT $body$--$body$; DROP TABLE cpu")); + } + + @Test + void testTrailingSemicolonIsStillAcceptedAsOneStatement() { + assertDoesNotThrow(() -> validator.validate("SELECT * FROM hertzbeat_logs ; ")); + } + + @Test + void testSelectOnlyRejectsWrites() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DELETE FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("INSERT INTO cpu VALUES (1)")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("UPDATE cpu SET value = 1")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("TRUNCATE TABLE cpu")); + } + + /** + * Metric tables are created per metric on demand, so this mode constrains what a + * statement may do, not which table it may touch. + */ + @Test + void testSelectOnlyAcceptsAnyTableAndNestedReads() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM any_metric_table")); + assertDoesNotThrow(() -> selectOnly.validate( + "SELECT value FROM cpu WHERE host = (SELECT host FROM hosts LIMIT 1)")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT a FROM t1 UNION ALL SELECT b FROM t2")); + } + + /** + * The only sql executor today talks to GreptimeDB, whose range query syntax JSqlParser + * cannot parse. These are ordinary reads and used to run, so read only mode has to keep + * accepting them rather than turn a richer dialect into a rule that no longer fires. + */ + @Test + void testSelectOnlyAcceptsDialectTheParserDoesNotUnderstand() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate( + "SELECT ts, avg(value) RANGE '10s' FROM cpu ALIGN '5s' FILL LINEAR")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu ALIGN '5s'")); + } + + /** + * Accepting what the parser cannot read must not become a way through: the statement + * count and the leading keyword are established without the parser, so they still hold + * for a string it never understood. + */ + @Test + void testUnparsableStatementMustStillBeOneRead() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT avg(value) RANGE '10s' FROM cpu ALIGN '5s'; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("TQL EVAL (0, 10, '5s') sum(cpu)")); + } + + /** + * A semicolon that is data or commentary is not a statement separator, and a statement + * scan that cannot tell the difference would reject ordinary queries. + */ + @Test + void testSemicolonInsideLiteralOrCommentDoesNotSplitTheStatement() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'a; DROP TABLE cpu'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'it''s; fine'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu -- ; DROP TABLE cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu /* ; DROP TABLE cpu */ LIMIT 1")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM \"cpu;usage\"")); + } + + @Test + void testUnclosedLiteralOrCommentIsRejected() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'open")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu /* open")); + } + + /** + * A dollar-quoted literal is refused outright rather than skipped over, so that neither + * answer to "does this dialect have dollar quoting" can hide a statement. + * + *

Skipping would lose a stacked statement to a dialect that has it, since the comment + * marker in {@code SELECT $$--$$; DROP TABLE cpu} is data rather than a comment. Skipping + * would equally lose one to a dialect that does not, since the semicolon in + * {@code SELECT 1 $$;DROP TABLE cpu$$} really does separate statements there. + */ + @Test + void testDollarQuotedLiteralIsRejectedRatherThanSkipped() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$--$$; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT 1 $$;DROP TABLE cpu$$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT 1 $t$;DROP TABLE cpu$t$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$a; -- DROP TABLE cpu$$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $body$a; -- DROP TABLE cpu$body$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$open")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $body$open")); + // a literal opening right after an identifier is still a literal + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT a$$b;c$$ FROM cpu")); + } + + /** + * A dollar sign that opens nothing is an ordinary character, so reads keep working. + */ + @Test + void testLoneDollarSignIsNotTreatedAsLiteral() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT $1 FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT a$b FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE cost = '$5; x'")); + } + + /** + * `SELECT ... INTO` parses as a select but writes a table in the dialects that support it. + */ + @Test + void testSelectOnlyRejectsSelectInto() { + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly() + .validate("SELECT * INTO backup FROM cpu")); + } + + @Test + void testSelectOnlyAcceptsCte() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("WITH x AS (SELECT 1 AS v) SELECT * FROM x")); + assertDoesNotThrow(() -> selectOnly.validate( + "WITH x AS (SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s') SELECT * FROM x")); + } + + /** + * An outermost node that is a plain select proves nothing about the rest of the tree: a + * write hides in a cte, in a branch of a set operation, or in a subquery, and JSqlParser + * reports the outermost node of all three as a select. + */ + @Test + void testSelectOnlyRejectsWritesNestedInsideReads() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (INSERT INTO cpu VALUES (1) RETURNING *) SELECT * FROM x")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (SELECT id FROM t) DELETE FROM cpu WHERE id IN (SELECT id FROM x)")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM cpu UNION SELECT * INTO backup FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM (SELECT * INTO backup FROM cpu) t")); + } + + /** + * The nested writes above have to stay rejected when the parser cannot read the dialect + * and there is no tree to walk, which is the case the whole read only mode exists for. + */ + @Test + void testNestedWritesStayRejectedWithoutTheParser() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM (DELETE FROM cpu RETURNING *) t ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (DELETE FROM cpu RETURNING *) SELECT avg(v) RANGE '10s' FROM x ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * INTO backup FROM cpu ALIGN '5s'")); + } + + /** + * The word scan matches whole words only, so ordinary reads whose identifiers or functions + * merely contain one keep working. An identifier that collides outright can be quoted. + */ + @Test + void testWriteWordScanDoesNotCatchOrdinaryReads() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT delete_count, insert_rate FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT truncate(value, 2) FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT replace(msg, 'a', 'b') FROM logs")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'drop table cpu'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT \"drop\" FROM cpu")); + } + + /** + * The whitelist says which tables a statement may touch, so on its own it lets a write + * through as long as every table it names is allowed. + */ + @Test + void testWhitelistModeRejectsSelectIntoOnAnAllowedTable() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT * INTO backup FROM hertzbeat_logs")); + } + + /** + * The whitelisting mode needs the parse tree to enumerate table names, so unlike read + * only mode it has nothing to fall back on and keeps rejecting what it cannot parse. + */ + @Test + void testWhitelistModeStillRejectsWhatItCannotParse() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT avg(value) RANGE '10s' FROM hertzbeat_logs ALIGN '5s'")); + } }