From dc52ca6d9199655ed59e771d790a09b43fa85701 Mon Sep 17 00:00:00 2001 From: Dominik Zalewski Date: Mon, 16 Oct 2023 16:49:32 +0200 Subject: [PATCH 1/3] Simplify how SQL injection is detected when using query.comment-format Instead of failing the query in case a character NOT in the allowlist is detected, now we search for occurence of the closing comment '*/' and only then fail the query. --- .../logging/SessionInterpolatedValues.java | 32 ++----------------- .../FormatBasedRemoteQueryModifier.java | 13 +++++--- .../TestFormatBasedRemoteQueryModifier.java | 30 +++++++++++------ 3 files changed, 30 insertions(+), 45 deletions(-) diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/logging/SessionInterpolatedValues.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/logging/SessionInterpolatedValues.java index ca7c0fed5345..5ac804179577 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/logging/SessionInterpolatedValues.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/logging/SessionInterpolatedValues.java @@ -17,18 +17,14 @@ import io.trino.spi.connector.ConnectorSession; import java.util.function.Function; -import java.util.function.Predicate; -import java.util.regex.Pattern; - -import static java.util.Objects.requireNonNull; public enum SessionInterpolatedValues implements InterpolatedValue { QUERY_ID(ConnectorSession::getQueryId), - SOURCE(new SanitizedValuesProvider(session -> session.getSource().orElse(""), "$SOURCE")), + SOURCE(session -> session.getSource().orElse("")), USER(ConnectorSession::getUser), - TRACE_TOKEN(new SanitizedValuesProvider(session -> session.getTraceToken().orElse(""), "$TRACE_TOKEN")); + TRACE_TOKEN(session -> session.getTraceToken().orElse("")); private final Function valueProvider; @@ -42,28 +38,4 @@ public String value(ConnectorSession session) { return valueProvider.apply(session); } - - static class SanitizedValuesProvider - implements Function - { - private static final Predicate VALIDATION_MATCHER = Pattern.compile("^[\\w_-]*$").asMatchPredicate(); - private final Function valueProvider; - private final String name; - - private SanitizedValuesProvider(Function valueProvider, String name) - { - this.valueProvider = requireNonNull(valueProvider, "valueProvider is null"); - this.name = requireNonNull(name, "name is null"); - } - - @Override - public String apply(ConnectorSession session) - { - String value = valueProvider.apply(session); - if (VALIDATION_MATCHER.test(value)) { - return value; - } - throw new SecurityException("Passed value %s as %s does not meet security criteria. It can contain only letters, digits, underscores and hyphens".formatted(value, name)); - } - } } diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/FormatBasedRemoteQueryModifier.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/FormatBasedRemoteQueryModifier.java index 2c5163a84f81..360e05927451 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/FormatBasedRemoteQueryModifier.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/logging/FormatBasedRemoteQueryModifier.java @@ -39,11 +39,14 @@ public FormatBasedRemoteQueryModifier(FormatBasedRemoteQueryModifierConfig confi @Override public String apply(ConnectorSession session, String query) { - try { - return query + " /*" + interpolator.interpolate(session) + "*/"; - } - catch (SecurityException e) { - throw new TrinoException(JDBC_NON_TRANSIENT_ERROR, e.getMessage()); + return query + " /*" + checkForSqlInjection(interpolator.interpolate(session)) + "*/"; + } + + private String checkForSqlInjection(String sql) + { + if (sql.contains("*/")) { + throw new TrinoException(JDBC_NON_TRANSIENT_ERROR, "Rendering metadata using 'query.comment-format' does not meet security criteria: " + sql); } + return sql; } } diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestFormatBasedRemoteQueryModifier.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestFormatBasedRemoteQueryModifier.java index 64b729dd6bcd..5f47dbc96b0e 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestFormatBasedRemoteQueryModifier.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/logging/TestFormatBasedRemoteQueryModifier.java @@ -79,23 +79,18 @@ public void testForSQLInjectionsByTraceToken() assertThatThrownBy(() -> modifier.apply(connectorSession, "SELECT * from USERS")) .isInstanceOf(TrinoException.class) - .hasMessage("Passed value */; DROP TABLE TABLE_A; /* as $TRACE_TOKEN does not meet security criteria. It can contain only letters, digits, underscores and hyphens"); + .hasMessageMatching("Rendering metadata using 'query.comment-format' does not meet security criteria: Query=.* Execution for user=Alice with source=source ttoken=\\*/; DROP TABLE TABLE_A; /\\*"); } @Test public void testForSQLInjectionsBySource() { - TestingConnectorSession connectorSession = TestingConnectorSession.builder() - .setTraceToken("trace_token") - .setSource("*/; DROP TABLE TABLE_A; /*") - .setIdentity(ConnectorIdentity.ofUser("Alice")) - .build(); + testForSQLInjectionsBySource("*/; DROP TABLE TABLE_A; /*"); + testForSQLInjectionsBySource("Prefix */; DROP TABLE TABLE_A; /*"); + testForSQLInjectionsBySource(""" - FormatBasedRemoteQueryModifier modifier = createRemoteQueryModifier("Query=$QUERY_ID Execution for user=$USER with source=$SOURCE ttoken=$TRACE_TOKEN"); - assertThatThrownBy(() -> modifier.apply(connectorSession, "SELECT * from USERS")) - .isInstanceOf(TrinoException.class) - .hasMessage("Passed value */; DROP TABLE TABLE_A; /* as $SOURCE does not meet security criteria. It can contain only letters, digits, underscores and hyphens"); + Multiline */; DROP TABLE TABLE_A; /*"""); } @Test @@ -180,6 +175,21 @@ private void testFormatWithValidValues(String value) .isEqualTo("SELECT * FROM USERS /*source=%1$s ttoken=%1$s*/".formatted(value)); } + private void testForSQLInjectionsBySource(String sqlInjection) + { + TestingConnectorSession connectorSession = TestingConnectorSession.builder() + .setTraceToken("trace_token") + .setSource(sqlInjection) + .setIdentity(ConnectorIdentity.ofUser("Alice")) + .build(); + + FormatBasedRemoteQueryModifier modifier = createRemoteQueryModifier("Query=$QUERY_ID Execution for user=$USER with source=$SOURCE ttoken=$TRACE_TOKEN"); + + assertThatThrownBy(() -> modifier.apply(connectorSession, "SELECT * from USERS")) + .isInstanceOf(TrinoException.class) + .hasMessageContaining("Rendering metadata using 'query.comment-format' does not meet security criteria: Query="); + } + private static FormatBasedRemoteQueryModifier createRemoteQueryModifier(String commentFormat) { return new FormatBasedRemoteQueryModifier(new FormatBasedRemoteQueryModifierConfig().setFormat(commentFormat)); From 7d8cc86347a6306abd9455f5faaac08d758bda69 Mon Sep 17 00:00:00 2001 From: Dominik Zalewski Date: Mon, 23 Oct 2023 09:52:12 +0200 Subject: [PATCH 2/3] for ci From ee2e8c5525486e646ad31c61a7a075361270cd89 Mon Sep 17 00:00:00 2001 From: Dominik Zalewski Date: Mon, 23 Oct 2023 09:53:34 +0200 Subject: [PATCH 3/3] for ci