BTalic207022 (Community Member) asked a question.
ID 89)
I'm not sure what's wrong here
For executing queries in my app I'm using PreparedStatement . I have custom class QueryModel where I collect query and all parameters.
Then based on QueryModel instance I create PreparedStatement and set all needed parameters.
Scanner showing issue on executeQuery()
Description
This database query contains a SQL injection flaw. The function call constructs a dynamic SQL query using a variable
derived from untrusted input. An attacker could exploit this flaw to execute arbitrary SQL queries against the database.
Example of my custom QueryModel which I used to generate PreparedStatement
sqlQuery = "SELECT * FROM table WHERE username = ? and password = ?"
params = {ArrayList@12810} size = 2
0 = {QueryParam@12812}
type = "String"
value = "USERNAMETEST"
1 = {QueryParam@12813}
type = "String"
value = "PASSWORDTEST"
------------------------------------------------------------------------------------
public class QueryModel {
String sqlQuery;
List<QueryParam> params;
public QueryModel() {
params = new ArrayList();
}
public void addToParams(String type, Object value) {
if(params == null)
params = new ArrayList();
params.add(new QueryParam(type, value));
}
}
-----------------------------------------------------------------------------------------
Method where I generate PreparedStatement base on my dynamic instance QueryModel
public PreparedStatement makeQueryDbCall(QueryModel queryModel) throws SQLException {
if (connection == null || !connection.isValid(0))
connection = configuration.dataSource().getConnection();
PreparedStatement prepareStatement = connection.prepareStatement(queryModel.sqlQuery);
if (queryModel.getParams() != null && queryModel.getParams().size() > 0) {
int index = 1;
for (QueryParam param: queryModel.getParams()) {
if (param.type.equals(Integer.class.getSimpleName())) {
prepareStatement.setInt(index, (int) param.value);
} else if(param.type.equals(String.class.getSimpleName())) {
prepareStatement.setString(index, (String) param.value);
}
index ++;
}
}
return prepareStatement;
}
---------------------------------------------------------------------------------------------
public PrepareResponse callRetryableServiceQuery(QueryModel queryModel) {
try {
return retryableServiceCall(() -> genericDao.makeQueryDbCall(queryModel), queryModel);
}catch (Exception e){
oracleErrorLogService.saveQueryOracleErrorLog(queryModel, e);
throw e;
}
}
-----------------------------------------------------------------------------------------------------------
public static <T> T getObject(BaseRepository baseRepository, QueryModel queryModel, Class clazz) {
PrepareResponse response = baseRepository.callRetryableServiceQuery(queryModel);
if (!response.isStatus()) {
baseRepository.closeResultSet(response.getSpReturnData());
return null;
}
ResultSet rs = null;
try {
rs = ((PreparedStatement) response.getSpReturnData()).executeQuery(); //place where scanner showing issue
} catch (SQLException e) {
logger.error("Failed execute query: {}", e.getMessage());
e.printStackTrace();
}
...
}
.png){kind=logo}
I suspect, but this would be better confirmed by a Veracode Security Consultant, that what you are seeing is the static analyzer flagging the whole "QueryModel" object as tainted. When one field of an object is "tainted", like the parameters, the static analyzer typically considers the whole object "tainted", and thus would probably assume that the query is tainted. I'm not sure your "QueryModel" approach will work well with the static analyzer as it currently works.
As a test, if you remove your query from the QueryModel object, and then pass constant string directly to prepareStatement, I suspect the flaw will "disappear".
@Robert (Community Member)
Yes, you're right, If I pass constant string instead of value from query model flaw disappear
// PreparedStatement prepareStatement = connection.prepareStatement(queryModel.sqlQuery);
PreparedStatement prepareStatement = connection.prepareStatement("query");
In the end whether this is really an issue?
If you are always using constant strings with query parameters for your queries, then, I don't believe there is a risk of an SQL Injection attack. However, if you use the Veracode static scanner, you will need to mark every "flaw" it generates related to your query use cases as "mitigated by design" or "false positive" (although I don't think Veracode would consider this a false positive, but it is a limitation of the static scanner, and the way it treats objects as tainted if any part of the data is tainted, as opposed to tracking the tainted data through the object in more detail). This maybe be frustrating or annoying if you have a lot of cases, or they may "re-appear" in different versions of the scan as your code changes. Up to you to judge how that works for you.