How Secure Is This Login System? (Using Cookies In PHP)

Question

I've created a login page and I'm using Cookies. Down below is the code for Login Page:

Register

<?php
session_start();
if(isset($_POST['submitted'])){
$errors = array();
$mysqli = new mysqli('localhost', 'db123', 'db123', 'db123');
$username = $_POST['username'];
$result = $mysqli->query("SELECT * FROM registered_users WHERE phone_number = '$username'");
$data = mysqli_fetch_assoc($result);
if($result->num_rows == 0) {
     echo 'Username Not Found!';
} elseif($data['otp_verified'] === 'false'){
echo 'OTP Not Verified, Click Here To Verify Your Number';
}
else{
    $encryptpass=md5($_POST['password']);
    $cookie_username = $_POST['username'];
    if($encryptpass == $data['password']){
    echo 'Login Is Verified';
    $Month = 86400 + time(); 
     setcookie('user', $cookie_username, $Month);
    header("location:dashboard.php");
    }
    else{
    echo 'Login/Password Incorrect :(';
    }
}
$mysqli->close();   
}

?>

And Finally, Here's the code for dashboard.php and all other pages which are restricted:

<?php
session_start();

if(!isset($_COOKIE['user']))
{
header("location:index.php");
die();
}
?>

My Questions: 1. How Secure Is This Login System? 2. How I can improve it? Thanks in advance :)

It's not very secure. Use `password_hash()` instead of `md5()`. Use prepared statement. Do not use cookies, use sessions. — Qirel, Apr 30 '19 at 08:12
just like the statement above, after doing those things, you could probably use oath instead and save you the trouble doing it right — Kevin, Apr 30 '19 at 08:16
Okay, But session is destroyed when user close the browser. I want users to stay logged in for specific interval of time :/ — Paul, Apr 30 '19 at 08:16
You can implement a remember me feature, Paul. But be careful of that, it can introduce security holes - and has to be done properly. But always, always use sessions for this, not cookies. — Qirel, Apr 30 '19 at 08:18
Sessions can survice browser closure if you specify their [lifetime](https://www.php.net/manual/en/function.session-set-cookie-params.php). — KIKO Software, Apr 30 '19 at 08:18
Hi @KIKO Software can you tell me about the problems with the code? Thanks :) — Paul, Apr 30 '19 at 08:19
@KIKOSoftware I'd say it's off-topic (hence my downvote). I feel like this question would be indeed better for code review. — treyBake, Apr 30 '19 at 08:20
Please read the comment by Qirel, he addresses the major problems. — KIKO Software, Apr 30 '19 at 08:20
@treyBake That could indeed be a valid reason to downvote, but Paul is not to know, unless you tell him.... which you just did... — KIKO Software, Apr 30 '19 at 08:22

KIKO Software · Accepted Answer · 2019-04-30T08:59:00.193

Here's a non-exhaustive list of problems/solutions:

Your code is difficult to read because it is not properly indented.
You should use prepared statemens to guard against SQL-injection.
You give hints to hackers by having different error messages. When the username is correct and the password wrong you say: "Login/Password Incorrect :(", but if the username is wrong you say: "Username Not Found!". That way a hacker can know if an username is correct, and half the job is done.
Better not use md5() for password encryption.
Use password_hash() for handling passwords.
Do not store the username in a cookie. Again, you're leaking information.
Don't use cookies, there's just no need to do that, use sessions and store information on the server, not on the user's machine.
You seem to have stored usernames as phone_number. So which one is it? It is either an username or a phone number, it cannot be both. Even if you use phone numbers as user names, call them what they are.
Sloppy coding: $errors = array(); is not used anywhere. You don't check the result of new mysqli(), the connection might fail. Same is true for $mysqli->query().
You take care to close the database, but then why don't you release the query result with $result->close();? Either do both, or none.

Security is a difficult topic, it's really hard to get it right, and what might be good today, might be bad tomorrow.

score 0 · Answer 2 · answered Apr 30 '19 at 08:22

0

Its very unsecure

there can be sql injections (because the username goes directly to the databasa)
md5 is obsolete since years
you save the username unencrypted

answered Apr 30 '19 at 08:22

TheBlueOne

486
5
13

As you daid: (there can be sql injections (because the username goes directly to the databasa), What are the best practices to resolve this issue? – Paul Apr 30 '19 at 08:23
Thanks for your time and efforts :) – Paul Apr 30 '19 at 08:29
you can use a libary like [meekro db](https://meekro.com/) or use `private static function SecureParam($param) { return stripslashes(addslashes($param)); }` to avoid sql injection – TheBlueOne Apr 30 '19 at 08:29
1

The most definitive and ONLY proper way of preventing SQL injection, is by using a prepared statement. Don't think you can strip and filter the input, you'll end up mangling the data more than you should - just use a prepared statement. Its simpler and better. – Qirel Apr 30 '19 at 08:32
And encrypting the username will be a massive pain to work around. It's not needed, it can be in cleartext. The password on the other hand; use `password_hash()` instead of `md5()`. – Qirel Apr 30 '19 at 08:34
So, I need to use Prepared Statement(https://www.youtube.com/watch?v=I4JYwRIjX6c) and password_hash to make it secure? – Paul Apr 30 '19 at 08:39
Use prepared statement with placeholders (https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php), use `password_hash()` when registering, `password_verify()` when logging in, use sessions instead of cookies. That's a good start :-) – Qirel Apr 30 '19 at 08:40

How Secure Is This Login System? (Using Cookies In PHP)

2 Answers2