diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java index f5a0b22523e..7acbc6a4650 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java @@ -9,7 +9,9 @@ import org.springframework.util.StringUtils; import java.util.List; +import java.util.Locale; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static com.google.common.primitives.Ints.tryParse; @@ -86,10 +88,67 @@ public List query(String filter, String sortBy, boolean ascending, String zon } private String getQuerySQL(SearchQueryConverter.ProcessedFilter where) { + // The SQL fragment comes from the configured SearchQueryConverter (default + // SimpleSearchQueryConverter; replaceable via setQueryConverter). Every shipped + // implementation validates attribute names against a hardcoded allow-list and + // binds every user-supplied value as a named parameter (see e.g. + // SimpleSearchQueryConverter#whereClauseFromFilter and #comparisonClause). + // assertSafeGeneratedSql adds a defense-in-depth check on the resulting fragment + // so any future converter that violates that contract still cannot inject SQL. + String sqlFragment = where.getSql(); + assertSafeGeneratedSql(sqlFragment); + if (where.hasOrderBy()) { - return getBaseSqlQuery() + " where (" + where.getSql().replace(ORDER_BY, ")" + ORDER_BY); + return getBaseSqlQuery() + " where (" + sqlFragment.replace(ORDER_BY, ")" + ORDER_BY); } else { - return getBaseSqlQuery() + " where (" + where.getSql() + ")"; + return getBaseSqlQuery() + " where (" + sqlFragment + ")"; + } + } + + // Matches a leading DML/DDL keyword followed by any whitespace (including \t, \n) or + // a "(", which together cover the realistic ways such a keyword could be smuggled past + // a literal-space prefix check (e.g. "select\tfoo", "select(", "SELECT\nfoo"). + private static final Pattern DISALLOWED_LEADING_CLAUSE = Pattern.compile( + "^(select|insert|update|delete|drop|alter|create|truncate|merge|union|grant|revoke|exec|execute|call|with|replace|rename|comment)(\\s|\\().*", + Pattern.CASE_INSENSITIVE | Pattern.DOTALL); + + // Strips leading whitespace and any number of leading "(" so the keyword guard above + // also catches parenthesis-wrapped bypasses such as "(select 1)" or "((select 1))". + private static final Pattern LEADING_PARENS_OR_WHITESPACE = Pattern.compile("^[\\s(]+"); + + protected static void assertSafeGeneratedSql(String sqlFragment) { + if (!StringUtils.hasText(sqlFragment)) { + throw new IllegalArgumentException("Invalid filter: empty SQL fragment"); + } + + String normalized = sqlFragment.toLowerCase(Locale.ROOT); + if (normalized.contains(";") + || normalized.contains("--") + || normalized.contains("#") + || normalized.contains("/*") + || normalized.contains("*/")) { + throw new IllegalArgumentException("Invalid filter: disallowed SQL token in generated query"); + } + + // The caller wraps the fragment as `where (` + fragment + `)`. A fragment whose + // first non-whitespace character is `)` would break out of that wrapper (e.g. + // `) or 1=1 or (`) and change the surrounding query logic without using any of + // the tokens above. Reject it explicitly. + String leadingTrimmed = normalized.stripLeading(); + if (leadingTrimmed.startsWith(")")) { + throw new IllegalArgumentException("Invalid filter: unexpected leading token in generated query"); + } + + String trimmed = LEADING_PARENS_OR_WHITESPACE.matcher(normalized).replaceFirst(""); + // Also reject the extended wrapper-breakout where one or more leading `(` are + // immediately followed by `)` (e.g. `"()) or 1=1 or (1=1"` or `"(()) or ..."`): + // after stripping leading whitespace and `(`, the next non-whitespace token is `)`, + // which closes the caller's `where (` wrapper one paren-level too early. + if (trimmed.startsWith(")")) { + throw new IllegalArgumentException("Invalid filter: unexpected leading token in generated query"); + } + if (DISALLOWED_LEADING_CLAUSE.matcher(trimmed).matches()) { + throw new IllegalArgumentException("Invalid filter: unexpected SQL clause in generated query"); } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java index d82b31e9774..02e7b392fef 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java @@ -200,6 +200,11 @@ public List retrieveByScimFilterOnlyActive( // build WHERE clause final ProcessedFilter where = joinConverter.convert(filter, sortBy, ascending, zoneId); final String whereClauseScimFilter = where.getSql(); + // The SQL fragment comes from the configured SearchQueryConverter (here joinConverter, + // see AbstractQueryable#getQuerySQL for the full converter-contract justification); + // assertSafeGeneratedSql adds a defense-in-depth check so this direct concatenation + // is protected by the same guard as the inherited query() path. + assertSafeGeneratedSql(whereClauseScimFilter); String whereClause = "idp.active is true and ("; if (where.hasOrderBy()) { whereClause += whereClauseScimFilter.replace(ProcessedFilter.ORDER_BY, ")" + ProcessedFilter.ORDER_BY); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java new file mode 100644 index 00000000000..72cef016609 --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryableSqlGuardTests.java @@ -0,0 +1,225 @@ +package org.cloudfoundry.identity.uaa.resources.jdbc; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.jdbc.core.RowMapper; +import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Focused tests for {@link AbstractQueryable#assertSafeGeneratedSql(String)} — the + * defense-in-depth guard added to address CodeQL alert #27 ("Query built from + * user-controlled sources"). These tests verify that unsafe constructs are rejected + * and that typical generated SCIM filter fragments are accepted, so the guard cannot + * be silently relaxed in the future. + */ +class AbstractQueryableSqlGuardTests { + + /** + * Tiny subclass that exposes the {@code protected static} guard for direct testing. + * No state, no behavior — just a hook to invoke the validator. + */ + private static final class GuardHarness extends AbstractQueryable { + private GuardHarness() { + super((NamedParameterJdbcTemplate) null, (JdbcPagingListFactory) null, (RowMapper) null); + } + + static void invoke(String sqlFragment) { + assertSafeGeneratedSql(sqlFragment); + } + + @Override + protected String getBaseSqlQuery() { + return ""; + } + + @Override + protected String getTableName() { + return ""; + } + + @Override + protected void validateOrderBy(String orderBy) { + // no-op + } + } + + // ---------- Empty / blank fragments ---------- + + @ParameterizedTest + @ValueSource(strings = {"", " ", "\t", "\n"}) + void rejectsEmptyOrBlankFragments(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("empty SQL fragment"); + } + + @Test + void rejectsNullFragment() { + assertThatThrownBy(() -> GuardHarness.invoke(null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("empty SQL fragment"); + } + + // ---------- Statement separators / comment tokens ---------- + + @ParameterizedTest + @ValueSource(strings = { + "username = :__value_0; drop table users", // statement separator + "username = :__value_0 -- and 1=1", // ANSI line comment + "username = :__value_0 # mysql line comment", // MySQL # line comment + "username = :__value_0 /* block comment */", // start of block comment + "username = :__value_0 */ trailing" // end of block comment + }) + void rejectsDisallowedTokens(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("disallowed SQL token"); + } + + // ---------- Leading DML/DDL clauses (the keyword guard) ---------- + + @ParameterizedTest + @ValueSource(strings = { + "select * from users", + "SELECT * from users", + "insert into users values (1)", + "update users set x = 1", + "delete from users", + "drop table users", + "alter table users add column x", + "create table foo(x int)", + "truncate users", + "merge into users", + "union select 1", + "with x as (select 1) select * from x", + "grant all to public", + "revoke select on users", + "exec something", + "execute something", + "call proc()", + "replace into users", + "rename table users to bar", + "comment on column foo is 'x'" + }) + void rejectsLeadingDmlDdlKeywordWithSpace(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + + /** + * Bypasses raised by Copilot review: the prior implementation only blocked + * "{keyword} " (literal space). These variants must also be rejected. + */ + @ParameterizedTest + @ValueSource(strings = { + "SELECT\tfoo", // tab + "SELECT\nfoo", // newline + "SELECT\rfoo", // carriage return + "SELECT\ffoo", // form feed + "select(1)", // open paren, no whitespace + " select foo", // leading whitespace before keyword + "UPDATE\tusers", // mixed-case + tab + "Drop(table)" // mixed-case + paren + }) + void rejectsKeywordFollowedByWhitespaceVariantsOrParen(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + + /** + * Parenthesis-wrapped bypass raised by Copilot review: a fragment like + * "(select 1)" or "((select 1))" must be rejected — leading whitespace and + * "(" are stripped before the DML/DDL keyword check. + */ + @ParameterizedTest + @ValueSource(strings = { + "(select 1)", + "((select 1))", + "( select 1 )", + "(\tselect 1)", + "( ( select 1 ) )", + " (select 1) ", + "((((delete from users))))", + "(UPDATE users set x=1)", + "(DROP table users)", + "(union select 1)" + }) + void rejectsParenthesisWrappedKeyword(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected SQL clause"); + } + + /** + * Wrapper-breakout bypass raised by Copilot review: the caller composes + * {@code "where (" + fragment + ")"}, so a fragment whose first non-whitespace + * character is {@code ")"} would close the wrapper and inject arbitrary + * boolean logic without using {@code ;}, comments, or DML/DDL keywords. + */ + @ParameterizedTest + @ValueSource(strings = { + ") or 1=1 or (", + ") OR 1=1 OR (", + ")", + " ) or 'a'='a' or (", + "\t) or 1=1 or (", + "\n) or 1=1 or (", + "))) or 1=1 or (((" + }) + void rejectsLeadingCloseParenWrapperBreakout(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected leading token"); + } + + /** + * Extended wrapper-breakout bypass raised by Copilot review: a fragment can also + * close the surrounding {@code "where ("} wrapper early by starting with one or + * more {@code "("} immediately followed by {@code ")"} (e.g. {@code "()) or 1=1 or (1=1"}). + * After stripping leading whitespace and any leading {@code "("}, the next + * non-whitespace token must not be {@code ")"}. + */ + @ParameterizedTest + @ValueSource(strings = { + "()) or 1=1 or (1=1", + "(()) or 1=1 or ((1=1", + "((())) or 1=1", + "( )) or 1=1 or (1=1", // whitespace between ( and ) + "(\t)) or 1=1 or (1=1", // tab between ( and ) + "(\n)) or 1=1 or (1=1", // newline between ( and ) + " ()) or 1=1 or (1=1", // leading whitespace before ( + "())", // bare ()) — closes wrapper twice + // Verbatim variants from the Copilot review + "()) or 1=1 or (", + "( ) or 1=1 or (" + }) + void rejectsParenWrappedCloseParenBreakout(String fragment) { + assertThatThrownBy(() -> GuardHarness.invoke(fragment)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("unexpected leading token"); + } + + // ---------- Typical generated SCIM filter fragments are accepted ---------- + + @ParameterizedTest + @ValueSource(strings = { + // shapes produced by SimpleSearchQueryConverter — values are bound parameters + "username = :__value_0", + "username = :__value_0 and origin = :__value_1", + "(username = :__value_0 or origin = :__value_1) and active = :__value_2", + "lower(username) = lower(:__value_0)", + "username = :__value_0 order by created desc", + // an attribute that happens to start with a keyword-prefix (must not be rejected) + "selectable = :__value_0", + "createdby = :__value_0" + }) + void acceptsTypicalGeneratedFragments(String fragment) { + assertThatCode(() -> GuardHarness.invoke(fragment)).doesNotThrowAnyException(); + } +}