HHH-14225 CVE-2020-25638 Potential for SQL injection on use_sql_comments logging enabled

dreab8 · Sanne · commit d22bbb5c339c · 2020-11-13T13:34:09.000Z
diff --git a/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java b/hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java
@@ -24,6 +24,7 @@
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
+import java.util.regex.Pattern;
 
 import org.hibernate.HibernateException;
 import org.hibernate.LockMode;
@@ -140,6 +141,9 @@ public abstract class Dialect implements ConversionContext {
 	 */
 	public static final String CLOSED_QUOTE = "`\"]";
 
+	private static final Pattern ESCAPE_CLOSING_COMMENT_PATTERN = Pattern.compile( "\\*/" );
+	private static final Pattern ESCAPE_OPENING_COMMENT_PATTERN = Pattern.compile( "/\\*" );
+
 	private final TypeNames typeNames = new TypeNames();
 	private final TypeNames hibernateTypeNames = new TypeNames();
 
@@ -3002,6 +3006,14 @@ 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;
 	}
 }
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
@@ -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
@@ -9,6 +9,8 @@
 import java.util.LinkedHashMap;
 import java.util.Map;
 
+import org.hibernate.dialect.Dialect;
+
 /**
  * An SQL <tt>DELETE</tt> 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
@@ -90,7 +90,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
@@ -65,7 +65,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
@@ -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
@@ -40,7 +40,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
@@ -148,7 +148,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
@@ -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
@@ -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 <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ */
+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
@@ -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 <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ */
+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
@@ -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 <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ */
+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<TestEntity> result = findUsingQuery( "test1", appendLiteral, entityManager );
+
+			TestEntity test1 = result.get( 0 );
+			assertThat( test1.getValue(), is( appendLiteral ) );
+		} );
+
+		doInJPA( this::entityManagerFactory, entityManager -> {
+
+			List<TestEntity> result = findUsingCriteria( "test1", appendLiteral, entityManager );
+
+			TestEntity test1 = result.get( 0 );
+			assertThat( test1.getValue(), is( appendLiteral ) );
+		} );
+	}
+
+	public List<TestEntity> findUsingCriteria(String id, String appendLiteral, EntityManager entityManager) {
+		CriteriaBuilder builder = entityManager.getCriteriaBuilder();
+		CriteriaQuery<TestEntity> criteria = builder.createQuery( TestEntity.class );
+		Root<TestEntity> root = criteria.from( TestEntity.class );
+
+		Path<Object> idPath = root.get( "id" );
+		CompoundSelection<TestEntity> selection = builder.construct(
+				TestEntity.class,
+				idPath,
+				builder.literal( appendLiteral )
+		);
+		criteria.select( selection );
+
+		criteria.where( builder.equal( idPath, builder.parameter( String.class, "where_id" ) ) );
+
+		TypedQuery<TestEntity> query = entityManager.createQuery( criteria );
+		query.setParameter( "where_id", id );
+		return query.getResultList();
+	}
+
+	public List<TestEntity> findUsingQuery(String id, String appendLiteral, EntityManager entityManager) {
+		TypedQuery<TestEntity> 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();
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -187,7 +187,7 @@ public String toStatementString() {`
`187`	`187`	`StringBuilder buf = new StringBuilder( guesstimatedBufferSize );`
`188`	`188`
`189`	`189`	`if ( StringHelper.isNotEmpty( comment ) ) {`
`190`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`190`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`191`	`191`	`}`
`192`	`192`
`193`	`193`	`buf.append( "select " )`
Original file line number	Diff line number	Diff line change
`@@ -90,7 +90,7 @@ public Insert setTableName(String tableName) {`
`90`	`90`	`public String toStatementString() {`
`91`	`91`	`StringBuilder buf = new StringBuilder( columns.size()*15 + tableName.length() + 10 );`
`92`	`92`	`if ( comment != null ) {`
`93`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`93`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`94`	`94`	`}`
`95`	`95`	`buf.append("insert into ")`
`96`	`96`	`.append(tableName);`
Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ public String toStatementString() {`
`65`	`65`
`66`	`66`	`StringBuilder buf = new StringBuilder( (columnNames.size() * 15) + tableName.length() + 10 );`
`67`	`67`	`if ( comment!=null ) {`
`68`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`68`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`69`	`69`	`}`
`70`	`70`	`buf.append( "insert into " ).append( tableName );`
`71`	`71`	`if ( !columnNames.isEmpty() ) {`
Original file line number	Diff line number	Diff line change
`@@ -126,7 +126,7 @@ public void addOrderBy(String orderByString) {`
`126`	`126`	`public String toQueryString() {`
`127`	`127`	`StringBuilder buf = new StringBuilder( 50 );`
`128`	`128`	`if ( comment != null ) {`
`129`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`129`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`130`	`130`	`}`
`131`	`131`	`buf.append( "select " );`
`132`	`132`	`if ( distinct ) {`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ public Select(Dialect dialect) {`
`40`	`40`	`public String toStatementString() {`
`41`	`41`	`StringBuilder buf = new StringBuilder(guesstimatedBufferSize);`
`42`	`42`	`if ( StringHelper.isNotEmpty(comment) ) {`
`43`		`- buf.append("/* ").append(comment).append(" */ ");`
	`43`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`buf.append("select ").append(selectClause)`
Original file line number	Diff line number	Diff line change
`@@ -148,7 +148,7 @@ public String toStatementString() {`
`148`	`148`	`);`
`149`	`149`
`150`	`150`	`if ( comment != null ) {`
`151`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`151`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`152`	`152`	`}`
`153`	`153`
`154`	`154`	`buf.append( "select " );`
Original file line number	Diff line number	Diff line change
`@@ -166,7 +166,7 @@ public Update setWhere(String where) {`
`166`	`166`	`public String toStatementString() {`
`167`	`167`	`StringBuilder buf = new StringBuilder( (columns.size() * 15) + tableName.length() + 10 );`
`168`	`168`	`if ( comment!=null ) {`
`169`		`- buf.append( "/* " ).append( comment ).append( " */ " );`
	`169`	`+ buf.append( "/* " ).append( Dialect.escapeComment( comment ) ).append( " */ " );`
`170`	`170`	`}`
`171`	`171`	`buf.append( "update " ).append( tableName ).append( " set " );`
`172`	`172`	`boolean assignmentsAppended = false;`