Is my Java code still vulnerable to SQL injection?

Question

We have a Java web application that was vulnerable to blind SQL injection attacks. Our developers fixed the code by using the replaceAll() function to convert single quotes to two single quotes. I am trying to understand whether the following lines of code would still be vulnerable to SQL injection attacks?

String userInput1 = in.getuserInput1();
check = (String)form.get("userInput1");
String userInput2 = (String)form.get("userInput1_express");
userInput2 = (userInput2 == null) ? "=" : userInput2.replaceAll("'", "''");
if (userInput1 != null && !check.trim().equals("")) {
    iQueryObject.addQuery("in.userInput1", userInput1.replaceAll("'", "''"), userInput2, null);
    sbSelect.append("AND WWXX_YYYYMM " + userInput2 + " '" + userInput1.replaceAll("'", "''") + "' ");
}

So far, we have tried to scan the updated application using BurpSuite and sqlmap, both have been unable to identify any SQL injection issues.

The back-end database being used is Oracle.

The flaws of this method are addressed in this answer to No single quotes is allowed, Is this SQL Injection point still exploitable?. In short, it could be exploitable depending on the database backend. Apart from that, don't just fix a broken design by making it slightly less broken. Instead use the right design, i.e. statements with parameters. See bobby tables for more on this. — Steffen Ullrich, Jan 30 '20 at 15:00
Also at stackoverflow.com there is a highly voted question which asks exactly this too: Can I protect against SQL injection by escaping single-quote and surrounding user input with single-quotes?. — Steffen Ullrich, Jan 30 '20 at 15:10

score 3 · Accepted Answer · answered Jan 30 '20 at 15:03

3

Yes, your filtering on userinput2 is insufficient, the use of a single quote is not necessary for exploitation with that syntax. I didn't analyse the code any further. "Blacklisting" characters is not a reliable defense against SQL injection. You should alter the code to use parameterized queries instead.

answered Jan 30 '20 at 15:03

wireghoul

5,959
2
18
26

Slightly more specifically: since userInput2 isn't inside a string to start with, there's no reason the attacker would need to use a ' to get out of the string that they aren't in. – user253751 Jan 30 '20 at 18:16

Is my Java code still vulnerable to SQL injection?

1 Answers1