Improving DB calls by using PreparedStatement

The Issue

One of the easiest attacks that could be done to a web application is to exploit SQL injection flaws. Attackers usually do this by inserting arbitrary SQL commands to URL parameters or endpoint payloads. The code snippet below shows a typical method prone to this issue.

	public static ReturnObject getSomethingFromDB(String param1, String param2) throws DinaaException {
		ReturnObject returnObj = new ReturnObject();
		String conditions = "";
		String postConditions = "";
		if (!StringUtil.isEmpty(param1)) {
			conditions = "WHERE column1 IN (SELECT column_name FROM table_name WHERE column_name2 <> '" +  param2   +  
							"'  AND column_name1 = " + param1 + ") ";
			postConditions = "GROUP BY column1, column2 ORDER BY COUNT(*) DESC LIMIT 4";
		}		
		Connection con = null;
		Statement stm = null;
		ResultSet rst = null;
		try {
			con = JdbcUtil.getConnection();
			stm = con.createStatement();
			StringBuffer sql = new StringBuffer();
			sql.append("SELECT column1, column2, COUNT(*) FROM " + TableConstants.TABLE_TO_QUERY + " " + 
						conditions + " " + postConditions + ";");
			System.out.println("QUERY: " + sql.toString());
			rst = stm.executeQuery(sql.toString());
			
			while (rst.next()) {
				returnObj.setProp1(rst.getString("column1") == null ? StringUtil.EMPTY : rst.getString("column1"));
				returnObj.setProp1(rst.getString("column2") == null ? StringUtil.EMPTY : rst.getString("column2"));
			}
		} catch (SQLException ex) {
			throw new DinaaException(ex);
		} catch (NamingException ex) {
			throw new DinaaException(ex);
		} finally {
			try {
				rst.close();
				stm.close();
				JdbcUtil.freeConnection(con);
			} catch (Exception ex) {
				ex.printStackTrace();
			}
		}
		return returnObj;
	}

The example is a bit involved so here is a brief summary of what it does.

1. The method accepts two parameters param1 and param2 and returns an result of type ReturnObject.
2. Two variables conditions and postConditions are initialized and are dynamically given additional WHERE and GROUP BY clauses using string concatenation if param1 is not empty.
3. A connection is made to the database and the SQL statement is completed by concatenating the SELECT along with the table name to the conditions and postConditions.
4. The result is parsed and the ReturnObject's properties are set before it is returned.

The reason this poses a possible threat is because the parameters could potentially be used to add arbitrary sql statements that can be used to attack the database.

In the example, what if we set param1 = "%anything%;drop all;" ?

Just for discussion's sake, let's say the query above succeeds, then we can clearly see that by using concatenation of parameters, we introduce the possibility of anyone calling this method to delete the whole database!

The Fix

There are many ways to refactor the example code. One of the simplest ways would be to use PreparedStatements. In the proposed solution below, we will also consider the ease of refactoring the code. Meaning, the less changes we need to make, the better.

To do that, let's focus on the query itself. Basically, the completed query with conditions would look like this if param1 is empty:

SELECT column1, column2, COUNT(*) FROM TABLE

If not the query becomes:

SELECT column1, column2, COUNT(*) 
FROM TABLE 
WHERE column1 IN (
	SELECT column_name 
	FROM table_name 
	WHERE column_name2 <> PARAM2 
	AND column_name1 = PARAM1
)
GROUP BY column1, column2 
ORDER BY COUNT(*) DESC LIMIT 4

So looking at this context, there are actually just two queries that this method would execute. So let's make the queries more readable by simply saving them into constants(or enums, or maps if you wish)

public static final String SELECT_FROM_TABLE = "SELECT column1, column2, COUNT(*) FROM TABLE";

public static final String SELECT_FROM_TABLE_WITH_CONDITION = "SELECT column1, column2, COUNT(*) " + 
		"FROM TABLE " + 
		"WHERE column1 IN " + 
		"(SELECT column_name " + 
		"FROM table_name WHERE column_name2 <> ? AND column_name1 = ? ) " + 
		"GROUP BY column1, column2 ORDER BY COUNT(*) DESC LIMIT 4";

Notice that we replaced references to PARAM1 and PARAM2 with question marks(?).

So now let's see how the method looks like when it is refactored:

Note: This class could be created in a separate class file or as an inner class.

	public class DBStrings {
		public static final String SELECT_FROM_TABLE = "SELECT column1, column2, COUNT(*) FROM TABLE";
		public static final String SELECT_FROM_TABLE_WITH_CONDITION = "SELECT column1, column2, COUNT(*) " + 
					"FROM TABLE " + 
					"WHERE column1 IN " + 
					"(SELECT column_name " + 
					"FROM table_name WHERE column_name2 <> ? AND column_name1 = ? ) " + 
					"GROUP BY column1, column2 ORDER BY COUNT(*) DESC LIMIT 4";
	}

Then refactor the getSomethingFromDB method to:

	public static ReturnObject getSomethingFromDB(String param1, String param2) throws DinaaException {
		
		ReturnObject returnObj = new ReturnObject();	
		Connection con = null;
		PreparedStatement stm = null;
		ResultSet rst = null;
		String sql = StringUtil.isEmpty(param1) ? DBStrings.SELECT_FROM_TABLE : DBStrings.SELECT_FROM_TABLE_WITH_CONDITION;
		try {
			con = JdbcUtil.getConnection();
			con.create
			stm = con.prepareStatement(sql);
			if(!StringUtil.isEmpty(param1)){
				stm.setString(0, param2);
				stm.setString(1, param1);
			}
			rst = stm.executeQuery();
			while (rst.next()) {
				returnObj.setProp1(rst.getString("column1") == null ? StringUtil.EMPTY : rst.getString("column1"));
				returnObj.setProp1(rst.getString("column2") == null ? StringUtil.EMPTY : rst.getString("column2"));
			}
		} catch (SQLException ex) {
			throw new DinaaException(ex);
		} catch (NamingException ex) {
			throw new DinaaException(ex);
		} finally {
			try {
				rst.close();
				stm.close();
				JdbcUtil.freeConnection(con);
			} catch (Exception ex) {
				ex.printStackTrace();
			}
		}
		return returnObj;
	}

The cool thing here is that this particular method now has gained two distinct improvements. First, since PreparedStatements allow the database to cache previous results, if we call the method with the same set of arguments, the database would use a cached result as needed, thus improving query performance. And of course, we are now properly protected from SQL injection attacks because the database will now treat anything you pass in param1 and param2 as strings and will escape the contents of those parameters as needed to make it conform to a SQL string format.