This is an automated email from the ASF dual-hosted git repository. borinquenkid pushed a commit to branch 8.0.x-hibernate7 in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit fcd1a1d233fbba3ca8996b29e2d40543d36cc4dc Author: Walter Duque de Estrada <[email protected]> AuthorDate: Mon Feb 23 16:24:52 2026 -0600 Injection vector in DefaultSchemaHandler fixed --- grails-data-hibernate7/GORM-QUERY-SAFETY-AUDIT.md | 182 ++++++++++++++++++ .../gorm/jdbc/schema/DefaultSchemaHandler.groovy | 22 ++- .../jdbc/schema/DefaultSchemaHandlerSpec.groovy | 206 +++++++++++++++++++++ 3 files changed, 408 insertions(+), 2 deletions(-) diff --git a/grails-data-hibernate7/GORM-QUERY-SAFETY-AUDIT.md b/grails-data-hibernate7/GORM-QUERY-SAFETY-AUDIT.md new file mode 100644 index 0000000000..cc4a86419b --- /dev/null +++ b/grails-data-hibernate7/GORM-QUERY-SAFETY-AUDIT.md @@ -0,0 +1,182 @@ +# GORM Query Safety Audit โ `grails-data-hibernate7` + +> **Scope**: Section 2.6 of `HIBERNATE7-GRAILS8-UPGRADE.md` +> **Priority**: P2 โ SQL injection is OWASP Top-10 #3 +> **Module**: `grails-data-hibernate7-core` + +--- + +## Summary + +| Risk Level | Count | +|------------|-------| +| ๐ด HIGH | 2 | +| ๐ก MEDIUM | 3 | +| ๐ข LOW / Safe | 5 | + +--- + +## ๐ด HIGH โ Real Injection Vectors + + +--- + +### H-2 ยท `HibernateGormStaticApi` โ String-concatenated Queries Bypass GString Parameterization + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy` | +| **Lines** | 238โ244, 248, 233 | + +**Code**: +```groovy +List executeQuery(CharSequence query) // line 238 โ no params at all +Integer executeUpdate(CharSequence query) // line 243 โ no params at all +D find(CharSequence query) // line 248 โ no params at all +List<D> findAll(CharSequence query) // line 233 โ no params at all +``` + +The mitigation in `HibernateHqlQuery.createHqlQuery` (line 128) converts `GString` interpolations to named parameters. **However**, this only applies when the caller passes a Groovy `GString`. If the caller passes an already-evaluated `String` (e.g. from string concatenation), the GString check fires `false` and the raw interpolated string reaches `session.createQuery(hqlToUse)` unchanged. + +**Example of unsafe caller pattern** (application code): +```groovy +// UNSAFE โ userInput is a plain String after concatenation +String q = "from User where name = '" + userInput + "'" +User.find(q) // reaches Hibernate as raw HQL + +// SAFE โ GString is parameterized at GORM layer +User.find("from User where name = ${userInput}") +``` + +**Recommended fix**: +- Deprecate the single-argument `executeQuery(CharSequence)` and `executeUpdate(CharSequence)` overloads; require callers to pass params. +- Add a runtime `String` (non-`GString`) guard that logs a warning when no parameters are supplied and the query contains literals that look like values. + +--- + +## ๐ก MEDIUM โ Potential Risks Requiring Context + +### M-1 ยท `findWithSql` / `findAllWithSql` โ Native SQL Without Enforced Parameterization + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy` | +| **Lines** | 224, 228 | + +```groovy +D findWithSql(CharSequence sql, Map args = Collections.emptyMap()) +List<D> findAllWithSql(CharSequence query, Map args = Collections.emptyMap()) +``` + +Both take a `CharSequence` that is passed to `session.createNativeQuery(hqlToUse, clazz)` with `isNative = true`. GString detection still runs, so `GString` interpolations are converted to named params. But: +1. String-concatenated SQL bypasses the GString check (same as H-2). +2. Native SQL carries no Hibernate type-safety โ column names and table names cannot be parameterized at all. +3. The `Map args` default value is an empty map, making unparameterized calls easy to write accidentally. + +**Recommended fix**: Rename to `findWithNativeSql` / `findAllWithNativeSql` to make the risk explicit. Add a deprecation note requiring `namedParams` or `positionalParams` argument. + +--- + +### M-2 ยท `executeUpdate(CharSequence query)` Single-Argument Overload โ Structural Risk + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy` | +| **Lines** | 243โ244, 549โ555 | + +`executeUpdate` is particularly dangerous because it performs writes. The parameterized overloads exist (`executeUpdate(query, Map params, Map args)` at line 549 and `executeUpdate(query, Collection, Map)` at line 554) but the no-args overload (line 243) remains and is the shortest form, likely the most commonly used. + +Same root cause as H-2 but elevated concern because of the write path. + +**Recommended fix**: `@Deprecated` the single-argument form; point callers to `executeUpdate(query, params, args)`. + +--- + +### M-3 ยท `HibernateGormInstanceApi.nextId()` โ GString with Class Name in HQL + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/HibernateGormInstanceApi.groovy` | +| **Line** | 178 | + +```groovy +String hql = "select max(e.id) from ${persistentEntity.name} e" +``` + +`persistentEntity.name` is a class name from framework metadata โ **not user input** โ so this is not an active injection risk. However: +1. The GString interpolation reaches `buildNamedParameterQueryFromGString` which tries to bind `persistentEntity.name` as a named parameter (`:p0`), producing malformed HQL: `select max(e.id) from :p0 e`. +2. In practice this does not crash becauseโฆ wait โ class names are structural. This should be a plain `String.format` or concatenation, not a GString, to avoid confusing the parameterizer. + +**Recommended fix**: Change to plain concatenation: +```groovy +String hql = "select max(e.id) from " + persistentEntity.name + " e" +``` + +--- + +## ๐ข SAFE โ Verified Parameterized Paths + +### S-1 ยท `HibernateHqlQuery.buildNamedParameterQueryFromGString` โ GString Mitigation (WORKS) + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/query/HibernateHqlQuery.java` | +| **Lines** | 97โ112, 128โ130 | + +GString interpolations are converted to named parameters before the query string reaches Hibernate. Each `${value}` becomes `:p0`, `:p1`, etc. and is bound via `setParameter()`. This correctly prevents injection when callers use GString syntax. + +--- + +### S-2 ยท `HibernateSession.deleteAll` / `updateAll` โ JPA Criteria Builder (SAFE) + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java` | +| **Lines** | 96โ128, 137โ179 | + +Both methods use `JpaQueryBuilder` to construct parameterized HQL. All values are bound via `query.setParameter(JpaQueryBuilder.PARAMETER_NAME_PREFIX + (i+1), parameters.get(i))`. No user-controlled strings are concatenated into the query. + +--- + +### S-3 ยท `HibernateQuery` / `JpaCriteriaQueryCreator` โ JPA Criteria API (SAFE) + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/query/HibernateQuery.java`, `JpaCriteriaQueryCreator.java` | + +Query construction goes through the JPA `CriteriaBuilder` API entirely. Values are passed as typed `ParameterExpression` objects. No string interpolation. + +--- + +### S-4 ยท `HibernateHqlQuery` โ `"from " + clazz.getName()` (SAFE) + +| Field | Value | +|-------|-------| +| **File** | `core/src/main/groovy/org/grails/orm/hibernate/query/HibernateHqlQuery.java` | +| **Line** | 146 | + +```java +q = session.createQuery("from " + clazz.getName(), clazz); +``` + +`clazz` is derived from `PersistentEntity.getJavaClass()` โ a framework-managed class reference, not user input. + +--- + +### S-5 ยท Dynamic Finders โ GORM Criteria API (SAFE by design) + +Dynamic finders (`findByName`, `findAllByTitleAndAuthor`, etc.) are translated to typed criteria queries via the GORM query parser. All values arrive as method arguments and are bound as typed parameters. No string interpolation occurs. + +--- + +## Action Items for Grails 8 + +| ID | Action | Target | Priority | +|----|--------|--------|----------| +| A-1 | Validate / quote schema name in `DefaultSchemaHandler.useSchema` and `createSchema` using JDBC identifier quoting | `DefaultSchemaHandler.groovy` | P1 (H-1) | +| A-2 | Deprecate `executeQuery(CharSequence)` and `executeUpdate(CharSequence)` single-arg overloads; require parameterized forms | `HibernateGormStaticApi.groovy` | P2 (H-2) | +| A-3 | Add runtime warning when a plain `String` (not `GString`) is passed to a no-param query API | `HibernateHqlQuery.java` | P2 (H-2) | +| A-4 | Rename `findWithSql` / `findAllWithSql` to `findWithNativeSql` / `findAllWithNativeSql`; deprecate old names | `HibernateGormStaticApi.groovy` | P2 (M-1) | +| A-5 | Change `nextId()` HQL from GString to plain concatenation to avoid accidental parameterization of class name | `HibernateGormInstanceApi.groovy:178` | P3 (M-3) | +| A-6 | Add Groovy AST transform or type-checking extension emitting a compile-time warning for `GString` passed to `executeQuery` / `executeUpdate` / `find` / `findAll` (Groovy 5 feature) | New AST transform | P2 (8.1+) | +| A-7 | Document safe query patterns in GORM reference guide: prefer named params, prefer criteria API, never concatenate user input | GORM docs | P2 | diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy index 1ce878dc21..964e957180 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy +++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandler.groovy @@ -55,7 +55,7 @@ class DefaultSchemaHandler implements SchemaHandler { @Override void useSchema(Connection connection, String name) { - String useStatement = String.format(useSchemaStatement, name) + String useStatement = String.format(useSchemaStatement, quoteName(connection, name)) log.debug('Executing SQL Set Schema Statement: {}', useStatement) connection .createStatement() @@ -69,13 +69,31 @@ class DefaultSchemaHandler implements SchemaHandler { @Override void createSchema(Connection connection, String name) { - String schemaCreateStatement = String.format(createSchemaStatement, name) + String schemaCreateStatement = String.format(createSchemaStatement, quoteName(connection, name)) log.debug('Executing SQL Create Schema Statement: {}', schemaCreateStatement) connection .createStatement() .execute(schemaCreateStatement) } + /** + * Quotes a schema/catalog identifier using the JDBC-reported identifier quote character so + * that schema names are never spliced as raw SQL tokens. Any embedded occurrences of the + * quote character itself are stripped from the name to prevent escaping the enclosure. + * <p> + * If the driver reports {@code " "} (space) as the quote string โ meaning identifier quoting + * is not supported โ the name is returned as-is (preserving existing behaviour). + */ + protected static String quoteName(Connection connection, String name) { + String q = connection.metaData.identifierQuoteString + if (q == null || q.trim().isEmpty()) { + return name + } + // Remove every occurrence of the quote char inside the name to prevent breakout + String sanitized = name.replace(q, '') + return "${q}${sanitized}${q}" + } + @Override Collection<String> resolveSchemaNames(DataSource dataSource) { Collection<String> schemaNames = [] diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandlerSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandlerSpec.groovy new file mode 100644 index 0000000000..4f153a05e6 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/schema/DefaultSchemaHandlerSpec.groovy @@ -0,0 +1,206 @@ +/* + * 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 + * + * https://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.grails.datastore.gorm.jdbc.schema + +import java.sql.Connection +import java.sql.DatabaseMetaData +import java.sql.Statement + +import spock.lang.Specification +import spock.lang.Unroll + +/** + * Unit tests for {@link DefaultSchemaHandler}. + * + * Verifies that schema names are always quoted via JDBC identifier-quote characters before being + * interpolated into DDL statements, preventing SQL injection through malicious tenant identifiers. + */ +class DefaultSchemaHandlerSpec extends Specification { + + // ------------------------------------------------------------------------- + // quoteName โ unit tests (protected static helper) + // ------------------------------------------------------------------------- + + @Unroll + void "quoteName wraps '#name' with quote char '#quote' โ '#expected'"() { + given: + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> quote } + def conn = Mock(Connection) { getMetaData() >> meta } + + expect: + DefaultSchemaHandler.quoteName(conn, name) == expected + + where: + name | quote | expected + 'myschema' | '"' | '"myschema"' + 'MY_SCHEMA' | '"' | '"MY_SCHEMA"' + 'tenant_1' | '"' | '"tenant_1"' + // injection attempt wrapped inside quotes โ semicolon cannot start a new statement + 'public; DROP TABLE users' | '"' | '"public; DROP TABLE users"' + // embedded quote chars are stripped before re-quoting to prevent breakout + 'bad"name' | '"' | '"badname"' + // backtick quote (MySQL style) + 'myschema' | '`' | '`myschema`' + 'bad`name' | '`' | '`badname`' + // quoting not supported (driver returns space) + 'myschema' | ' ' | 'myschema' + 'myschema' | null | 'myschema' + 'myschema' | '' | 'myschema' + } + + // ------------------------------------------------------------------------- + // useSchema โ quoted DDL is executed + // ------------------------------------------------------------------------- + + void "useSchema executes SET SCHEMA with quoted name"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.useSchema(conn, 'myschema') + + then: + executedSql == ['SET SCHEMA "myschema"'] + } + + void "useSchema wraps injection payload inside quotes so it cannot break out"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.useSchema(conn, 'public; DROP TABLE users--') + + then: "dangerous payload is contained inside the identifier quotes" + executedSql == ['SET SCHEMA "public; DROP TABLE users--"'] + } + + void "useSchema strips embedded quote chars before wrapping to prevent identifier breakout"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.useSchema(conn, 'bad"; DROP TABLE users; --') + + then: "embedded quote is removed โ breakout is impossible" + executedSql == ['SET SCHEMA "bad; DROP TABLE users; --"'] + } + + // ------------------------------------------------------------------------- + // createSchema โ quoted DDL is executed + // ------------------------------------------------------------------------- + + void "createSchema executes CREATE SCHEMA with quoted name"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.createSchema(conn, 'tenant_42') + + then: + executedSql == ['CREATE SCHEMA "tenant_42"'] + } + + void "createSchema wraps injection payload inside quotes"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.createSchema(conn, 'tenant; DROP TABLE users--') + + then: + executedSql == ['CREATE SCHEMA "tenant; DROP TABLE users--"'] + } + + // ------------------------------------------------------------------------- + // useDefaultSchema + // ------------------------------------------------------------------------- + + void "useDefaultSchema calls useSchema with the configured default name"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '"' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() // default is PUBLIC + + when: + handler.useDefaultSchema(conn) + + then: + executedSql == ['SET SCHEMA "PUBLIC"'] + } + + // ------------------------------------------------------------------------- + // Custom statement templates + // ------------------------------------------------------------------------- + + void "custom useSchemaStatement template is honoured with quoting"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> '`' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler('USE %s', 'CREATE SCHEMA IF NOT EXISTS %s', 'main') + + when: + handler.useSchema(conn, 'mydb') + + then: + executedSql == ['USE `mydb`'] + } + + // ------------------------------------------------------------------------- + // Fall-through when quoting is unsupported + // ------------------------------------------------------------------------- + + void "when driver reports quoting unsupported (space) the name is used unquoted"() { + given: + def executedSql = [] + def statement = Mock(Statement) { execute(_ as String) >> { String sql -> executedSql << sql; true } } + def meta = Mock(DatabaseMetaData) { getIdentifierQuoteString() >> ' ' } + def conn = Mock(Connection) { getMetaData() >> meta; createStatement() >> statement } + def handler = new DefaultSchemaHandler() + + when: + handler.useSchema(conn, 'plainschema') + + then: + executedSql == ['SET SCHEMA plainschema'] + } +}
