Do I need to add a token when generating Session?

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
phpwalker
Forum Commoner
Posts: 81
Joined: Sun Apr 23, 2006 12:18 pm

Do I need to add a token when generating Session?

Post by phpwalker »

I'm creating a login page and protected pages.

login.php

Code: Select all

<?php 
require_once "config.inc.php"; // include the database information 

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

$clean = array();
$mysql = array();

$clean['user'] = strip_tags($_POST['username']);
$clean['pw'] = $_POST['password'];

$salt = 'alenn';
$password_hash = md5($salt . md5($clean['pw'] . $salt));

$mysql['user'] = mysql_real_escape_string($clean['user']);


// formulate and execute the query
$cmd  = "SELECT * FROM inb_contact WHERE name='{$mysql['user']}' AND password='$password_hash'";

$sql = mysql_query($cmd) or die("username was incorrect. MySQL said".mysql_error()); // this checks to see if the username exists 

$result = mysql_fetch_array($sql); // puts the database information into an array 

// Present results based on validity.
	if (mysql_num_rows($sql) == 1) {
             
				session_start(); 
				$_SESSION['sessioname'] = $mysql['user']; 
				header("location: protected.php"); 

	} else {
				echo "You are not authorized!";
	}

}else{
	echo "Na Tink.";
}

?>

protected.php

Code: Select all

<?php

$authorize = 0;
session_start();

if(isset($_SESSION['sessioname'])){ 
                $authorize = 1;
}else{ 
// Your protected stuff goes here if you wish to echo the username echo $_SESSION[”sessioname”] 
	echo "YOU MUST BE LOGGED IN TO SEE THIS!"; 

if($authorize == 1){
	echo "welcome ".$_SESSION['sessioname']; 
                //show content here
}
}
In the protected.php, I've added $authorize variable to check if the user is the intended one. Is this neccessary or I've overdid something?

Can I use session to stored the $authorize value -> $_SESSION['authorize']; So that it will become...

login.php

Code: Select all

<?php 

//Is this suitable to start session here?
session_start(); 
$_SESSION['authorize'] = 0;

require_once "config.inc.php"; // include the database information 

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

$clean = array();
$mysql = array();

$clean['user'] = strip_tags($_POST['username']);
$clean['pw'] = $_POST['password'];

$salt = 'alenn';
$password_hash = md5($salt . md5($clean['pw'] . $salt));

$mysql['user'] = mysql_real_escape_string($clean['user']);


// formulate and execute the query
$cmd  = "SELECT * FROM inb_contact WHERE name='{$mysql['user']}' AND password='$password_hash'";

$sql = mysql_query($cmd) or die("username was incorrect. MySQL said".mysql_error()); // this checks to see if the username exists 

$result = mysql_fetch_array($sql); // puts the database information into an array 

// Present results based on validity.
	if (mysql_num_rows($sql) == 1) {
             
                                                                $_SESSION['authorize'] = 1; //set it to be true if successfully logged in
				$_SESSION['sessioname'] = $mysql['user']; 
				header("location: protected.php"); 

	} else {
				echo "You are not authorized!";
	}

}else{
	echo "Na Tink.";
}

?>
protected.php

Code: Select all

session_start();

if(isset($_SESSION['sessioname'])){ 
     if($_SESSION['authorize'] == 1){
	echo "welcome ".$_SESSION['sessioname']; 
                //show content here
      }
      else{
                echo "unauthorize user";
       }
}else{ 
	echo "YOU MUST BE LOGGED IN TO SEE THIS!"; 


}
Should I regenerate the session ID to make that more secure?

Thanks in advance for the helps..
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Should I regenerate the session ID to make that more secure?
Yes, every time a user's authorization status changes.

For your other question - yes it seems redundant, if there is data in $_SESSION which is only set when the user has authenticated successfully. I would add two things to this though - session timeout checks and forcing logout on wrong credentials. Optionally, you may implement IP checks on the session owner, but let the users choose if this protection should be active, some of them do not control the IPs of their requests.

Edit:

Code: Select all

$clean['user'] = htmlentities($_POST['username'], ENT_QUOTES, your_encoding_here); //although in your case you may strip_tags first if you so badly want it
...
$_SESSION['sessioname'] = $clean['user']; //not $mysql['user']
It also seems that ti should be called $html, not $clean.

header("location: protected.php"); should use a full URL (http://example.com/protected.php)
Post Reply