How to avoid "Security - A prepared statement is generated from a nonconstant String" FindBugs Warning

Question

I am working on a project that has a piece of code like the one below:

String sql = "SELECT MAX(" + columnName + ") FROM " + tableName;                
PreparedStatement ps = connection.prepareStatement(sql);

Is there any way that I can change this code so that FindBugs stop giving me a "Security - A prepared statement is generated from a nonconstant String" warning ?

Please assume that this code is safe regarding SQL INJECTION since I can control elsewhere in the code the possible values for "tableName" and "columnName" (they do not come come directly from user input).

Icao

Answer 1

Do not concatenate the sql String by +. You can use

String sql = String.format("SELECT MAX(%s) FROM %s ", columnName, tableName);

This is slower than concatenating a String so you should initialize this static then this is not a problem.

I think using a StringBuilder will also fix this warning.

Another way you can avoid this warning is to add @SuppressWarnings("SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING") above that string (or the method/or the class).

You could also use a Filter File to define rules which should be excluded.

An

Hi user714965, thanks for the reply. I have other occurrences of this warning using StringBuilder, so that does not work. The annotation works (it is good to clarify to other readers that its fully qualified name is @edu.umd.cs.findbugs.annotations.SuppressWarnings). Now i have to decide if i want the FindBugs lib in my project or not. Thanks again. – Icao

@ederribeiro: A dependency to FindBugs from your project might be not the best you are right. In my original answer I missed one point, please see the last paragraph. – An

This solves this particular issue because OP is sure that his code is safe from SQL INJECTION. But it is error prone and thus a bad practice in general. Prepared statements with parameters is the way to go. Not gonna vote negative since it actually resolves the question but I advice not to do it that way. – Overgrowth

Answer 2

private static final String SQL = "SELECT MAX(?) FROM ?";
PreparedStatement ps = connection.prepareStatement(sql);
ps.preparedStatement.setInt(1,columnName);
ps.preparedStatement.setString(2,tableName);

if you are using prepared statement, then in parameter should be a final string and parameters should be added later using setInt, setString methods.

this will resolve the findbug warning.

Hagi

did you exectute your statement? Binding table name you get ORA-00903: invalid table name. Passing column name works, but you get the column name back (not the column value). This is NOGO! – Ci

Answer 3

Neither String.format nor StringBuilder (or StringBuffer) helped me.

Solution was "prepareStatement" isolation:

private PreparedStatement prepareStatement(Connection conn, String sql) throws SQLException {
    return conn.prepareStatement(sql);
}

Overstep

That didn't help me either. – Flutter

Answer 4

Try using the following...

private static final String SQL = "SELECT MAX(%s) FROM %s";

And then using a String.format() call when you use it...

PreparedStatement ps = connection.prepareStatement(String.format(sql,columnName,tableName));

If that doesn't solve the problem, you can always ignore that check; turn it off in your FindBugs configuration.

If that doesn't work (or isn't an option), some IDEs (like IntelliJ) will also let you suprress warnings with either specially formatted comments or annotations.

Capillarity

Hi Jesse, thanks for the reply. I tried to do as you told but unfortunately it didn't work. Your last suggestion, the same that @user714965 mentioned, does the trick. Now i have to decide if i want to pay the price of having FindBugs lib in my project just to get rid of warning. – Icao

Answer 5

If you make sure that is no possibility of SQL injection, use the SuppressFBWarnings annotation on the method:

@edu.umd.cs.findbugs.annotations.SuppressFBWarnings("SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING")

Turoff

Answer 6

It is possible to use concatenation to create your String. Doing so does not cause the Security warning. And is preferrable for clarity's sake when dealing with long SQL statements which would be better written split across many lines

Using variables to construct the String is what causes the security warning.

This will cause the warning :

String columnName = getName();
String tableName  = getTableName();
final String sql = "SELECT MAX(" + columnName + ") FROM " + tableName;
PreparedStatement ps = connection.prepareStatement(sql);

This will not not work :

String columnName = getName();
String tableName  = getTableName();
final String sql = "SELECT MAX(" + "?" + ")" +
                   "FROM " + "?";
PreparedStatement ps = connection.prepareStatement(sql);
ps.setString(1, columnName);
ps.setString(2, tableName);

It does not work because prepared statements only allow parameters to be bound for "values" bits of the SQL statement.

This is the solution that works :

private static final boolean USE_TEST_TABLE = true;
private static final boolean USE_RESTRICTED_COL = true;
private static final String TEST_TABLE = "CLIENT_TEST";
private static final String PROD_TABLE = "CLIENT";
private static final String RESTRICTED_COL ="AGE_COLLATED";
private static final String UNRESTRICTED_COL ="AGE";

....................

final String sql = "SELECT MAX(" +
        ( USE_RESTRICTED_COL ? RESTRICTED_COL : UNRESTRICTED_COL ) +  ")" +
        "FROM " +
        ( USE_TEST_TABLE ? TEST_TABLE : PROD_TABLE );
PreparedStatement ps = connectComun.prepareStatement(sql);

But it only works if you have to chose between two tables whose names are known at compile time. You could use compounded ternary operators for more than 2 cases but then it becomes unreadable.

The 1st case might be a security issue if getName() or getTableName() gets the name from untrusted sources.

It is quite possible to construct a secure SQL statement using variables if those variables have been previously verified. This is your case, but FindBugs can't figure it out. Findbugs is not able to know what sources are trusted or not.

But if you must use a column or table name from user or untrusted input then there is no way around it. You have to verify yourself such string and ignore the Findbugs warning with any of the methods proposed in other answers.

Conclusion : There is no perfect solution for the general case of this question.

Overgrowth

Hi @jose-antonio-dura-olmos thanks for your suggestion but, have you tried it ? It doesn't seem to be possible to set "metadata" (such as table names or column names) using prepared statement. I've tried with mysql and h2 with no success. There is also a user in this thread that tried with Oracle and failed as well. – Icao

I have production code which changes table names this way. I have not checked this particular code but I'll do so. – Overgrowth

This what happens for talking out of memory. I needed to choose between two tables, one for production and one for test. But it did not work that way since table and column names can't be variable; sentence precompile needs to know the table and column names used to verify correct sintax. Thus they can't be variable. I got to a solution which still allowed me to use concatenation without getting the warning. I've updated my answer with that solution. – Overgrowth

Answer 7

StringBuilder sql = new StringBuilder();
sql.append("SELECT MAX(")
   .append(columnName)
   .append(") FROM ")
   .append(tableName);

PreparedStatement ps = connection.prepareStatement(sql);
ps.execute();

Driest

This will not even compile, for two reasons. And why would you even use a StringBuilder here? That makes zero sense. – Laflam