What is the security issue in this code?

Question

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 loginName = "stackoverflow";
String passwd = "codeReview";
Class.forName(DBdriver);
Connection conn = DriverManager.getConnection(DataURL, loginName, passwd);
String Username = request.getParameter("USER");
String Password = request.getParameter("PASSWORD");
int iUserID = -1;
String loginUser = "";
String sel = "SELECT UserID, Username FROM USERS WHERE Username = '" 
    +Username + "' AND Password = '" + Password + "'";
Statement selectStatement = conn.createStatement ();
ResultSet result = selectStatement.executeQuery(sel);
if (result.next()) {
       iUserID = result.getInt(1);
       loginUser = result.getString(2);
}
PrintWriter wr = response.getWriter ();
if (iUserID >= 0) {
       wr.println ("you logged in: " + loginUser);
} else {
       wr.println ("you cannot login in !Access Denied!")
}

Answer 1

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).

Answer 2

Some other notes:

1. Don't mix naming styles:

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

   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);
       loginUser = result.getString(2);
   } else {
       wr.println ("you cannot login in !Access Denied!");
   }
   

   Or even better:

   final boolean hasUser = result.next();
   if (!hasUser) {
       wr.println ("you cannot login in !Access Denied!");
       return; // etc.
   }
   final int userID = result.getInt(1);
   final String loginUser = result.getString(2);
   

   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");
   loginUser = result.getString("Username);
   

   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();
   

Answer 3

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.