Did I miss any vulns here?

Question

I recently took a beating in a Security Engineer interview and I did not get any specific feedback on how I did. I'd like to re-ask the question to the community so I can polish up on these elements. My list of vulns found are below.

You are at a login page, two form elements User: / Password: Your username is Peter.

Upon Successful login of the app, you are directed to a page that says Hello 'Peter'. ( The source of 'Peter' on this page comes from the user variable populated at the login username form )

The generic code for this is as follows:

public String login( String user, String pass ) { 
       if (pass == DB.lookup(user)){ 
            return "Hello " + user; 
           }
       else { .....} 
       } 

1. What are all of the vulnerabilities present in this sample login application?

2. How would you fix them?

3. How would you store your passwords?

4. If you store them that way, by what mechanism do you recover the password to validate it against what the user typed into the password form?

   Here is what I see so far:

XSS: The fact that user input (user) is pushed into the responding jsp page without going through an encoding library or validator means you are owned.

Fix: I would implement the sanitizer from the OWASP java encoder project into the HTML element.

I would store the passwords as a salted hash using bcrypt. A unique salt for each hashed password.

Answer 1

Nobody's yet explicitly mentioned that there's potential for SQL injection. The user string needs proper validation to ensure that its present and contains just a valid username.

Answer 2

Without seeing the code on the server, I can't be 100% sure. Especially since I don't know what DB.lookup() does. For all I know, it could handle many of these security issues.

public String login( String user, String pass ) { 
   if (pass == DB.lookup(user)){ 
        return "Hello " + user; 
       }
   else { .....} 
   } 

What are all of the vulnerabilities present in this sample login application? How would you fix them?

1. No timeout on attempting to login by IP/cookie, and none by user. Easily allows both a Denial of Service (even DDoS) attack, and a brute-force attack.
2. No Checking whether or not user or pass are null, and therefore of the proper length and in the right format to be checked against the database. What if the page returns an error, and allows you to try and figure out more through trial and error?
3. Possible SQL injection attack on DB.lookup(user), but it's also possible that DB.lookup() handles this.
4. return "Hello " + user; Again, because there's no proper checking, you can use this to inject anything into the server's page. This means you could run rogue ASP/JSP/PHP/whatever code.
5. Again, without seeing the DB.lookup() code, I can't tell what it does, but I can make an assumption. You appear to be comparing pass to the username. This could mean you may be able to try to login with the password being the same as the username, and use it to log in, without having their password. You could login with login("Jake", "Jake");, and it would work.

How would you store your passwords?

In the database, not anything the user should have access to, using bcrypt / hashed+salted (unique, obviously) + a reliable SlowEquals() password. A properly-implemented SlowEquals/XOR will prevent side-channel timing attacks.

If you store them that way, by what mechanism do you recover the password to validate it against what the user typed into the password form?

Check if the password input is not null, is of proper length for a password, and does not contain unallowable characters. Three attempts maximum. You check if the salted hash corresponds to the password.

Regarding both user and pass, you must check to see if they are not null, are of the proper length for each field in the database, and that they're in the proper format. To check the proper format, you can use regex to ensure it's alphanumerical with the correct symbols. Or you can strip all non-used characters.

And don't forget, you need to test both the username and password against DB.lookup() Otherwise you can login with the user's username as the password! Horrible!

The more secure code might look something like this (untested pseudo code, btw):

Pseudo class for DB:

public static class DB
{
     public static boolean lookup(string u, string p)
     {
          // Test salted hash against the user's hash for that particular username.
          return (encryption.testPassword(p, getUsersHashedPassWord(u))) ? true : false;
     }
}

The error-checking pseudo-code:

private boolean properLength(String u, String p)
{
     return ((u.length > 3 && u.length <= 12) && (p.length > 8 && p.length <= 30)) ? true : false;
}

private boolean properFormat(String u, String p)
{
    return (regex.Valid(u, usernameRegex) && regex.Valid(p, passwordRegex)) ? true : false; 
}

private String stripBadStuff(String stuff)
{
    // Just in case or something...
    return EncodingFunction.ToASCII(stuff).regexReplace(badCharacaterRegex, ""); 
}



public String login( String user, String pass ) 
{ 
   if (!loginTriesExceeded) // Currently unhandled for example.
   {
       if ((user != null && pass != null) && properLength(user, pass) && properFormat(user, pass))
       {
            // Unhandled for example. This is so they can't change their IP address to continue trying.
            if (!userLoginTriesExceeded)
            {
               String newUser = stripBadStuff(user);
               String newPass = stripBadStuff(pass);
               // Would be changing DB.lookup() to return true if valid login is detected. DB.lookup(newUser,newPass) will now be assumed that it tests the plaintext password against the salted hash. 
               if (DB.lookup(newUser, newPass)) 
               { 
                    // Prevent injection. This will assume DB.lookup() tests the password against the stored hash.
                    return "Hello " + newUser; 
               }
               else 
               {
                    return "Invalid login.";
               }
           }
           else
           {
                return "User login attempts exceeded.";
           }
        }
        else
        {
             return "Invalid login";
        }
    }
    else
    {
        // Do nothing, or inform the user that they've exceeded max logins.
        return null; // So all code paths return a value. 
    }
} 

Remember, that is untested pseudo code. There may be some things I've missed.

Answer 3

Vulnerabilities I can see:

1. Here it may not count as a vulnerability (as such), but the password does not seem to be hashed in that code.
2. String comparison using == is vulnerable to statistical timing analysis.
3. The XSS you found.
4. I'll assume DB.lookup is safe to use like this without getting SQL injection vulnerabilities. Still the error handling is missing, as pointed out by @SourLolita.
5. There does not seem to be any XSRF protection, it's not visible in that code at least. I don't know how to exploit that in this case.

How to fix them:

1. Hash passwords using a password hashing algorithm, like bcrypt or scrypt.
2. Use the password hashing library comparison/checking function.
3. Pass the name through an appropriate escaping function hopefully provided by the framework in use.
4. Add an error check. If user names are secret, timing immunity applies here as well, and then it will be complicated.
5. I would have to read up on that. It's been a while. One way is to have a special random token submitted with the form and as a cookie and require that they match.