Login script

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
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Login script

Post by social_experiment »

I have two parts of code i need help / advice and suggestions on. I am trying to create a secure login and authentication system for an administrative back-end. Nothing serious to protect, i just dont want any unwanted users reading and / or snooping where they dont belong.

Firstly, the code that does the login. This is pre-production hence the comments and un-squashed mysli statements and odd mysqli_error().

Code: Select all

 
<?php
 #rm comments when in use.
 ob_flush();
 # starts the session
 session_start();
 
 # removing all $_SESSION elements
 $_SESSION[] = array();
 
 # contains the data needed to connect to the mysqli database
 require('admin_details.php');
 
 # if empty fields are submitted for either of the username or password, the user is redirected back to the login form
 if ( empty($_POST['pass_word']) || empty($_POST['user_name']) ) {
  header('location: login_form.php');
 }
 else {
 # both fields have been submitted with content. 
  $connection_string = mysqli_connect(HOST, USER, PASS);
  if ( !$connection_string ) { 
   #send mail indicating an error trying to connect to the db
   $err_date = date('Y-d-m H:i:s');
   $subj = 'Error while trying to login to the admin system.';
   $msg = "An error was encountered while connecting to the database at $err_date";
   $headers = "From: SITE";      
   @mail(ADDY, $subj, $msg, $headers);
   }
  else {   
   $select_database = mysqli_select_db($connection_string, DB);
   
   if ( !$select_database ) { echo mysqli_error(); }
   else {
   
     $salt1 = hash('sha256', addslashes($_POST['user_name']));
     $salt2 = hash('sha256', addslashes($_POST['pass_word']));
     
     $select_string = mysqli_query($connection_string, "SELECT * FROM admin_user WHERE username_field = '".hash('sha256',$salt1)."' AND password_field = '".hash('sha256',$salt2)."' ");
     $resultant = mysqli_num_rows($select_string);  
     #if a match is found against the username / password 
     if ($resultant == 1) {
      #send email indicating a login was made.
      $login_date = date('Y-d-m H:i:s');
      $subj = 'Administrator Login confirmed';
      $msg = "A login was made at $login_date from IP ".$_SERVER['REMOTE_ADDR']."";
      $headers = "From: SITE";      
      @mail(ADDY, $subj, $msg, $headers);
      
      session_regenerate_id();
       
      $administrator_session = mysqli_fetch_object($select_string);       
      $_SESSION['access_key'] = $administrator_session->id;  
      #create random fingerprint     
      $random_num_max = 123456789;
      $salt = mt_rand(0, $random_num_max);  
      $fingerPrint = hash('sha256', $salt);
      $_SESSION['fingerprint'] = $fingerPrint;
      #close mysqli connection
      mysqli_close($connection_string);
      session_write_close();
      header("location: admin_index_page.php");
      exit(); 
     }
     else { 
      header("location: login_form.php");
     }   
   }  
  }
 }
 
 ob_end_flush();
 ?>
 
Secondly, the code that checks that the user has actually logged in ( auth code ).

Code: Select all

<?php 
 session_start();
        
 if ( !isset($_SESSION['access_key']) || 
      trim($_SESSION['access_key'] == '') ||
      !isset($_SESSION['fingerprint'])
      || strlen($_SESSION['fingerprint']) != 64
       )         
 {      
 $_SESSION[] = array(); 
 session_regenerate_id();
 header("location: error.php"); 
 }
    
?>
I also have a few additional questions. Is passing the session_id() in a URL ( for a system with a back-end ) a good practise? And the mysqli queries in use in my script, are they secure enough or should there be more escaping anywhere in the script?

Thanks in advance.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

Just a note. Sending a header request does not stop the current execution. You are using { and } to separate the execution flow, so, nothing bad can happen there, but you should be careful and remember that header() is just another function call and doesn't stop the execution.

Can I ask you why would you want to remove comments when "in use"?

Code: Select all

$_SESSION[] = array();
Actually does not clean the session array. Remove the [].

The salts are poorly constructed. Salts should be (pseudo) random. If you are interested and you have some time, try to learn something from my blog post: http://www.phptalk.net/2009/01/27/all-a ... d-entropy/

