Skip to content

Commit

Permalink
HHH-14225 CVE-2020-25638 Potential for SQL injection on use_sql_comme…
Browse files Browse the repository at this point in the history
…nts logging enabled
  • Loading branch information
dreab8 authored and Sanne committed Nov 13, 2020
1 parent d48e19d commit d22bbb5
Show file tree
Hide file tree
Showing 12 changed files with 217 additions and 9 deletions.
14 changes: 13 additions & 1 deletion hibernate-core/src/main/java/org/hibernate/dialect/Dialect.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 " )
Expand Down
4 changes: 3 additions & 1 deletion hibernate-core/src/main/java/org/hibernate/sql/Delete.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import java.util.LinkedHashMap;
import java.util.Map;

import org.hibernate.dialect.Dialect;

/**
* An SQL <tt>DELETE</tt> statement
*
Expand Down Expand Up @@ -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 ) {
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/sql/Insert.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/sql/Select.java
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 " );
Expand Down
2 changes: 1 addition & 1 deletion hibernate-core/src/main/java/org/hibernate/sql/Update.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
}

0 comments on commit d22bbb5

Please sign in to comment.