False positives for SQL injection from find-sec-bugs

Question

We're using find-sec-bugs with findbugs to find potential problems in our code. We using Spring JDBCTemplate for our DB access, and find-sec-bugs seems to think we have SQL injection vulnerabilities all over the place. The simplest example is as follows:

public class MyDataRepo {
    private final String getDataSql;

    public PilotRepositoryImpl(DataSource dataSource) {
        jdbcTemplate = new JdbcTemplate(dataSource);
        getDataSql = "SELECT ID, FIRST_NAME, LAST_NAME, USERNAME, EMAIL FROM USERS WHERE COMPANY_ID = ? AND ID = ?";
        //...
    }

    public MyData getMyData(String companyId, UUID userId)
    {
        return jdbcTemplate.queryForObject(getDataSql, new Object[]{companyId, userId}, myDataRowMapper);
    }
}

This results it thinking it is vulnerable to SQL injection, which it clearly isn't (Please correct me if I'm wrong).

If I copy and paste the string directly into the method like this:

return jdbcTemplate.queryForObject("SELECT ID, FIRST_NAME, LAST_NAME, USERNAME, EMAIL FROM USERS WHERE COMPANY_ID = ? AND ID = ?", new Object[]{companyId, userId}, myDataRowMapper);

then it thinks it's fine. I like having the SQL defined at the top of my class and not buried in each method. I don't really want to have to add @SuppressFBWarnings everywhere, as that pretty much defeats the purpose.

Is there a better way to get around this? Is there something actually wrong with what we're doing?

Answer 1

I like having the SQL defined at the top of my class and not buried in each method.

Try using a static method instead of a field.

public class MyDataRepo {

    private static String getDataSql() {
        return "SELECT ID, FIRST_NAME, LAST_NAME, USERNAME, EMAIL FROM USERS WHERE COMPANY_ID = ? AND ID = ?"
    }

    public PilotRepositoryImpl(DataSource dataSource) {
        jdbcTemplate = new JdbcTemplate(dataSource);
        //...
    }

    public MyData getMyData(String companyId, UUID userId) {
        return jdbcTemplate.queryForObject(getDataSql(), new Object[]{companyId, userId}, myDataRowMapper);
    }
}

This results it thinking it is vulnerable to SQL injection, which it clearly isn't (Please correct me if I'm wrong).

I don't disagree but the wider scope might invite more attack vectors.

Answer 2

The code is safe. FSB does not recognize at the moment that the field read is a final field and its source was safe.

This false positive will eventually be ignored when this issue gets fix: https://github.com/find-sec-bugs/find-sec-bugs/issues/385.