1
\$\begingroup\$

I have php cookie based login system on my website using below given code. I want to know that whether this login system is secure or not, Whether it can be hacked in any way?

<?php
session_start();
if(isset($_SESSION["userid"]))
{
 header("location:index.php");
}
include( $_SERVER['DOCUMENT_ROOT'] . '/conndb.php' );

if(isset($_POST["login"]))   
{  
 if(!empty($_POST["member_name"]) && !empty($_POST["member_password"]))
 {
  $name = mysqli_real_escape_string($conn, $_POST["member_name"]);
  $password = mysqli_real_escape_string($conn, $_POST["member_password"]);
$stmt_check = $conn->prepare("SELECT * FROM userz WHERE email =? AND pass = ? AND active ='1'");
$stmt_check->bind_param("ss", $name, $password);
$stmt_check->execute();
$stmt_check->store_result();
$numberofrows = $stmt_check->num_rows;

if(($numberofrows) > 0){

    setcookie ("member_login",$name,time()+ (10 * 365 * 24 * 60 * 60));  
    setcookie ("member_password",$password,time()+ (10 * 365 * 24 * 60 * 60));
    $_SESSION["userid"] = $name;
   header("location:index.php"); 

} else {

   $message = "Invalid Login OR Email not Verified";  
}

$stmt_check->close();

 }
 else
 {
  $message = "Both are Required Fields";
 }
}  
?>

Besides this code, below piece of code is placed on each page to check whether the user is already logged in or not.

 <?php
    session_start(); 
    if (empty($_SESSION['userid'])) {
        header('Location: users/login.php');
        exit;
    }
    ?>
\$\endgroup\$
  • 2
    \$\begingroup\$ It is not considered 'secure' to store plain passwords in a database, but in some cases I could live with that. More problematic is that you're not protecting yourself against a brute force attack, which is the most common attack type. en.wikipedia.org/wiki/Brute-force_attack I could also copy a cookie of any user that has logged in, and I would be in as well. This is called session hijacking: en.wikipedia.org/wiki/Session_hijacking Your system would be 'good enough' for a very low grade of security, where it's not really a problem if it got hacked. \$\endgroup\$ – KIKO Software Feb 2 '18 at 21:11
  • \$\begingroup\$ @KIKOSoftware, can you suggest me some safe and advanced login script available to download.? \$\endgroup\$ – John Feb 3 '18 at 2:32
  • \$\begingroup\$ Most PHP frameworks have a good login system. For instance: laravel.com/docs/5.5/authentication or docs.zendframework.com/zend-authentication/intro If you want to write your own system you will first have to learn what a safe and advanced login script has to cope with. Never forget that the security should only need to be as good, as the sensitivity of the data it protects. Basic login systems, like yours, will work fine for simple cases where you only want to filter registered users from non-registered users, and no real data protection is required. \$\endgroup\$ – KIKO Software Feb 3 '18 at 10:06
1
\$\begingroup\$

As the comments have said, writing a login systems securely is really hard. I would strongly recommend using an existing framework like one of the ones KIKO Software has put in his comment. With that disclaimer out of the way, let's have a look at the code.


<?php
session_start();
if(isset($_SESSION["userid"]))
{
 header("location:index.php");
}

Any time you use a redirect, it should be immediately followed by an exit(). If you don't do this the rest of the code will still be executed, and if the user's browser ignores the Location header they can still view the rest of the page. Not so bad here, but can lead to nasty access control issues. HTTP headers are normally capitalised with a space after the colon like Location: /index.php but I don't think there any (common) browser that would complain about this.

if(isset($_POST["login"]))   
{  
 if(!empty($_POST["member_name"]) && !empty($_POST["member_password"]))

A user could submit an array here for the username or password, rather than a string, which you probably don't want, as some PHP functions do strange things with arrays. If you only want a string, you should check for it here.

 {
  $name = mysqli_real_escape_string($conn, $_POST["member_name"]);
  $password = mysqli_real_escape_string($conn, $_POST["member_password"]);

The mysql_real_escape_string() function has some issues with certain character sets, so you need to be careful with it. But since you're using prepared statements, you don't need to escape these, and in fact if you escape them and they have certain characters like single quotes, you'll end up with the wrong values.

$stmt_check = $conn->prepare("SELECT * FROM userz WHERE email =? AND pass = ? AND active ='1'");

Is active a string? I'd expect that to be an integer (or even boolean), but you have single quotes around the 1, so it's being treated as a string by MySQL.

Doing SELECT * is normally a bad sign - you should only select the columns you actually need (which doesn't look like any in this case, you could probably get away with a count).

As mentioned in the comments, storing plaintext passwords is a really bad idea. You should be using a secure hashing function like bcrypt/scrypt/PBKDF2 to store passwords.

$stmt_check->bind_param("ss", $name, $password);
$stmt_check->execute();
$stmt_check->store_result();
$numberofrows = $stmt_check->num_rows;
if(($numberofrows) > 0){

Could the number of rows returned ever by more than 1? That would mean that you have duplicate users in your database, which would be a sign something has gone badly wrong. Unless there's something very strange in the database, it'd be better to check that exactly 1 row has been returned.

setcookie ("member_login",$name,time()+ (10 * 365 * 24 * 60 * 60));  
setcookie ("member_password",$password,time()+ (10 * 365 * 24 * 60 * 60));

Storing the plaintext password (or any password) in a cookie is a very bad idea, especially when it's going to be stored for a ten years. Any cookies you're creating should also be marked as HttpOnly, Secure (assuming you're using SSL, which you really should be), and optionally SameSite.

$_SESSION["userid"] = $name;

It's good to be consistent internally with your variable names. So far the same information has been called member_name, name, email and userid

header("location:index.php");

See comments on previous header() usage.

} else {
  $message = "Invalid Login OR Email not Verified";  
}

$stmt_check->close();

This could have been closed earlier, immediately after you get the number of rows.


As KIKO Software mentioned in his first comment, there are also some other security features that are lacking:

  • Account lockout
  • CAPTCHA or other brute-force protection
  • Session hijacking

Writing authentication code is really, really difficult, and if you solve all of these issues, there are still all kinds of other complexities like timing attacks, user enumeration, etc. You're much better off letting someone else deal with that pain.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.