2

Goal: dynamically generate a PreparedStatement immune to SQL injection. 

    // This is a bad method. SQL injection danger . But it works 
    private PreparedStatement generateSQLBad(Connection connection, String tableName, 
        String columnName, String columnType) throws SQLException {
        String sql = "create table " + tableName + " (" + columnName + " " + columnType + ")";
        PreparedStatement create = connection.prepareStatement(sql);
        return create;
    }

    // I tried this. But it didn't work  
    private PreparedStatement generateSQLGood(Connection connection, String tableName, 
        String columnName, String columnType) throws SQLException {
        String sql = "create table ? (? ?)";
        PreparedStatement create = connection.prepareStatement(sql);
        create.setString(1, tableName);
        create.setString(2, columnName);
        create.setString(3, columnType);
        return create;
    }

How to dynamically generate PreparedStatement where user could choose tablename, columntype etc. and no danger of SQL injection?

Mark Rotteveel
  • 100,966
  • 191
  • 140
  • 197
john
  • 647
  • 5
  • 23
  • 53
  • 1
    There is no safe way to do this. – Elliott Frisch Jan 01 '19 at 03:36
  • THank you so much and Happy New Year. what's second best? – john Jan 01 '19 at 03:37
  • Option one here but it's the only one that works (if have you support dynamic tables). – Elliott Frisch Jan 01 '19 at 03:43
  • 1
    The second version won't even work. `generateSQLBad` will work, but injection isn't even the biggest problem. You should probably _not_ allow people from the outside to create tables. – Tim Biegeleisen Jan 01 '19 at 03:52
  • 1
    Why do you think you need this capability in the first place? There must be any number of more viable alternatives; please help us to help you choose one by sharing your motivation with us. – Kevin Anderson Jan 01 '19 at 03:54
  • i was deploying application to production and got this `checkmarx warning` for "Dynamic SQL". I thought I should fix it. – john Jan 01 '19 at 04:10
  • 1
    *I thought I should fix it.* You should. There is still no safe way to do this. I echo the other's though, **why** are you doing this? – Elliott Frisch Jan 01 '19 at 04:12
  • The application has a feature that allows users to copy data from one database to another. Those databases are specified by the user. User provides credentials etc. All works fine except when the table in the "target" database does not exist. In this case the application attempts to create a table and guesses the table SQL based on the user input so that the user doesn't have to waste time and hassle writing her own create statement. This feature requires dynamic sql – john Jan 01 '19 at 04:53
  • Anytime you hand over control of a database structure to a user, you lose some security. The only way I can think of to mitigate this is to prevent any known SQL keywords from the user-entered data... But I'm also no SQL expert; this is just a thought. – Zephyr Jan 01 '19 at 05:45
  • There are also dialect-specific features like T-SQL's [QUOTENAME](https://learn.microsoft.com/en-us/sql/t-sql/functions/quotename-transact-sql?view=sql-server-2017) function that can help mitigate the risk of SQL injection, but they are not necessarily bulletproof. Better to limit the table name to a subset of characters, e.g., `A-Z`, `0-9`, and `_`. – Gord Thompson Jan 01 '19 at 11:49

1 Answers1

1

You can't use ? parameter placeholders for identifiers (table names and column names). Nor can they be used for SQL keywords, like data types. Preparing a query needs to be able to validate the syntax, and validate that your table names and so on are legal. This must be done at prepare time, not at execute time. SQL doesn't allow parameters to contain syntax. They are always treated as scalar values. That's how they protect against SQL injection.

So parameters can only be used in place of scalar literals, like quoted strings or dates, or numeric values.

What to do for dynamic identifiers? As the comments suggest, the best you can do is filter the inputs so they're not going to introduce SQL injection. In a way, dynamic SQL based partially on user input is SQL injection. You just need to allow it in a controlled way.

All SQL implementations allow you to use special characters in your table names if you delimit the identifier. Standard SQL uses double-quotes for delimiters. MySQL uses back-ticks, and Microsoft SQL Server uses square brackets.

The point is that you can make strange-looking table names this way, like table names containing spaces, or punctuation, or international characters, or SQL reserved words. 

CREATE TABLE "my table" ( col1 VARCHAR(20) );

CREATE TABLE "order" ( col1 VARCHAR(20) );

See also my answer to https://stackoverflow.com/a/214344/20860

But what if the table name itself contains a literal double-quote character? Then you must escape that character. Either use a double character or a backslash:

CREATE TABLE "Dwayne ""The Rock"" Johnson" ( col1 VARCHAR(20) );

CREATE TABLE "Dwayne \"The Rock\" Johnson" ( col1 VARCHAR(20) );

You could alternatively design your function to check the dynamic table name for such characters, and either strip them out or throw an exception.

But even if you make the statement safe by carefully filtering the input, this might not satisfy the checkmarx warning. SQL injection testers have no way of analyzing your custom code to be sure it filters input reliably.

You may just have to do your best to make the dynamic SQL safe, knowing that checkmarx will always complain about it. Write comments in your code explaining your safety measures to future developers who read your code.

Also write unit tests to ensure that dangerous inputs result in either safe DDL statements, or else exceptions.

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828