Search code examples
javapostgresqlprepared-statementsql-injection

SQLinjection concerns when dynamic table variable is needed in Java query string


Scenerio:

I've got about 50 tables I want to be able to pull data from and refresh a local database each time a user clicks a Java "update all" button. I can't write to the remote DB or I would just make a view with a prepared statement or stored procedure.

Creating a custom prepared statement for each table seems hard to maintain, but I also don't want to make my code vulnerable to SQLInjection.

To reduce my risk, I have a whitelist of table names (derived from local DB),a limited string length, specified tables (data and date), and have added the tag table_name into the string concatenation.

My hope is that by adding the tag field most SQLinjections would simply fail due to syntax error. These would be handled with a try catch and wound't be able to cause harm (may be wrong).

While it may be possible to change the table name, I'm not too concerned as it would have to be a legitimate table with the proper structure and I'm grabbing all the tables anyway.

Question:

What other SQLinjection concerns should I have with using the prepared statement below?

List<String> tableNamesWhiteList; 
...

  for (String t:tableNamesWhiteList)
  {       
    if(t.length < 30)
{
  try{
      String stm = "select '"+t+"' as table_name, data, date from " + t +" where data = ?";
      pst = con.prepareStatement(stm);
      pst.setInt(1, 100);
      rs = pst.executeQuery();
....            

Hope this makes sense


Solution

  • I'll preface what I'm about to say by pointing out that I am not a security analyst.

    I don't think what you've done here is either effective or necessary. First, SQL injection depends on the parser reading to the end of the first valid SQL statement and executing it before continuing. So if the above code were vulnerable at all, (and I don't think it is, but I'll cover that in a moment,) your query

    select '"+t+"' as table_name, data, date from " + t +" where data = ?
    

    could be attacked by setting t equal to

    1' from pg_users; drop table pg_users;
    

    and the parser would execute the SELECT, then execute the DROP, then choke on the AS, but by then it would be too late. That's how SQL injection works.

    The fact that you're using a prepared statement, though, means that the entire query string has to be parsed from one end to the other before any of it is executed, and any attempt to inject through t will almost certainly leave invalid syntax at the end of the query which would choke the parser before any of it could be executed. Even if that were not the case, your table name is coming from your own code, picked from a preset list, and if that's been compromised you have bigger problems to worry about than SQL injection.

    Writing secure code is good, but armour-plating your code makes it slow and difficult to maintain and can rarely be justified.

    One alternative you might consider, though, was used on a commercial product I worked on a few years ago. That alternative is to store the queries for the 50 tables in another table, perhaps indexed by the table name. The code might look something like this:

    String stm = "SELECT query FROM table_queries WHERE table_id = ?";
    pst = con.prepareStatement(stm);
    pst.setInt(1, tableID);
    rs = pst.executeQuery();
    stm = rs.first().getString("query");
    pst = con.prepareStatement(stm);
    pst.setInt(1, 100);
    rs = pst.executeQuery();