From c370b60c6b660079e35cf97a975033cef5c09b30 Mon Sep 17 00:00:00 2001
From: brinn <brinn>
Date: Thu, 15 Nov 2012 13:27:38 +0000
Subject: [PATCH] [BIS-260] Fix SQL injections for custom queries. Add support
for backward compatible specifications of strings with '${code}' and
'{${array}}'. Add support for simplified specification of PostgreSQL arrays
with "=ANY({${myarray}})". Add support for overriding the parameter type as
placeholder metadata like in: ${code::varchar}. This is necessary to get
things working for Oracle as the Oracle JDBC driver does not support calling
ParameterMetaData.getParameterType().
SVN: 27635
---
.../cisd/openbis/plugin/query/server/DAO.java | 152 +++++++++++++++++-
.../openbis/plugin/query/server/DAOTest.java | 52 +++++-
2 files changed, 197 insertions(+), 7 deletions(-)
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/plugin/query/server/DAO.java b/openbis/source/java/ch/systemsx/cisd/openbis/plugin/query/server/DAO.java
index cd0884505bc..46248c8c5a4 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/plugin/query/server/DAO.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/plugin/query/server/DAO.java
@@ -30,12 +30,15 @@ import java.sql.Types;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.Set;
import javax.sql.DataSource;
+import org.apache.commons.lang.StringUtils;
import org.springframework.dao.DataAccessException;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.PreparedStatementCallback;
@@ -45,6 +48,7 @@ import org.springframework.jdbc.support.JdbcUtils;
import ch.systemsx.cisd.common.exceptions.UserFailureException;
import ch.systemsx.cisd.common.string.Template;
+import ch.systemsx.cisd.common.string.Template.IToken;
import ch.systemsx.cisd.common.utilities.Counters;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DateTableCell;
import ch.systemsx.cisd.openbis.generic.shared.basic.dto.DoubleTableCell;
@@ -72,6 +76,58 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
private static final String ENTITY_COLUMN_NAME_SUFFIX = "_KEY";
+ private static final Map<String, Integer> SQL_TYPE_CODE_TO_TYPE_MAP =
+ new HashMap<String, Integer>();
+
+ private static final Map<Integer, String> SQL_TYPE_TO_TYPE_CODE_MAP =
+ new HashMap<Integer, String>();
+
+ static
+ {
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("ARRAY", Types.ARRAY);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("BIGINT", Types.BIGINT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("BINARY", Types.BINARY);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("BIT", Types.BIT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("BLOB", Types.BLOB);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("BOOLEAN", Types.BOOLEAN);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("CHAR", Types.CHAR);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("CLOB", Types.CLOB);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("DATALINK", Types.DATALINK);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("DATE", Types.DATE);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("DECIMAL", Types.DECIMAL);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("DISTINCT", Types.DISTINCT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("DOUBLE", Types.DOUBLE);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("FLOAT", Types.FLOAT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("INTEGER", Types.INTEGER);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("JAVA_OBJECT", Types.JAVA_OBJECT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("LONGNVARCHAR", Types.LONGNVARCHAR);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("LONGVARBINARY", Types.LONGVARBINARY);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("LONGVARCHAR", Types.LONGVARCHAR);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("NCHAR", Types.NCHAR);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("NCLOB", Types.NCLOB);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("NULL", Types.NULL);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("NUMERIC", Types.NUMERIC);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("NVARCHAR", Types.NVARCHAR);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("OTHER", Types.OTHER);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("REAL", Types.REAL);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("REF", Types.REF);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("ROWID", Types.ROWID);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("SMALLINT", Types.SMALLINT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("SQLXML", Types.SQLXML);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("STRUCT", Types.STRUCT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("TIME", Types.TIME);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("TIMESTAMP", Types.TIMESTAMP);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("TINYINT", Types.TINYINT);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("VARBINARY", Types.VARBINARY);
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("VARCHAR", Types.VARCHAR);
+ for (Map.Entry<String, Integer> entry : SQL_TYPE_CODE_TO_TYPE_MAP.entrySet())
+ {
+ SQL_TYPE_TO_TYPE_CODE_MAP.put(entry.getValue(), entry.getKey());
+ }
+ // Convenience mapping
+ SQL_TYPE_CODE_TO_TYPE_MAP.put("STRING", Types.VARCHAR);
+ }
+
private static Map<String, EntityKind> entityKindByColumnName =
new HashMap<String, EntityKind>();
@@ -206,6 +262,59 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
return (TableModel) template.execute(resolvedQuery, callback);
}
+ /**
+ * A class for figuring out the parameter type.
+ * <p>
+ * Note that some JDBC drivers (like the one from Oracle) have
+ * {@link ParameterMetaData#getParameterType(int)} not implemented and thus requires setting it
+ * explicitly in the variable metadata.
+ */
+ private static class ParameterTypeProvider
+ {
+ private final Template template;
+
+ private final Map<Integer, Entry<String, String>> indexMap;
+
+ private final ParameterMetaData paramMD;
+
+ ParameterTypeProvider(Template template, Map<Integer, Entry<String, String>> indexMap,
+ ParameterMetaData paramMD)
+ {
+ this.template = template;
+ this.indexMap = indexMap;
+ this.paramMD = paramMD;
+ }
+
+ int getParameterCount() throws SQLException
+ {
+ return paramMD.getParameterCount();
+ }
+
+ int getParameterType(int param) throws SQLException
+ {
+ final Entry<String, String> entry = indexMap.get(param - 1);
+ final String overrideTypeCode =
+ StringUtils.upperCase(template.tryGetMetadata(entry.getKey()));
+ if (overrideTypeCode != null)
+ {
+ final Integer paramType = SQL_TYPE_CODE_TO_TYPE_MAP.get(overrideTypeCode);
+ if (paramType == null)
+ {
+ throw new SQLDataException("Invalid SQL type code '" + overrideTypeCode + "'");
+ }
+ return paramType;
+ } else
+ {
+ return paramMD.getParameterType(param);
+ }
+ }
+
+ String getParameterTypeName(int param) throws SQLException
+ {
+ return SQL_TYPE_TO_TYPE_CODE_MAP.get(getParameterType(param));
+ }
+ }
+
private static PreparedStatementCreator createSQLPreparedStatement(final String sqlQuery,
final QueryParameterBindings bindingsOrNull)
{
@@ -218,6 +327,22 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
final Map<Integer, Entry<String, String>> indexMap =
new HashMap<Integer, Entry<String, String>>();
final Template template = new Template(sqlQuery);
+ final Set<String> arrayVariableNames = new HashSet<String>();
+ // Support for legacy PostgreSQL array specifications
+ addToSet(template.replaceBrackets("'{", "}'", "", ""), arrayVariableNames);
+ // Support for legacy string parameters: replace '${var}' with ${var}.
+ template.replaceBrackets("'", "'", "", "");
+ // Support for simplified array specification in PostgreSQL
+ addToSet(template.replaceBrackets("{", "}", "", ""), arrayVariableNames);
+ for (String arrayVariableName : arrayVariableNames)
+ {
+ final IToken token = template.tryGetRightNeighbor(arrayVariableName);
+ if (token != null && token.tryGetValue() != null
+ && token.tryGetValue().startsWith("::text[]") == false)
+ {
+ token.setSubString(0, 0, "::text[]", "");
+ }
+ }
if (bindingsOrNull != null)
{
for (Entry<String, String> entry : bindingsOrNull.getBindings().entrySet())
@@ -231,8 +356,10 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
}
}
final PreparedStatement psm = con.prepareStatement(template.createText());
- final ParameterMetaData pmd = psm.getParameterMetaData();
- for (int i = 1; i <= pmd.getParameterCount(); ++i)
+ final ParameterTypeProvider ptp =
+ new ParameterTypeProvider(template, indexMap,
+ psm.getParameterMetaData());
+ for (int i = 1; i <= ptp.getParameterCount(); ++i)
{
final Entry<String, String> entry = indexMap.get(i - 1);
if (entry == null)
@@ -242,7 +369,7 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
final String strValue = entry.getValue();
try
{
- switch (pmd.getParameterType(i))
+ switch (ptp.getParameterType(i))
{
case Types.BIT:
case Types.BOOLEAN:
@@ -277,9 +404,14 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
case Types.NCHAR:
case Types.NVARCHAR:
case Types.LONGNVARCHAR:
- case Types.ARRAY:
psm.setString(i, strValue);
break;
+ case Types.ARRAY:
+ psm.setString(
+ i,
+ arrayVariableNames.contains(entry.getKey()) ? "{"
+ + strValue + "}" : strValue);
+ break;
case Types.TIME:
psm.setTime(i, Time.valueOf(strValue));
break;
@@ -291,8 +423,8 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
break;
default:
throw new SQLDataException("Unsupported SQL type "
- + pmd.getParameterTypeName(i) + "("
- + pmd.getParameterType(i) + ") for variable "
+ + ptp.getParameterTypeName(i) + "("
+ + ptp.getParameterType(i) + ") for variable "
+ entry.getKey());
}
} catch (RuntimeException ex)
@@ -303,6 +435,14 @@ class DAO extends SimpleJdbcDaoSupport implements IDAO
}
return psm;
}
+
+ private void addToSet(List<IToken> tokens, Set<String> nameSet)
+ {
+ for (IToken token : tokens)
+ {
+ nameSet.add(token.tryGetName());
+ }
+ }
};
}
}
diff --git a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/plugin/query/server/DAOTest.java b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/plugin/query/server/DAOTest.java
index c0f2c4ba336..4b53c0f4cfc 100644
--- a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/plugin/query/server/DAOTest.java
+++ b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/plugin/query/server/DAOTest.java
@@ -94,7 +94,37 @@ public class DAOTest extends AbstractTransactionalTestNGSpringContextTests
}
@Test
- public void testQueryWithArrayBinding()
+ public void testQueryWithStringBinding()
+ {
+ String query =
+ "select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code like ${code} order by id";
+ QueryParameterBindings bindings = new QueryParameterBindings();
+ bindings.addBinding("code", "200811050921591%");
+ testQueryWithBindings(query, bindings);
+ }
+
+ @Test
+ public void testQueryWithStringBindingExplicitType()
+ {
+ String query =
+ "select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code like ${code::string} order by id";
+ QueryParameterBindings bindings = new QueryParameterBindings();
+ bindings.addBinding("code", "200811050921591%");
+ testQueryWithBindings(query, bindings);
+ }
+
+ @Test
+ public void testQueryWithStringBindingOldStyle()
+ {
+ String query =
+ "select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code like '${code}' order by id";
+ QueryParameterBindings bindings = new QueryParameterBindings();
+ bindings.addBinding("code", "200811050921591%");
+ testQueryWithBindings(query, bindings);
+ }
+
+ @Test
+ public void testQueryWithPostgreSQLArrayBinding()
{
String query =
"select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code = any(${codes}::text[]) order by id";
@@ -103,6 +133,26 @@ public class DAOTest extends AbstractTransactionalTestNGSpringContextTests
testQueryWithBindings(query, bindings);
}
+ @Test
+ public void testQueryWithPostgreSQLArrayBindingOldStyle()
+ {
+ String query =
+ "select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code = any('{${codes}}'::text[]) order by id";
+ QueryParameterBindings bindings = new QueryParameterBindings();
+ bindings.addBinding("codes", "20081105092159188-3, 20081105092159111-1");
+ testQueryWithBindings(query, bindings);
+ }
+
+ @Test
+ public void testQueryWithPostgreSQLArrayBindingSimplified()
+ {
+ String query =
+ "select id, code as DATA_SET_KEY, registration_timestamp, is_valid from data where code = any({${codes}}) order by id";
+ QueryParameterBindings bindings = new QueryParameterBindings();
+ bindings.addBinding("codes", "20081105092159188-3, 20081105092159111-1");
+ testQueryWithBindings(query, bindings);
+ }
+
@Test(expectedExceptions = DataIntegrityViolationException.class)
public void testQueryWithBindingsSQLInjection()
{
--
GitLab