# What is the security issue in this code?

I was asked how this code has a security risk. Does anyone have any ideas what it is? I am new on the security topic and don't know what to look for.

String DBdriver = "com.ora.jdbc.Driver";
String DataURL = "jdbc:db://localhost:5112/users";
String passwd = "codeReview";
Class.forName(DBdriver);
Connection conn = DriverManager.getConnection(DataURL, loginName, passwd);
int iUserID = -1;
String sel = "SELECT UserID, Username FROM USERS WHERE Username = '"
Statement selectStatement = conn.createStatement ();
ResultSet result = selectStatement.executeQuery(sel);
if (result.next()) {
iUserID = result.getInt(1);
}
PrintWriter wr = response.getWriter ();
if (iUserID >= 0) {
wr.println ("you logged in: " + loginUser);
} else {
}

• I guess you're not acquainted with little Bobby Tables. Dec 9 '11 at 6:49

Off the top of my head:

1. Those parameters don't seem to be escaped before being stitched into a query (opening up SQL injection attacks)
2. The database seems to be keeping passwords in plaintext rather than as salted hashes (which is an especially bad combo with the previous point)

You'd really want to

• escape the username and password before dropping them into a SELECT statement
• keep the password stored as a hashed value at minimum (you'd then want to verify the users' input by hashing the password you get from them and comparing that to what you have stored). Ideally, you'd salt them too.

It wouldn't hurt to accept the login info as a POST parameter either (just to reduce the number of logs username/password combos would end up in).

• how can i escaped to prevent sql injection, any example? Dec 9 '11 at 3:31
• @Kit Ho - Sorry, not my language. SQL-related functions/methods tend to be available everywhere though; check the docs rather than rolling your own. Dec 9 '11 at 4:31
• Rather than escaping parameters, I would use setNString. Dec 9 '11 at 5:32
• @KitHo: OWASP provides a cheat sheet with some sources, but you should also review their top 10 for self-education: owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet Dec 9 '11 at 18:05

Some other notes:

1. Don't mix naming styles:

String DataURL = "jdbc:db://localhost:5112/users";


Variable names should be camelCase (with lowercase first letter).

2. Instead of checking that iUserID is -1 or not check the result of result.next() and set a boolean flag:

final boolean hasUser = result.next();
if (hasUser) {
iUserID = result.getInt(1);
} else {
}


Or even better:

final boolean hasUser = result.next();
if (!hasUser) {
return; // etc.
}
final int userID = result.getInt(1);


It removes magic strings from the code and makes it easier to read.

Note that I moved the variable declarations too. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables has a good overview on this topic. (Google for "minimize the scope of local variables", it's on Google Books too.))

3. Instead of the ResultSet.get*(int columnIndex) methods use the ResultSet.get*(String columnLabel) ones:

userID = result.getInt("UserID");


It also removes some magic numbers from the code and makes it easier to read.

4. This:

PrintWriter wr = response.getWriter ();


should be:

PrintWriter wr = response.getWriter();


(From here):

Note that a blank space should not be used between a method name and its opening parenthesis. This helps to distinguish keywords from method calls.

5. I'd use longer variable names for readability:

PrintWriter writer = response.getWriter();

• so is that the main insecure issue is that the SQL injection? Dec 10 '11 at 16:51
• Yes, I agree with @Inaimathi. I haven't found any more security issue. Dec 10 '11 at 23:30

There are three issues, from least to greatest.

1. The connection info to the database is in your code. Should be in a separate configuration file so that it can be changed and the developer doesn't need to know the production password. Depending upon the setup (single dev does dev and dba for instance), this may not be a big issue.

2. SQL injection vulnerability, a username of '; drop table users; -- might just ruin yor day.

3. You are storing the users password in plaintext. You should never store the users password, you should be storing a hash of the users password. Users recycle passwords. If you store the password, and your system is hacked (or you have a disgruntled employee), that can be used to access their other accounts. Such a breech might open your employer to a great deal of liability... Because this effects multiple systems, it is the most serious.