Does my login box contain any security flaws?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Does my login box contain any security flaws?

Post 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,
Last edited by impulse() on Sun Feb 25, 2007 11:33 am, edited 1 time in total.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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.
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post 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?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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. :)
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post 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?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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.
(#10850)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
printf
Forum Contributor
Posts: 173
Joined: Wed Jan 12, 2005 5:24 pm

Post 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.
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post 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>";
    }
  }
}
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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.
User avatar
William
Forum Contributor
Posts: 332
Joined: Sat Oct 25, 2003 4:03 am
Location: New York City

Post 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.
Post Reply