If you want to tighten up your security I suggest adding a brute force protector that logs IPs and doesn't allow more than a few attempts per IP.
User avatar
omniuni
Forum Regular
Posts: 738
Joined: Tue Jul 15, 2008 10:50 pm
Location: Carolina, USA

Re: Login script

Post by omniuni »

Ooh! I remember the original post on this!

From the comments on your blog post Kai:
It is impossible to learn anything in school unless you are in the United States or in Germany.
I'm in the U.S., and it's difficult for me to track down a professor that would be able to help me in such an area, even though we're one of the top 5 engineering schools (at my last check). Maybe I should try moving to Germany :?

OK, OK, that was :offtopic:

Anyway, I may be missing something, but I get the distinct impression that you could simplify this somewhat. For example, you could try session_destroy() to clear session data. Pull your hashing/encryption stuff into a function, and then you can do plenty of complex stuff to your salts without having to rewrite pieces of the script other than the function. Also, if memory serves me, it's usually a good idea to do something to session variable keys that makes them harder to access than say $_SESSION['fingerprint'], for example, hash the users IP address and use 'fingerprint_$IpHash' instead. (If I'm wrong, and it makes no difference whatsoever, someone please correct me.)
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

@omniuni: I read some Wikipedia to find out schools which teach about that kinds of things and either a) I read wrong b) source was wrong or c) I was drunk, either way it is actually discussed in the comments section at the bottom of the page. Maybe I should update the post at some point.
User avatar
omniuni
Forum Regular
Posts: 738
Joined: Tue Jul 15, 2008 10:50 pm
Location: Carolina, USA

Re: Login script

Post by omniuni »

@Kai: We actually have the professors, and they're awesome if you can get in touch with them, it's just that the classes that teach it are difficult to get into because of all the prerequisites you need. I ended up moving out of Computer Science into Technical Education because I have trouble with Calculus, and besides that I was not really learning anything in the programming classes. Just Java, and not useful Java at that!
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login script

Post by social_experiment »

Valid point about the comments, ill keep them there regardless. Thank you :)

Strangely enough, i devised those salts after reading your article on randomness and entropy. How can i secure them more? Maybe by removing the $_POST[] information from them and adding something similar to them like i did with the $fingerprint.

I'll rm the [] from $_SESSION[] = array().

omniuni, thank you for the note on using a function for creating the salts.
Also, if memory serves me, it's usually a good idea to do something to session variable keys that makes them harder to access than say $_SESSION['fingerprint'], for example, hash the users IP address and use 'fingerprint_$IpHash' instead. (If I'm wrong, and it makes no difference whatsoever, someone please correct me.)
Would hashing the session_id() be a good idea here? And use it in a similar manner as $_SESSION['fingerprint'] inside the auth code? Or ( and please correctly if im understanding this incorrectly ) use $fingerprint and combine it with a hashed version of the IP?

Thanks for all the help.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

The purpose of a fingerprint is to strengthen session security. Usually you would

1) Create an unguessable/random fingerprint after a successful logon.
2) Store it in the $_SESSION.
3) Pass it through URIs (GET).

And whenever a page is requested, if $_GET['fingerprint'] doesn't match $_SESSION['fingerprint'], the request needs to be rejected. If you do this, then anyone getting your session identifier will not yet be able to get into your session - he still needs the valid fingerprint. That's how fingerprints are useful. However, there are drawbacks to this scenario; each link have to contain the fingerprint (and forms have to contain it in the target path) and also, leaving/closing the website will make the user to lose his fingerprint - unusable for "automatic logins". If you have a dynamic fingerprint that changes, you would create your application a ton harder to crack, but AJAX and Back button would be unusable.

A strong fingerprint is not a hash of any user supplied information. A good fingerprint is as random as possible. On Linux, you can just read /dev/urandom to gather strong random data or you can use openssl_random_pseudo_bytes() if you are using PHP 5.3.

The same thing applies to salts; they have to be generated using a stronger random source. A hash of a user supplied username is not random.
User avatar
omniuni
Forum Regular
Posts: 738
Joined: Tue Jul 15, 2008 10:50 pm
Location: Carolina, USA

