2

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?

dnc253
  • 39,967
  • 41
  • 141
  • 157

2 Answers2

0

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.

jmehrens
  • 10,580
  • 1
  • 38
  • 47
  • You should not modified your code to complied to a Static Analysis unless it has a true benefit (aside from not raising an issue). The code above will also rise an issue. – h3xStream Nov 13 '18 at 21:50
0

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.

h3xStream
  • 6,293
  • 2
  • 47
  • 57