Page 1 of 1
Does my login box contain any security flaws?
Posted: Sun Feb 25, 2007 9:25 am
by impulse()
I'm creating a very simple form with username and password and pass them to PHP via POST. From there grab any rows from a MySQL DB where the username is equal to the username that was entered in the form. I then compare the password from the MySQL result to the password entered in the form on the webpage. I have used mysql_real_escape_string for passing data to PHP from the form and I have used stripslashes to pass data from the MySQL DB to PHP.
This is how the code looks at the moment:
Code: Select all
$usernameInput = mysql_real_escape_string($_POST["username"]);
$passwordInput = mysql_real_escape_string($_POST["password"]);
$query = mysql_query("SELECT customerid, customerpassword
FROM customer
WHERE customerid = '$usernameInput'");
$NR = mysql_num_rows($query);
if ($NR > 1) {
mail("x", "x", "Function checkLogin() finding more than 1 row for username");
}
if (!$NR) {
echo "Unable to find username, please contact us<br>";
}
while ($res = mysql_fetch_array($query)) {
$usernameDatabase = stripslashes($res['customerid']);
$passwordDatabase = stripslashes($res['customerpassword']);
}
if ($usernameInput == $usernameDatabase && $passwordInput == $passwordDatabase) {
$_SESSION["authSuccess"];
}
Regards,
Posted: Sun Feb 25, 2007 11:29 am
by Mordred
It is secure, but buggy.
Remove stripslashes and compare $_POST["username"] (not $usernameInput) with $usernameDatabase (same for password).
Edit: You also have to force the registration process not to allow duplicating usernames. With that in mind, remove the check for number of rows and add LIMIT 1 to your query.
Posted: Sun Feb 25, 2007 11:35 am
by impulse()
I'm assuming I still need to leave this line of code in play:
Code: Select all
$usernameInput = mysql_real_escape_string($_POST["username"]);
Or else wont that give users access to SQL inject as they please?
Posted: Sun Feb 25, 2007 11:52 am
by feyd
Mordred wrote:Remove stripslashes and compare $_POST["username"] (not $usernameInput) with $usernameDatabase (same for password).
I can agree with the first part, but the second not at all. They must be escaped, possibly after a stripslashes() if magic quotes are on. get_magic_quotes_gpc() may be of interest, impulse().
The submitted values should probably be checked that they exist before attempting to use them. Maybe that's elsewhere?
Mordred wrote:Edit: You also have to force the registration process not to allow duplicating usernames. With that in mind, remove the check for number of rows and add LIMIT 1 to your query.
Easy fix here is by making the username field a unique key, and preferably not binary.

Posted: Sun Feb 25, 2007 12:43 pm
by impulse()
get_magic_quotes_gpc() may be of interest, impulse().
Oh, I see. I'm escaping input data if PHP doesn't do it by default, otherwise, if PHP is setup to automatically escape data then the code itself doesn't do any escaping?
Posted: Sun Feb 25, 2007 12:58 pm
by feyd
impulse() wrote:Oh, I see. I'm escaping input data if PHP doesn't do it by default, otherwise, if PHP is setup to automatically escape data then the code itself doesn't do any escaping?
PHP isn't quite that good at escaping the data as the thing receiving the data. Use the result of a positive from
get_magic_quotes_gpc() to run
stripslashes() over the variables. Then use
mysql_real_escape_string() after that.
Posted: Sun Feb 25, 2007 1:15 pm
by Christopher
I'd prefer to see something like this:
Code: Select all
$usernameInput = preg_replace('/[^a-zA-Z0-9]/', '', $_POST["username"]);
$passwordInput = preg_replace('/[^a-zA-Z0-9\-\_\=\+]/', '', $_POST["password"]);
Or check if there are illegal characters and don't do query, just report error, if there are.
And I prefer doing the escaping in the database portion of the code so I can be sure it is always being done. Otherwise there could be code path that can circumvent it.
Posted: Mon Feb 26, 2007 3:34 am
by Mordred
feyd wrote:Mordred wrote:Remove stripslashes and compare $_POST["username"] (not $usernameInput) with $usernameDatabase (same for password).
I can agree with the first part, but the second not at all. They must be escaped, possibly after a stripslashes() if magic quotes are on. get_magic_quotes_gpc() may be of interest, impulse().
The submitted values should probably be checked that they exist before attempting to use them. Maybe that's elsewhere?
I disagree with your disagreement

Maybe I wasn't clear enough. In this check:
Code: Select all
if ($usernameInput == $usernameDatabase && $passwordInput == $passwordDatabase) {
$_SESSION["authSuccess"];
}
...one must
compare $_POST["username"] (not $usernameInput) with $usernameDatabase (same for password).
Using an escaped $usernameInput in the query is still a must, but the escaped value shouldn't be used elsewhere, it's not logically correct to do so.
Agreed now? Shall I demonstrate?
Posted: Mon Feb 26, 2007 9:09 am
by feyd
Ahh, I misread your response. I thought you were solely referring to $usernameInput, while I completely missed the reference to $usernameDatabase.
Although I would probably just let the database handle that check.

It already does this for $usernameInput, actually it just doesn't do it for $passwordInput. Also, if the query returns zero records, a PHP E_NOTICE will fire due to $usernameDatabase and $passwordDatabase not being set.
Posted: Tue Feb 27, 2007 12:16 pm
by printf
To me it seems a little silly to SELECT data you already have. If your doing a login check, COUNT(*), if you have a match then you already know the username and password are valid. So SELECT customerid, customerpassword, in not a practical approach for what your doing. Also as others have said, check if magic_quotes is on, if it is, loop the expected SUPER GLOBALS and strip out the slashes and turn magic_quotes OFF. By doing this you don't need to be using stripslashes() on any database output. Also you need to validate the existents of ( $_POST['username'], $_POST['password'] ) before you even start using them. Your code also has (1) unneeded while() an (2) unneeded if(s). You can code anyway you like, but the more logical your code becomes, means the less chances you leave a opening that can be hacked, but also, and juat as important, your script execution time will be faster and the memory foot print will be a much less.
Posted: Sat Mar 03, 2007 6:25 am
by impulse()
OK I've taken all of your advice and I hope this is how my code should be looking now. Please let me know if it's still not spot on.
Code: Select all
function checkLogin() {
if ($_POST["username"] || $_POST["password"]) {
if (!get_magic_quotes_gpc()) {
$usernameInput = stripslashes($_POST["username"]);
$passwordInput = stripslashes($_POST["password"]);
}
else {
$usernameInput = $_POST["username"];
$passwordInput = $_POST["password"];
}
}
$query = mysql_query("SELECT customerid, customerpassword, failedLogins
FROM customer
WHERE customerid = '$usernameInput'");
$row = mysql_fetch_row($query);
$usernameDatabase = stripslashes($row[0]);
$passwordDatabase = stripslashes($row[1]);
$failedLogins = stripslashes($row[2]);
if ($failedLogins >= 5) {
echo "This account has been disabled due to failed login attempts. Please contact us<br>";
}
else {
if ($usernameInput == $usernameDatabase && $passwordInput == $passwordDatabase) {
$_SESSION["loggedInCustomer"] = 1;
$_SESSION["customerId"] = $usernameInput;
mysql_query("UPDATE customer
SET failedLogins = '0'
WHERE customerid = '$usernameInput'");
header("Location: {$domain}/index.php?action=137");
}
else {
mysql_query("UPDATE customer
SET failedLogins = failedLogins + 1
WHERE customerid = '$usernameInput'");
$failedNow = $failedLogins + 1;
echo "You have made {$failedNow} invalid login attempts of 5. Your account will be blocked at 5 failed logins";
echo "<form action = 'index.php?action=113' method = 'post'> <input type = 'submit' value = 'Try again'> </form>";
}
}
}
Posted: Sat Mar 03, 2007 7:46 am
by feyd
There's no escapement for the username and password supplied.
With either $_POST['username'] or $_POST['password'] existing (and not evaluating to a boolean false) the function's main code will be run.
As we've mentioned, your checking can be done by the database, entirely.
If
stripslashes() needs to be performed on data returning from the database, your insertion code needs tweaking.
Posted: Sat Mar 03, 2007 7:50 am
by Mordred
Nope
1. if ($_POST["username"] || $_POST["password"]) -- check them with isset (and possibly is_string)
2. if (!get_magic_quotes_gpc()) -- it means when they are off. When they are off, no need to stripslashes. When they are on - stripslashes.
3. Before putting $usernameInput in the query - mysql_real_escape_string
4. No stripslashes after mysql_fetch_row
5. You need to keep two versions of usernameInput - one as it came from the user and one after being escaped for mysql
This check:
if ($usernameInput == $usernameDatabase && $passwordInput == $passwordDatabase)
should use the ones as it came from the user
6. header("Location: {$domain}/index.php?action=137"); -- $domain is not set
Reread the entire thread and check if you've followed all previously pointed issues -- you will see that almost all of these here were mentioned before.
Posted: Fri Mar 23, 2007 7:31 am
by William
printf wrote:To me it seems a little silly to SELECT data you already have. If your doing a login check, COUNT(*), if you have a match then you already know the username and password are valid. So SELECT customerid, customerpassword, in not a practical approach for what your doing. Also as others have said, check if magic_quotes is on, if it is, loop the expected SUPER GLOBALS and strip out the slashes and turn magic_quotes OFF. By doing this you don't need to be using stripslashes() on any database output. Also you need to validate the existents of ( $_POST['username'], $_POST['password'] ) before you even start using them. Your code also has (1) unneeded while() an (2) unneeded if(s). You can code anyway you like, but the more logical your code becomes, means the less chances you leave a opening that can be hacked, but also, and juat as important, your script execution time will be faster and the memory foot print will be a much less.
If what you're saying is that he should do something like
Code: Select all
SELECT count(*) FROM `customer` WHERE `customerid`='$usernameInput' AND `customerpassword`='$passwordInput' LIMIT 1
then the plain password will be logged by MySQL, it's better to select the password and and then check it outside the query, so the plain version of the password isn't stored anywhere.
impulse(), you don't seem to hash your passwords either, I mean it is user preference, you might need to have them plain text, but I wouldn't recommend having the passwords stored as plain text, just in case your database is compromised in anyway. I would recommend using feyd's sha256 class, or PHP's built in sha1() function.