From 59fede7acaaa1579b561407aefa582311f7ebe78 Mon Sep 17 00:00:00 2001 From: Andrea Boriero Date: Tue, 29 Sep 2020 20:56:30 +0100 Subject: [PATCH] HHH-14225 CVE-2020-25638 Potential for SQL injection on use_sql_comments logging enabled --- .../java/org/hibernate/dialect/Dialect.java | 15 ++- .../internal/SelectStatementBuilder.java | 2 +- .../main/java/org/hibernate/sql/Delete.java | 4 +- .../main/java/org/hibernate/sql/Insert.java | 2 +- .../java/org/hibernate/sql/InsertSelect.java | 2 +- .../java/org/hibernate/sql/QuerySelect.java | 2 +- .../main/java/org/hibernate/sql/Select.java | 2 +- .../java/org/hibernate/sql/SimpleSelect.java | 2 +- .../main/java/org/hibernate/sql/Update.java | 2 +- .../hibernate/test/comments/TestEntity.java | 46 ++++++++ .../hibernate/test/comments/TestEntity2.java | 37 ++++++ .../test/comments/UseSqlCommentTest.java | 111 ++++++++++++++++++ 12 files changed, 218 insertions(+), 9 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity.java create mode 100644 hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity2.java create mode 100644 hibernate-core/src/test/java/org/hibernate/test/comments/UseSqlCommentTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java index a85f50b337f9..4685e15621a1 100644 --- a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java +++ b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java @@ -84,6 +84,7 @@ import org.hibernate.internal.CoreLogging; import org.hibernate.internal.CoreMessageLogger; import org.hibernate.internal.util.ReflectHelper; +import org.hibernate.internal.util.StringHelper; import org.hibernate.internal.util.collections.ArrayHelper; import org.hibernate.internal.util.io.StreamCopier; import org.hibernate.loader.BatchLoadSizingStrategy; @@ -151,6 +152,10 @@ public abstract class Dialect implements ConversionContext { "'", Pattern.LITERAL ); + + private static final Pattern ESCAPE_CLOSING_COMMENT_PATTERN = Pattern.compile( "\\*/" ); + private static final Pattern ESCAPE_OPENING_COMMENT_PATTERN = Pattern.compile( "/\\*" ); + public static final String TWO_SINGLE_QUOTES_REPLACEMENT = Matcher.quoteReplacement( "''" ); private final TypeNames typeNames = new TypeNames(); @@ -3075,7 +3080,15 @@ public String addSqlHintOrComment( } protected String prependComment(String sql, String comment) { - return "/* " + comment + " */ " + sql; + return "/* " + escapeComment( comment ) + " */ " + sql; + } + + public static String escapeComment(String comment) { + if ( StringHelper.isNotEmpty( comment ) ) { + final String escaped = ESCAPE_CLOSING_COMMENT_PATTERN.matcher( comment ).replaceAll( "*\\\\/" ); + return ESCAPE_OPENING_COMMENT_PATTERN.matcher( escaped ).replaceAll( "/\\\\*" ); + } + return comment; } public boolean supportsSelectAliasInGroupByClause() { diff --git a/hibernate-core/src/main/java/org/hibernate/loader/plan/exec/query/internal/SelectStatementBuilder.java b/hibernate-core/src/main/java/org/hibernate/loader/plan/exec/query/internal/SelectStatementBuilder.java index 3d2b1d8ac303..0a1f4df0ec7f 100644 --- a/hibernate-core/src/main/java/org/hibernate/loader/plan/exec/query/internal/SelectStatementBuilder.java +++ b/hibernate-core/src/main/java/org/hibernate/loader/plan/exec/query/internal/SelectStatementBuilder.java @@ -187,7 +187,7 @@ public String toStatementString() { StringBuilder buf = new StringBuilder( guesstimatedBufferSize ); if ( StringHelper.isNotEmpty( comment ) ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "select " ) diff --git a/hibernate-core/src/main/java/org/hibernate/sql/Delete.java b/hibernate-core/src/main/java/org/hibernate/sql/Delete.java index cd5affb8f3a8..61d97c4a3b58 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/Delete.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/Delete.java @@ -9,6 +9,8 @@ import java.util.LinkedHashMap; import java.util.Map; +import org.hibernate.dialect.Dialect; + /** * An SQL DELETE statement * @@ -36,7 +38,7 @@ public Delete setTableName(String tableName) { public String toStatementString() { StringBuilder buf = new StringBuilder( tableName.length() + 10 ); if ( comment!=null ) { - buf.append( "/* " ).append(comment).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "delete from " ).append(tableName); if ( where != null || !primaryKeyColumns.isEmpty() || versionColumnName != null ) { diff --git a/hibernate-core/src/main/java/org/hibernate/sql/Insert.java b/hibernate-core/src/main/java/org/hibernate/sql/Insert.java index 93083a90e139..a50afc29b07f 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/Insert.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/Insert.java @@ -94,7 +94,7 @@ public Insert setTableName(String tableName) { public String toStatementString() { StringBuilder buf = new StringBuilder( columns.size()*15 + tableName.length() + 10 ); if ( comment != null ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append("insert into ") .append(tableName); diff --git a/hibernate-core/src/main/java/org/hibernate/sql/InsertSelect.java b/hibernate-core/src/main/java/org/hibernate/sql/InsertSelect.java index fc8388b98281..f93829812174 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/InsertSelect.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/InsertSelect.java @@ -68,7 +68,7 @@ public String toStatementString() { StringBuilder buf = new StringBuilder( (columnNames.size() * 15) + tableName.length() + 10 ); if ( comment!=null ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "insert into " ).append( tableName ); if ( !columnNames.isEmpty() ) { diff --git a/hibernate-core/src/main/java/org/hibernate/sql/QuerySelect.java b/hibernate-core/src/main/java/org/hibernate/sql/QuerySelect.java index 649d97359407..fc685182d8a2 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/QuerySelect.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/QuerySelect.java @@ -126,7 +126,7 @@ public void addOrderBy(String orderByString) { public String toQueryString() { StringBuilder buf = new StringBuilder( 50 ); if ( comment != null ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "select " ); if ( distinct ) { diff --git a/hibernate-core/src/main/java/org/hibernate/sql/Select.java b/hibernate-core/src/main/java/org/hibernate/sql/Select.java index 30a516dfb88e..11e13d86b2d3 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/Select.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/Select.java @@ -42,7 +42,7 @@ public Select(Dialect dialect) { public String toStatementString() { StringBuilder buf = new StringBuilder(guesstimatedBufferSize); if ( StringHelper.isNotEmpty(comment) ) { - buf.append("/* ").append(comment).append(" */ "); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append("select ").append(selectClause) diff --git a/hibernate-core/src/main/java/org/hibernate/sql/SimpleSelect.java b/hibernate-core/src/main/java/org/hibernate/sql/SimpleSelect.java index 46f96b71cdb9..31b6aeed2202 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/SimpleSelect.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/SimpleSelect.java @@ -151,7 +151,7 @@ public String toStatementString() { ); if ( comment != null ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "select " ); diff --git a/hibernate-core/src/main/java/org/hibernate/sql/Update.java b/hibernate-core/src/main/java/org/hibernate/sql/Update.java index 7b9a3fdae83a..5dcb0fd73963 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/Update.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/Update.java @@ -166,7 +166,7 @@ public Update setWhere(String where) { public String toStatementString() { StringBuilder buf = new StringBuilder( (columns.size() * 15) + tableName.length() + 10 ); if ( comment!=null ) { - buf.append( "/* " ).append( comment ).append( " */ " ); + buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " ); } buf.append( "update " ).append( tableName ).append( " set " ); boolean assignmentsAppended = false; diff --git a/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity.java b/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity.java new file mode 100644 index 000000000000..7c425becc49e --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity.java @@ -0,0 +1,46 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.test.comments; + +import javax.persistence.Entity; +import javax.persistence.Id; + +/** + * @author Andrea Boriero + */ +@Entity +public class TestEntity { + @Id + private String id; + + private String value; + + public TestEntity() { + + } + + public TestEntity(String id, String value) { + this.id = id; + this.value = value; + } + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } + + public String getValue() { + return value; + } + + public void setValue(String value) { + this.value = value; + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity2.java b/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity2.java new file mode 100644 index 000000000000..58b626df6039 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/test/comments/TestEntity2.java @@ -0,0 +1,37 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.test.comments; + +import javax.persistence.Entity; +import javax.persistence.Id; + +/** + * @author Andrea Boriero + */ +@Entity +public class TestEntity2 { + @Id + private String id; + + private String value; + + public String getId() { + return id; + } + + public void setId(String id) { + this.id = id; + } + + public String getValue() { + return value; + } + + public void setValue(String value) { + this.value = value; + } +} diff --git a/hibernate-core/src/test/java/org/hibernate/test/comments/UseSqlCommentTest.java b/hibernate-core/src/test/java/org/hibernate/test/comments/UseSqlCommentTest.java new file mode 100644 index 000000000000..2bd6adf8c8c3 --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/test/comments/UseSqlCommentTest.java @@ -0,0 +1,111 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later. + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.test.comments; + +import java.util.List; +import java.util.Map; +import javax.persistence.EntityManager; +import javax.persistence.TypedQuery; +import javax.persistence.criteria.CompoundSelection; +import javax.persistence.criteria.CriteriaBuilder; +import javax.persistence.criteria.CriteriaQuery; +import javax.persistence.criteria.Path; +import javax.persistence.criteria.Root; + +import org.hibernate.cfg.AvailableSettings; +import org.hibernate.jpa.test.BaseEntityManagerFunctionalTestCase; + +import org.junit.Before; +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.is; +import static org.hibernate.testing.transaction.TransactionUtil.doInJPA; +import static org.junit.Assert.assertThat; + +/** + * @author Andrea Boriero + */ +public class UseSqlCommentTest extends BaseEntityManagerFunctionalTestCase { + + @Override + protected Class[] getAnnotatedClasses() { + return new Class[] { TestEntity.class, TestEntity2.class }; + } + + @Override + protected void addMappings(Map settings) { + settings.put( AvailableSettings.USE_SQL_COMMENTS, "true" ); + settings.put( AvailableSettings.FORMAT_SQL, "false" ); + } + + @Before + public void setUp() { + doInJPA( this::entityManagerFactory, entityManager -> { + TestEntity testEntity = new TestEntity(); + testEntity.setId( "test1" ); + testEntity.setValue( "value1" ); + entityManager.persist( testEntity ); + + TestEntity2 testEntity2 = new TestEntity2(); + testEntity2.setId( "test2" ); + testEntity2.setValue( "value2" ); + entityManager.persist( testEntity2 ); + } ); + } + + @Test + public void testIt() { + String appendLiteral = "*/select id as col_0_0_,value as col_1_0_ from testEntity2 where 1=1 or id=?--/*"; + doInJPA( this::entityManagerFactory, entityManager -> { + + List result = findUsingQuery( "test1", appendLiteral, entityManager ); + + TestEntity test1 = result.get( 0 ); + assertThat( test1.getValue(), is( appendLiteral ) ); + } ); + + doInJPA( this::entityManagerFactory, entityManager -> { + + List result = findUsingCriteria( "test1", appendLiteral, entityManager ); + + TestEntity test1 = result.get( 0 ); + assertThat( test1.getValue(), is( appendLiteral ) ); + } ); + } + + public List findUsingCriteria(String id, String appendLiteral, EntityManager entityManager) { + CriteriaBuilder builder = entityManager.getCriteriaBuilder(); + CriteriaQuery criteria = builder.createQuery( TestEntity.class ); + Root root = criteria.from( TestEntity.class ); + + Path idPath = root.get( "id" ); + CompoundSelection selection = builder.construct( + TestEntity.class, + idPath, + builder.literal( appendLiteral ) + ); + criteria.select( selection ); + + criteria.where( builder.equal( idPath, builder.parameter( String.class, "where_id" ) ) ); + + TypedQuery query = entityManager.createQuery( criteria ); + query.setParameter( "where_id", id ); + return query.getResultList(); + } + + public List findUsingQuery(String id, String appendLiteral, EntityManager entityManager) { + TypedQuery query = + entityManager.createQuery( + "select new org.hibernate.test.comments.TestEntity(id, '" + + appendLiteral.replace( "'", "''" ) + + "') from TestEntity where id=:where_id", + TestEntity.class + ); + query.setParameter( "where_id", id ); + return query.getResultList(); + } +}