How secure is my login script.

[mattcairns]

I'm making an online directory for a client, which is storing some very sensitive information. Please let me know how secure this script is, and if it sucks how to make it better.

first the login validation
[code]
if($_POST["submit"] == "Y"){
  $login = $_POST["login"];
  $password = $_POST["password"];
  $sql = mysql_query("SELECT * FROM `login_users` WHERE `login` = '$login'");
  $numRows = mysql_num_rows ($sql);
  $result = $sql or die(mysql_error());
  if($numRows == 0){
      $status = "<br><div align='center'>Invalid user name.</div>";
  }else{
      while($row = mysql_fetch_array($result)){
        $passwordCheck = $row["password"];
        $id = $row["id"];
        $name = $row["name"];
        $email = $row["email"];
        if($password == $passwordCheck){
            session_start();
            header("Cache-control: private");
            $_SESSION['confirmed'] = "1";
            $_SESSION['userID'] = "$id";
            $_SESSION['userName'] = "$name";
            $_SESSION['userEmail'] = "$email";
            header("Location: $hosturl/HTML/calendar.php");
        }else{
            $status = "<br><div align='center'>You entered the wrong password.</div>";
        }
      }
  }
}
[/code]

next is the code which determines wheter or not you can view the page:

[code]
session_start();
header("Cache-control: private");
if ($_SESSION["confirmed"] != "1"){
  header("Location: $hosturl/index.php?log=failed");
}
[/code]

thank you for any suggestions.

[.josh]

you should sanitize $login before using it in your query.

[code]
function clean_var($value){
  if (get_magic_quotes_gpc()) { stripslashes($value); }
  if (!is_numeric($value)) { mysql_real_escape_string($value); }   
  return $value;
}
.
.
$login = clean_var($_POST['login']);
[/code]

also, this isn't really a security issue , but you can combine these 2:
[code]
  // combine these 2:
  $sql = mysql_query("SELECT * FROM `login_users` WHERE `login` = '$login'");
  $result = $sql or die(mysql_error());

  // like so:
  $result = mysql_query("SELECT * FROM `login_users` WHERE `login` = '$login'") or die(mysql_error());
[/code]

[utexas_pjm]

You might want to think about hashing (md5) your passwords instead of storing them in plain text.

[mattcairns]

thank you very much, i was planning on using md5 have just been too lazy to put it in.  ;)

i was also curious as to why i would want/have to sanitize the login? is this something i should do on all text input fields, or just login fields?

[utexas_pjm]

Anything that you're going to put into a SQL query via PHP should be "sanitized" to make sure that someone doesn't inject malicious SQL into your forms.

"Sanitizing" usually consists of removing single quotes so that an end user can't enter stuff like

[code]
BSUser' or 't'='t
[/code]

into your username field.

[insrtsnhere13]

sorry to thread jack but is that all that is required to treat forms?  what about forms with larger content such as a message or forum post?

[utexas_pjm]

Perhaps I should have said escape single quotes instead of remove.

[ToonMariner]

use sha1 instead of md5 - its a bit stronger.

also look at using mysql_real_escape_string to sanitize your input - you can alos use that to prevent mysql injection (and detect it and log the offenders ip address etc etc)

[Accurax]

Md5 and Sha1 are a deterrant .... if someone manages to get hold of the hash it is neither difficult, nor time consuming to break it.... even adding a salt only increases the time it takles for modern cracking algorithms to truck through..... you should still do it though :)

[redbullmarky]

MD5/SHA1. both do the trick. but if they can manage to get hold of even your MD5'd password, then unfortunately they've probably gotten far enough into your system to make your stolen encrypted password the last of your worries.
sure - do it anyway. like Accurax said, it's a deterrant and covers certain line of attacks. but if someone (and i have tried this out during tests) can get through your frontline using an SQL injection or similar, then the fact that the password is encrypted makes little difference. If you can get as far as seeing it (normally by inserting a SELECT statement amongst a few other tricks), then there's no restrictions on CHANGING it (notably with an admin account) to a totally new MD5 that you DO know what its original form is, and voila - you're in.

