1
\$\begingroup\$

I create login form with session with four files:

  • connection.php
  • login.php
  • plan.php
  • logout.php

Please check my code and tell about any smokestack.

Database educate (create table users)

CREATE TABLE users(
id INT AUTO_INCREMENT,
username VARCHAR(50) NOT NULL,
password VARCHAR(100) NOT NULL,
PRIMARY KEY(id) );

Connection.php

<?php
$dsn = 'mysql:host=localhost;dbname=educate'; //Data source Name
$username = 'root';
$password = '';
$options = array(PDO::MYSQL_ATTR_INIT_COMMAND=> 'SET NAMES utf8');

 $conn = new PDO($dsn, $username, $password,$options);
 ?>

login.php

<?php
session_start();
if(!empty($_SESSION['username'])) {
header('location:plan.php');
}
require 'connection.php';


if(isset($_POST['login'])) {

$user = $_POST['username'];
$pass = $_POST['password'];

if(empty($user) || empty($pass)) {
$message = 'All field are required';
} else {
$query = $conn->prepare("SELECT username, password FROM users WHERE 
username=? AND password=? ");
$query->execute(array($user,$pass));
$row = $query->fetch(PDO::FETCH_BOTH);

if($query->rowCount() > 0) {
  $_SESSION['username'] = $user;
  header('location:plan.php');
} else {
  $message = "Username/Password is wrong";
}


}

}
?>

<!DOCTYPE html>
<html>
<head>
</head>
<body>
<?php
if(isset($message)) {
echo $message;
}
?>
<form action="#" method="post">
Username: <input type="text" name="username" placeholder="username"> 
 <br/><br/>
Password: <input type="password" name="password" placeholder="password">

<br/><br/>
<input type="submit" name="login" value="Login">

</form>

</html>

logout.php

<?php
session_start();
session_destroy();
header('location: login.php');
?>

plan.php

<?php
session_start();

if(isset($_SESSION['username'])) {
echo "Welcome <strong>".$_SESSION['username']."</strong><br/>";
} else {
header('location: login.php');
}

?>
<a href="logout.php">Logout</a>
Improve this question
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This is really similar to your last question (codereview.stackexchange.com/questions/133582/…). You got great solutions but you never selected an answer. \$\endgroup\$
    – BeetleJuice
    Jul 4, 2016 at 2:22
  • \$\begingroup\$ If anyone uses this code, then his/her software can be easily hacked. The user can also access any page after logout by just type page URL. \$\endgroup\$
    – Mukesh Patel
    Dec 21, 2018 at 17:11

1 Answer 1

Reset to default
3
\$\begingroup\$

Always authenticate the user before you take any restricted action.

In login.php you have:

session_start();
if(!empty($_SESSION['username'])) {
header('location:plan.php');
}

This code has 2 vulnerabilities I can think of:

  1. It assumes that the existence of username in the session is proof of authenticity. If your user doesn't log out, anyone who uses his browser much later to come to your site will be admitted; likewise anyone who logs HTTP requests and later looks at the log will find the session cookie and can impersonate the user. To fix, store a valid_until date in the session when the user logs in. Then when you check for credentials, you only admit the user if the login validity has not expired.

  2. Your site is vulnerable to Cross-Site Request Forgery (CSRF). Suppose your logged-in user visits my website where I have something like:

.

<--! Make user send request to your site without knowing -->
<img src="http://yoursite.com/plan.php?do=evil">

Because browsers send cookies automatically with every request, the user's session cookie will be sent to plan.php when the browser tries to load the image, and your script will treat the request as legitimate, therefore it will act on ?do=evil. To fix, set a token in your response headers when the user logs in, and expect the same token back from the request headers from legitimate requests.

The two points above lead to a modification of login.php. When the user logs in, you now do something like:

$_SESSION['username'] = $user;
$_SESSION['valid_until'] = time() + 60*60; //1hr from now
$_SESSION['csrf_token'] = 'long_random_string'; //research how to build this
header("X-CSRF: $_SESSION[csrf_token]");
header('location:plan.php');

When your user comes to my evil site and I try to connect to plan.php, the browser will still send the session cookie but it will not send the CSRF token, so before you grant access you would check for it:

session_start();
$logged_in = false;
if(isset($_SESSION['username'])){
    if(!$_SESSION['valid_until'] >= time()) $logged_in = false;
    $request_headers = apache_request_headers();
    $csrf = isset($request_headers["X-CSRF"])? $request_headers["X-CSRF"] : null;
    if($_SESSION['csrf_token']!==$csrf) $logged_in = false;
}
if($logged_in){/*give access*/}
else{/*block access*/}

By the way, this logic should be moved to a function that returns TRUE if the user is logged in, false otherwise. You can extend it with further checks. For instance you could record the IP address to which the session was granted, and if it doesn't match the IP of the request, deny access (to protect against an attacker stealing the session and CSRF token and trying to re-use it from another location).

In login.php you have:

$user = $_POST['username'];
$pass = $_POST['password'];
...
$query = $conn->prepare("SELECT username, password FROM users WHERE 
username=? AND password=? ");
$query->execute(array($user,$pass));

It's great that you use prepared statements to prevent SQL Injection, but this code is evidence that you're storing the passwords in clear text, without hashing. This means that if your database gets stolen, the thief will know your users' credentials, and can use them to impersonate the users on other services. It also means that anyone with access to the database can do the same thing. Instead, when a user registers, hash the password before saving it.

$password = password_hash($_POST['password'],PASSWORD_DEFAULT);

Then when a user logs in, you verify the submitted password against the hashed password. Do something like:

$query = $conn->prepare("SELECT username, password FROM users WHERE username=?");
... // <-execute query and capture password in $db_pass

if($query->rowCount() > 0 && password_verify($_POST['password'],$db_pass)){
    //credentials are correct

}else{/*user unknown or password incorrect*/}
Improve this answer
\$\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.