Php Login Class - Security and Logic check

Question

I write a simple php login class that only operate on php sessions to login user in the system.

My Question is that this class is secure enough to be used on production environment ?

NOTE : Die Statements are used only for testing of scripts

class.login.inc.php

class Login
{

//setSessionFunction sets a login session with user id and all user details array

private function setSession($user_id,$user_details)
{
  $_SESSION['user_id'] = $user_id;
  $_SESSION['user_details'] = $user_details;
  $_SESSION['key'] = $this->hashSession();
}

//GetIp function is used to get ip address of client
private function hashSession()
{
return sha1($_SERVER['HTTP_USER_AGENT'] . getIP());
}


//get sessions and verify them
public function getSession(){

if(isset($_SESSION['user_id']) && ($_SESSION['user_id']!== '') )
 if($this->verifySessionUser($_SESSION['user_id']) === true )
  if($this->hashSession() == $_SESSION['key'])
return true;
else
 return false;


}


//Logout Function 
public function logout(){
   $_SESSION['user_id'] = null;
   $_SESSION['key'] = null;
   $_SESSION = array();
   session_unset();
   session_destroy();
}
public function verifyUser($username,$password){
  $password = sha1($password);
  $conn = new mysqli(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_DATABASE);
  if(!$conn) die("Connection Error To DATABASE" . mysqli_connect_errno());
  $sql = "SELECT * FROM user WHERE username = ?";
  $mysqli = $conn->prepare($sql);
  if(!$conn) die("Query Error To DATABASE In Class Session" . mysqli_errno($conn));
  $mysqli->bind_param("s",$username);
  $mysqli->execute();
  $result = $mysqli->get_result();
  $mysqli->close();
  $conn->close();
   if($result->num_rows > 0)
      {
           $r = $result->fetch_assoc();
          if($r['password'] === $password)
          {
     $this->setSession($r['user_id'],$r);
     return true;
          }else return false;
     }
     else
        return false;
        return false;

}


//Private DataBase Verification of User Credentials
private function verifySessionUser($user_id){
$conn = new mysqli(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_DATABASE);
  if(!$conn) die("Connection Error To DATABASE" . mysqli_errno());
  $sql = "SELECT * FROM user WHERE user_id = ?";
  $mysqli = $conn->prepare($sql);
  if(!$conn) die("Query Error To DATABASE In Class Session" . mysqli_errno());
  $mysqli->bind_param("d",$user_id);
  $mysqli->execute();
  $result = $mysqli->get_result();
  $mysqli->close();
  $conn->close();
   if($result->num_rows > 0)
      {
     return true;
     }
     else
        return false;
  }
}

securepage.php

$user = new Session();
   if($user->getSession() == true)
      {
         header("location: admin.php");
         exit();
       }

Is there any security measures i should consider in future ?

tim · Accepted Answer · 2015-11-28 19:53:06Z

Security

sha1 is not secure enough for password hashing as it's way too fast. This is also noted in the documentation. Use bcrypt instead.
=== is not timing safe, use a timing safe string compare instead (bcrypt does this for you).
You should really use salts when hashing passwords (bcrypt does this for you).
You shouldn't safe the password in the session data.

Misc

Your formatting is all messed up, leading to less readable code. Your indentation is off quite often, your usage of newlines and curly brackets is inconsistent, and you should really always use curly brackets, even for one-line statements (to reduce bugs and increase readability; return false; return false; eg is hard to understand, especially with your indentation).
if (cond) return true; else return false; can be written as return cond.
Binding the session to the ip increases security, but decreases usability (eg for mobile users).
Having three nested ifs instead of one if with & can sometimes reduce readability.
You seem to use == and === interchangeably. In most cases, === would be the correct choice, as it can avoid bugs.
You should only create one db object and pass that around, instead of creating a new one each time.
I'm not sure what verifySessionUser is actually used for. The user didn't set the user id, you did. So it should always be correct, and the check should be unnecessary.
getSession: From the name, I would expect that this returns a session. But it doesn't. isLogedIn seems more fitting. And then I would rename verifyUser to logIn to make it fit.

thank you for your answer , i will try to follow your guidline in next scripts , i m kind of newbie in PHP — Ahtsham Farooq
– Ahtsham Farooq, Commented Nov 28, 2015 at 20:30

Stack Exchange Network

Php Login Class - Security and Logic check

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Php Login Class - Security and Logic check

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions