Is putting a string literal in executeQuery prone to an SQL injection in JDBC?

Question

I've been looking around and can't seem to find a solid answer to this. I was wondering if putting a string literal in executeQuery() is still prone to SQL injection.

So lets say I have this code:

  Connection conn = DriverManager.getConnection("jdbc:mysql://localhost:3306/","root","password");
  Statement stmt = conn.createStatement();

  ResultSet res = stmt.executeQuery("SELECT * from users where uid = "+uid);

Is this prone to a SQL injection?

Another question is, is just making the method that uses this code only throw an SQLException, and then trying and catching in main acceptable?

For example:

public void execMethod(String uid) throws SQLException {
      Connection conn = DriverManager.getConnection("jdbc:mysql://localhost:3306/","root","password");
      Statement stmt = conn.createStatement();

      ResultSet res = stmt.executeQuery("SELECT * from users where uid = "+uid);
    // execute some other code
    res.close();
}

public static void main(String[] args) {
    try {
        execMethod("123");
        execMethod("456");
    } catch(Exception ex) {
        ex.printStackTrace();
    }
}

Is this the standard or correct way of using SQL exceptions? I've never really worked with SQL and especially not Java and SQL. The tutorials I've read seem to only lay it out one way, so I'm pretty unsure of myself.

Answer 1

Short answer : yes.

You do not appear to be doing any kind of input validation so there isn't anything stopping uid from being something like "105 or 1=1"

You should probably use PreparedStatements tutorial here

PreparedStatement stmt = conn.prepareStatement("SELECT * from users where uid = ?")
stmt.setString(1, uid);

..same as before

Also you don't close the statement or the connection which should be done in a finally block incase an exception is thrown

Answer 2

Is this prone to a SQL injection?"

Yes, you have no control over what uid might actually contain.

See Using Prepared Statements for more details

Another question is, is just making the method that uses this code only throw an SQLException, and then trying and catching in main acceptable?"

Yes, but you should at least wrap the contends of execMethod in try-finally to ensure that you are closing the resources you open (or use a try-with-resources for Java 7)

public void execMethod(String uid) throws SQLException {

    try (Connection conn = DriverManager.getConnection("jdbc:mysql://localhost:3306/", "root", "password")) {
        try (PreparedStatement stmt = conn.prepareStatement("SELECT * from users where uid = ?")) {
            stmt.setString(1, uid);
            try (ResultSet res = stmt.executeQuery()) {
                // Process ressult set
            }
        }
    }
}

See The try-with-resources Statement for more details

But, I would only catch the SQLException for EACH call, not batch them together, as you won't know what failed and what succeeded

try {
    execMethod("123");
    try {
        execMethod("456");
    } catch (Exception ex) {
        // Maybe undo 123
        System.out.println("Failed 456");
        ex.printStackTrace();
    }
} catch (Exception ex) {
    System.out.println("Failed 123");
    ex.printStackTrace();
}

(assuming that 456 is dependent on the success of 123)

Answer 3

Yes. If uid can be entered by a user (it's not a String literal). I suggest you use a PreparedStatement, and a try-with-resources like

final String sql = "SELECT * from users where uid = ?";
try (PreparedStatement ps = conn.prepareStatement(sql)) {
    ps.setString(1, uid);
    try (ResultSet res = ps.executeQuery()) {
        while (res.next()) {
             // ...
        }
    }
}

The PreparedStatement (with bind variable) has at least these advantages

1. It can use the Statement cache on the server
2. It is not prone to SQL Injection