SpotBugs: SQL injection warning is false positive in this case?

Question

I have something like this:

PreparedStatement ps; 
// ...
public static final String sqlQuery = "select * from users where user_id = ?";
public ResultSet getResultData(int id) {
  ps = conn.prepareStatement(sqlQuery);    // SpotBugs warning here
  ps.setInteger(1, id);
  return ps.executeQuery();
}

SpotBugs says next:

This use of java/sql/Connection.prepareStatement(Ljava/lang/String;)Ljava/sql/PreparedStatement; can be vulnerable to SQL injection (with JDBC)

And suggest already implemented solution.

Is that false positive warning and should be suppressed or did I miss something?

I don't see any SQL Injection issue there, but I would strongly suggest you define `ps` inside a `try ()` with resources to handle resources correctly. Also I would assume `sqlQuery` is String, right? — The Impaler, Jan 03 '23 at 14:02
@TheImpaler thank you and yes, sqlQuery is String, thanks (fixed post). So at all supressing spotbugs is the only solution here? — Sergey Ivaniukovich, Jan 05 '23 at 10:52
`PreparedStatement ps` should be defined inside the method and not outside of it. Otherwise, any other class may change its value inadvertently. And again, it should be enclosed in a `try (PreparedStatement ps = ...) { ... }` statement. I mean, who's closing the resources? — The Impaler, Jan 05 '23 at 14:18

score 0 · Answer 1 · answered Jan 03 '23 at 14:17

SpotBugs may be tracing your code back to the caller of that getResultData() function. It may be seeing that there's a way for an untrusted user to provide an arbitrary integer and get it into that query. For example, if your user_id number is 123 and you can view your account with

https://example.com/account?user_id=123

then a cybercreep can try

https://example.com/account?user_id=124

to see somebody else's account. This is an unfortunately common exploit people use to steal data. Notoriously, Panera Bread's online ordering site was hacked this way a few years ago.

You can fix such problems by making sure the user_id values you pass into your function are authorized for viewing by your logged-in user.

SpotBugs: SQL injection warning is false positive in this case?

1 Answers1