SQL Injection is not just for query values.
I enjoy doing code reviews. I find that I learn about new techniques and continually come to appreciate developers' ingenuity. Of course, code review also reveals common bad practices - so today I want to put some detail behind a common scenario where you may see SQL Injection.
Many Java developers, particularly those using JPA and Hibernate, think that as long as they follow the rules, they are protected from SQL Injection. In some ways, that is true. If you use the Criteria API, you are probably less likely to introduce SQL Injection into your application. However, many developers focus energy on the right side of the comparison - that is the value in the expression. Many think that if you're working in HQL (Hibernate Query Language) that it is difficult to introduce SQL Injection.
The idiom goes something like this:
// Name is a variable in the line that sets the parameter.
String queryString = "select p from Project p where name = :name_parameter";
Query query = getEntityManager().createQuery(queryString);
query.setParameter(":name_parameter", name);
query.getResultList();
In this example, the input name is being properly escaped in the resulting query.
The problem
The problem is that if a developer builds the query structure dynamically, they can still be open to SQL Injection. This can be avoided by just writing the query you actually want. In many systems I've worked on, we have had dedicated DAO classes for different data and though they inherit certain things from an AbstractDAO, they don't build HQL queries dynamically.
The place where you see it introduced is where developers get smart and build abstraction
layers that map parameters to passed in values on the fly.
In other words, if you try to build a base class that handles a lot of the gory details of
building arbitrary query strings, then you might end up with code that looks like this:
StringBuilder builder = "select p from ";
builder.append(entity.className);
builder.append(" WHERE p.");
builder.append(fieldName);
builder.append(" = :");
builder.append(fieldName);
Query query = getEntityManager().createQuery(builder.toString());
query.setParameter(fieldName, fieldValue);
query.getResultList();
Since they are using entity, fieldName and fieldValue on the fly, they need to let fieldName be dynamic.
They don't even know which table the query is going to be on, they are essentially writing internal
framework code to handle a real world need.
Obviously, real world examples get much more complex with multiple search fields, subqueries, order and other clauses. It is impressive to see developers build elegant solutions to these many possibilities - but in this case, the point is that this code can cause a problem in both places that the fieldName is appended into the hql!
Make it Real
To illustrate this, I built a simple Grails based application and put it
on github. You should be able to install Grails
and run it with minimal effort - other than the Java sized downloads.
I recommend the copycat GVM - Groovy Version Manager as a starting point.
Instructions for installing Grails are here. With GVM, I
hardly had to do anything beyond:
gvm install grails
Grails is like Rails, but it uses Java frameworks like Hibernate and runs in Tomcat.
So this code is running in a JVM, generating HQL and will allow crazy SQL Injection (code below also in github):
package com.jemurai.triage
class ProjectController {
def scaffold = Project
def list() {
// Example of happy path:
// http://localhost:8081/gtriage/project/list?field=name
// &query=AZ
// Example breaking the left side:
// http://localhost:8081/gtriage/project/list?
// field=description%20is%20not%20null%20or%20name&query=AZ
if (params.field && params.query){
[projectInstanceList:
Project.executeQuery("select p from Project p where " +
params.field + " = ?", [params.query] ),
projectInstanceTotal: Project.count()]
} else {
[projectInstanceList: Project.list(),
projectInstanceTotal: Project.count()]
}
}
}
Its the part where I append the field into the query that can cause the problem.
Look at this more closely:
Project.executeQuery("select p from Project p where " + params.field + " = ?"
, [params.query])
In the example where I request this
URL:
http://localhost:8081/gtriage/project/list?
field=description%20is%20not%20null%20or%20name&query=AZ
The resulting query is
select p from Project where description is not null or name = "AZ"
Obviously, this is going to return any records with a non-null description.
Hopefully its easy to see how even though I properly parameterize the value, the field name
is a critical oversight!!!
Takeaways
- Avoid String concatenation wherever possible when building queries.
- Resist the urge to write an all powerful data layer abstraction. If you must, understand the responsibility you are taking on.
- Always think about who controls the input to your statements - anywhere in your statements!
- HQL is not blanket protection from SQL Injection!