Banning user after 5 login attempts

[Gilead]

Almost there. Let me walk through things.
The username and password are brought in from the form and the admins are checked, then the members are checked. If not found anywhere, then go to this section. Prints out your ip address, checks the admins again and adds a new attempt to login. Somehow the update is not happening and I have checked the syntax and it looks right, even tried a couple tricks I've learned here, but no go.

Thanks for the help, you guys have been invaluable in the project!

echo "Wrong Username or Password";
echo '<br />';
$ip=$_SERVER['REMOTE_ADDR'];
echo 'Your IP Address has been logged!&nbsp;';
echo $ip;

$sql="SELECT attempts FROM $admin_table WHERE username='$myusername'";
$result=mysql_query($sql);
echo $sql;
echo '<br />';
echo 'result:';
echo $result;
echo '<br />';
$count=mysql_num_rows($result);
echo "count";
echo $count;
echo '<br />';
if ($count==1){
$row = mysql_fetch_array($result);
$attempts=$row['attempts'];
echo 'attempts';
echo $attempts;
$attempts++;
echo 'attempts';
echo $attempts;
$addattempt="UPDATE $admin_table SET attempts = '". $attempts. "' WHERE username= '$myusername' ";

echo '<br />';
echo $addattempt;
if ($attempts==5){
echo '<meta http-equiv="refresh" content="2;url=banned.html">';
}
}

else {
if (!$_SESSION){
$_SESSION['attempts'];
}
$_SESSION['attempts']++;

}
echo print_r($_SESSION);
if ($attempts==5){
echo '<meta http-equiv="refresh" content="2;url=banned.html">';

}

else{
$_SESSION['attempts']++;
//echo '<meta http-equiv="refresh" content="2;url=index.php">';
}
}
}

The rest of it- if it can't find you in the admin table, then it uses a $_SESSION variable incremented every time through and if it's equal to 5 then you go to the ban page. I also check on the form page, so if you have 5 attempts, you automatically go to the banned page.
After it's working, if anyone has thoughts on how to do this better, I'm here to learn.
Thanks!

[httpwebwitch]

do this:

mysql_query($addattempt);


A few things that I'm seeing right away...

1) using a <meta> for redirection is odd, you might try a header() instead, sending a 301 status and a Location body (lots of tutorials out there explaining how to do that)

2) you can increment a value in SQL without SELECTing and UPDATEing and doing it all in PHP. e.g.
UPDATE table_name SET column_name = column_name + 1 WHERE [...];

3) beware of SQL injection. Your script is vulnerable because you're not escaping strings, and you're not verifying that number values are numeric.

4) What if my browser rejects cookies?

5) when I log in successfully, are my attempts reset back to 0?

6) What does banning a person accomplish? What are you preventing? (note: it's actually normal for someone - who forgets their password - to try more than 5 times while they're racking their brains). If it's brute-force attacks you're preventing, what other ways could you detect that?

7) Are you storing passwords as an encrypted hash, or as plaintext? Because if you've got any SQL injection holes, I can pwn your entire database full of users & admins (see #3). If you hash the passwords, at least they'll be useless to me when I steal them.

8) echo print_r($_SESSION);
print_r(), without providing a second argument of true, prints. You don't need to echo it.

9) if ($attempts==5)
Is there a way it might become 6? Why not use > instead of ==

10) To log in, I'm typing my password into a <form>. Is that form submitted over HTTPS?

11) Risk assessment. The proximity to perfection required has an inverse relationship with the consequences if it's not. In other words, is this a banking app where a security hole would destroy someone's life, or is it a tic-tac-toe game with a high score board.

#11 is a touchy subject. If someone's giving you a password, you owe it to them to keep it safe. Not because you're liable for anything other than your app's data, but because there lots of people who use the same darned password for everything they do online. Hacker gets into your app and finds passwords, you bet the next thing they'll do is try the same passwords over at PayPal, and start emptying accounts. Or they'll sell the list for $200 to someone else who will. See my point?

that's probably enough lecturing for now :)

[incrediBILL]

5 attempts is too few - sometimes it takes me that many attempts to realize I'm typing it right but the capslock is on, make it 10 ;)

[Gilead]

1. I'm using the meta direction in this case because headers are sent somewhere along the way, possibly just from the variable echoing I'm doing. Is there a better substitute? Once everything is working, I'll try the header command again.

3. Where can I properly escape? The data coming in is covered:
$myusername=mysql_real_escape_string((addcslashes($_POST['username'], "%_")));
$mypassword=mysql_real_escape_string((addcslashes($_POST['password'], "%_")));
4. I'm not writing any cookies. Didn't think session variables required javascript.
5. Yes. Already confirmed that part.
6. Essentially it just slows a potential attacker down. They would have to close down their browser to clear the session.
7. In addition to thestrings and slashes, I'm also using sha1 encryption when entering into the database.
8. Okay, thanks!
9. Good idea.
10. Have to talk to the client about that.
I appreciate all your good advice. This app is a very low target for a hacker, and I know nothing is 'hacker-proof,' but I do want to make it as secure as possible just on general principle.

@Bill 10 it is.

[eelixduppy]

1. The best substitute is using header(). You should be saving the echo'ed content until after such a redirection occurs to prevent the headers from being sent automatically.

3. Remove the addcslashes, you don't need to escape it twice, mysql_real_escape_string will do this.

6. Alternatively, you can just add a timeout period in between login attempts. With a decent password policy that you enforce when users create their passwords and a few second timeout in between logins, this will essentially make any brute force attempt not worth the effort.

Also, a bit unrelated but you do not need a new echo statement for variables, you can include them within the string or concatenate them together, e.g.:

echo "$sql<br />result: $result<br/>";

Personally, though, I think it is much cleaner to use sprintf when echoing things like this out, but that's just me :)

echo sprintf("SQL: %s<br/>Result: %s<br/>", $sql, $result);

[Gilead]

If I wanted to tell them they had the wrong username or password and show them their IP address, how then can I use the header function?

[eelixduppy]

Typically this information is displayed on the login page where the user tried logging in to make it simple for them to retry their login attempt without clicking around. If you went this route you would either use something like javascript and check asynchronously if they authenticated correctly, or submit directly to your php script and then redirect back to the referring page where that page would conditionally display a warning.

If you want to show this info directly on the login handler itself, you would not be able to show a message, timeout, and then redirect using header(), you would have to use a meta refresh; however, this refresh could be disabled in a browser, and therefore you should supply a supplementary link that they can click on just in case.

[incrediBILL]

If I wanted to tell them they had the wrong username or password and show them their IP address, how then can I use the header function?

Your logic is simply upside down.

if( failed ) {
if (failed==10) {
header("Location: http://www.example.com/banned.html");
exit;
} else {
echo "Login error, IP address, blah, etc.";
}
}

[httpwebwitch]

I'm not writing any cookies. Didn't think session variables required javascript.

by default, when a PHP session is started, a cookie is dropped into the browser with the session id in it.

the session id in the cookie corresponds with an array of stuff stored on the server. That's what you're looking up with the $_SESSION superglobal.

sessions are cool because the client (browser) only has a random-looking token, but in your app you can store info like $_SESSION['username'] and $_SESSION['accesscounter'] etc.

If the browser doesn't accept cookies, then PHP sessions stop working.

There are workarounds for this, but IMHO they're ugly

[Gilead]

Would it be good to simply check for javascript and request they turn it on?

[httpwebwitch]

JavaScript and cookies are not the same thing