Re: Login script

Post by omniuni »

Thanks, Kai. I'm bookmarking this topic!
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login script

Post by social_experiment »

I see. Thanks for the information. I guess while creating the script i forgot about how nonrandom the username and password actually is ( even though they might be hashed ). That part of the script i will definitely change.
And whenever a page is requested, if $_GET['fingerprint'] doesn't match $_SESSION['fingerprint'], the request needs to be rejected. If you do this, then anyone getting your session identifier will not yet be able to get into your session - he still needs the valid fingerprint. That's how fingerprints are useful. However, there are drawbacks to this scenario; each link have to contain the fingerprint (and forms have to contain it in the target path) and also, leaving/closing the website will make the user to lose his fingerprint - unusable for "automatic logins". If you have a dynamic fingerprint that changes, you would create your application a ton harder to crack, but AJAX and Back button would be unusable.


So if the fingerprint is passed along in the URL it would be wise to have to other things to check against when validating the page, like the $_SESSION['access_key'], so if an attacker does get their hands on the fingerprint they would still need other information to access the session.

But ( and here im going out on a limb ) once i create a fingerprint, and it is destroyed or unset when the user logs out, the session times out or the browser is closed, then that specific fingerprint will become useless?

Thanks again for the suggestions and pointers.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

social_experiment wrote:So if the fingerprint is passed along in the URL it would be wise to have to other things to check against when validating the page, like the $_SESSION['access_key'], so if an attacker does get their hands on the fingerprint they would still need other information to access the session.
What is that "access key" of yours?
social_experiment wrote:once i create a fingerprint, and it is destroyed or unset when the user logs out, the session times out or the browser is closed, then that specific fingerprint will become useless?
If you destroy the session, then the fingerprint dies along with it (I'm assuming you store the fingerprint in the session). So, if you kill the session that it is tied to, there's no use for the fingerprint any longer.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login script

Post by social_experiment »

Im using the row id from the mysql_query as the access key.

Code: Select all

$administrator_session = mysqli_fetch_object($select_string);       
         $_SESSION['access_key'] = $administrator_session->id;
And yes, i do store the fingerprint in a session variable.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

Let me rephrase: what are you trying to achieve with the use of an access key?
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login script

Post by social_experiment »

Im using it as an additional check ( $_SESSION['access_key'] ) within the authorise code, along with the $_SESSION['fingerprint'] variable.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Login script

Post by kaisellgren »

I don't see any use for the session access key you have there.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Login script

Post by social_experiment »

I've changed a few things in the script ( after reading the article about hashing. ) -

Code: Select all

$salt_select = mysqli_query($connection_string, "SELECT salt FROM salt_table");
 
   while ( $salt_obj = mysqli_fetch_object($salt_select) ) { 
   $salt = $salt_obj->salt;
   }
I created a salt table, which will house a dynamic salt that is changed each time the administrator has logged on successfully.

Code: Select all

$match_string = mysqli_query($connection_string, "SELECT id FROM admin_user WHERE username_field = '".hash('sha256',$_POST['user_name'].$salt)."' AND password_field = '".hash(   'sha256',   $_POST['pass_word'].$salt)."' ");
The poor salts based on the username && password have been removed. I instead used the following to create the salts ( as per the article )

Code: Select all

$salt = hash('sha256', uniqid(mt_rand(), true));
 $hashed_username = hash('sha256', $login_value.$salt);
This creates a 64 character hash which is stored within the database. ( for both the username && password ). Once the salt is updated, so is the hashes of the username and password.

Code: Select all

#example1
$salt = hash('sha384', uniqid(mt_rand(), true)); 
      $fingerPrint = hash('sha384', $salt);
      $_SESSION['fingerprint'] = $fingerPrint;
If i were to use the following code :

Code: Select all

#example2
$salt = hash('sha384', uniqid(mt_rand(), true)); 
      $fingerPrint = hash('whirlpool', $salt);
      $_SESSION['fingerprint'] = $fingerPrint;
would the hash be more secure than in example 1?
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
Post Reply