5

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.

  • The question "what are all the vulns" is too broad - you need to narrow it down – schroeder Nov 10 '15 at 22:23
  • It also depends somewhat on how this method is called, and what is done with the return value, how the DB.lookup() method is implemented, etc... – AviD Nov 10 '15 at 22:50
  • 2
    DB.lookup should have a null check. When an unused name is used it will either break, return a string indicating it's bad, or return null. If Peter isn't in the DB you can login without a password or with whatever value is returned for a bad username. – user72066 Nov 10 '15 at 22:54
  • Thanks AviD and Sour Lolita for your input. Looks like someone down voted me. Boo. This is my first time asking a questions... did I ask something dumb? – Taken Beatings Nov 11 '15 at 5:11
  • 1
    @NeilSmithline a snippet of pseudocode and the question "what are all the vulns?" is way too broad. The OP has added what he could see, and that improves things. – schroeder Nov 11 '15 at 18:06
3

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.

  • 1
    You could just use a query api without string concatenation problems. – Simon Lindgren Nov 12 '15 at 12:11
  • 1
    Correction: you should use parameterized queries to prevent string concatenation problems. – d0nut Nov 12 '15 at 15:04
3

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.

  • 1
    You could always omit the return "Login attempts succeeded"; line if you feel that way. This is used to prevent people from changing their IP/deleting their cookies, and attempting to brute force. – Mark Buffalo Nov 12 '15 at 14:59
  • 1
    I doubt that's what DB.lookup(user) did. It wouldn't even make sense to make a DB call to just retrieve the value you put in. It logically makes more sense that the intent of that call was to retrieve the password. Although the issue that you suggested probably wasn't an issue originally, the issue suggested by Sour Lolita, "DB.lookup should have a null check. When an unused name is used it will either break, return a string indicating it's bad, or return null." definitely is. – d0nut Nov 12 '15 at 15:11
  • 1
    Either way, this still ends up looking a lot better than OP's original code. I'll stop pestering you over nit-pick stuff ;P – d0nut Nov 12 '15 at 15:12
  • 1
    No, it's fine. And it doesn't make sense for the DB.lookup() to behave the way I'm describing, I agree. It also doesn't make sense for the programmer to code the way they did. ;) – Mark Buffalo Nov 12 '15 at 15:16
  • 1
    You know what they say: Peer reviewed code is the best code. :D – d0nut Nov 12 '15 at 15:53
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.