So - whilst its easy to think MD5 is a good defence - what Crayon Violent and utexas_pjm said is going to lock things down much better than simply encrypting your passwords alone. Pay close attention to a) magic_quotes (google will help you out here) b) making sure data send via a form or the URL is safe for the DB ([url=http://www.php.net/mysql_real_escape_string]mysql_real_escape_string[/url] as used in C_V's first example above) but also make sure that, if you are displaying pure user input to the screen or re-filling input boxes that the data has been properly santised ([url=http://www.php.net/htmlspecialchars]htmlspecialchars[/url], for example here).

It might seem like alot, but security is nothing to be taken lightly. I had one of my sites breached, and cleaning up afterwards isn't fun. Too many people are happy enough to chuck a couple of input boxes on the screen, label them "username" and "password", and compare them to whats in the database on submission without considering what can/will be done if the wrong person comes along.
the effort to not just learn about these coding measures, but to actually find out the ins and outs of why/what/when, etc, pays off, too. without sounding like the police or something, laws are ever changing to make sure that everyone that stores user data on a website takes every measure to lock it down.

[Ninjakreborn]

Believe me, clean those variables I learnt the hard way, if you saw my recent post here about being hacked.

[utexas_pjm]

FYI this me going off on a tangent...

[quote]
Md5 and Sha1 are a deterrant .... if someone manages to get hold of the hash it is neither difficult, nor time consuming to break it.... even adding a salt only increases the time it takles for modern cracking algorithms to truck through..... you should still do it though :)
[/quote]

While I understand the sentiment in warning that implementing MD5 or SHA1 hashing a password is not all that is necessary to secure a script, I think the above quote might be a bit misleading.  I know that computer scientists have de-ciphered MD5 and SHA1 however saying that is it neither difficult nor time consuming to break it is a bit of a stretch. 

If I'm completely wrong please show me the light ;).

[jsladek]

I've been taught the same thing.  It is no quick crack but it really depends on the complexity of the password.  Most crackers will run a dictionary attack and even use substitutions for letters, much like you would see in Leetspeak. i.e. $=s and 3 = e that way a passwork  like $lad3k is no sronger that sladek.  If the password is complex enough then the cracker will have to use brute force and hope that they can come up with it or a collision ( I think that is what they call it when a different password produces the same hash but this is rare).  This could take a very long time...  As an alternative, there are websites out there where users can put in hashes to have them try to crack. They have groups of servers working on just that.  If the hash has already been broken then the cracker can get it right away.

The last security class I has was 4 - 5 months ago, maybe something has changed....

-John

[redbullmarky]

right on both counts - but like i say, you should wonder how someone even knows your MD5'd password, never mind how/why/when they'll turn it into something useful.

absolutely guaranteed that if i was to play around with a website, and get as far as getting an MD5'd password, i'm in far enough to do pretty much anything I feel like. Adding new admin users with a password i know? not a worry. Changing pages? easy.

MD5, etc, should really be one of your LAST lines of defence. Too many people (and not implying you guys in any way) really do forget that, and think that having their password encrypted to the teeth with super-duper 22nd century encryption will save them from a bit of poor form coding and lack of proper validation.

Cheers

[mattcairns]

First off thank's for all the help and suggestions. I've started to implement a lot of the things, I just have a few more points I wanted to ask about.

-- Can I assume that if my login script is strong, I needn't worry about sanitizing form entries within the site, since only registered users (keeping my fingers crossed) will be accessing them. I ask this because I would like the user to be able to enter quotes and apostrophes. If this is not advised, please give me some suggestions about allowing this.

-- Secondly I have created a process page which process all of my forms (text entries, files uploads, etc...). Would it be smart to do all processing from the index page on itself as to not give away too much information?

I'm glad you mentioned uploading the files to a non web accessible folder and loading them in, I wasn't sure how I was going to protect .pdf's and such, so thank you very much.

[alpine]

It is important to secure all forms regardless of the user status (logged in or not), and don't forget either to secure a forgotten password form, register form etc. just as much as the login form.
I would also like to point out that using "common" table and field names like "users", "username" etc. also makes it easier if anyone should sneak in som injection in an "open" environment. This would probably be the first combinations to go off.
Also, using "or die(mysql_error())" etc. will revield pretty detailed information about your table structure if an illegal injection (that is a query) is made. Using "or die()" wouldn't give any hints.

About storing quotes etc., that is really no problem - just use htmlspecialchars($var, ENT_QUOTES) or htmlentities($var, ENT_QUOTES) and it will work securely transforming them to their html entities.

BUT, the best is (in addition) to restrict the user input and accept only data that you are expecting and allowing, for instance don't allow single quotes to make it into a query in a username/password check - match all form input against patterns and deny it before it gets to the db. The same patterns will ofcourse be needed in a register form etc. so that any illegal chars isn't registered either.

Just a few cents