Advice on if this is a secure way of reading/writing to database

[bytephp]

Hi all. I'm pretty new to PHP and am trying to write a simple CMS using procedual PHP and MySQLi. Thought I'd build a CMS just to get a basic understanding of how things work. I know this would be better done with OOPHP and using PDO for databases interaction, however would appreciate any advice on how what I've done could be improved. The main  areas I'm unsure on is.....

1. Am I using mysqli_real_escape_string(); ok?
2. When updating and deleteing users, is using the GET method ok as it seems a little unsafe when deleting users?
3. Is the sanitization ok?
4. Could I be making more use of functions, say for the session? If so how would you advise.
5. Anything else that need improving?

register.php - this is where I'm unsure on sanitization. Functions page is below this block of code.

<?php

require ('db-connection.php');
require ( 'functions.php' );
$pageTitle = 'Register';

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

	$errors = array(); // puts errors into array

	if ( empty( $_POST['name']) ) {
		$errors['name'] = 'Please enter a name';
	} else {
		$name = sanitize( $_POST['name'] );
		$name = mysqli_real_escape_string( $dbc, $name );
	}
	
	if ( empty ( $_POST['email'] ) ) {
		$errors['email'] = 'Please enter an email address';
	} else {
		$email = sanitize( $_POST['email'] );
		$email = mysqli_real_escape_string( $dbc, $email );
	}

	if ( empty( $_POST['username']) ) {
		$errors['username'] = 'Please enter a username';
	} else {
		$username = sanitize( $_POST['username'] );
		$username = mysqli_real_escape_string( $dbc, $username );
	}

	if ( empty( $_POST['password'] ) ) {
		$errors['password'] = 'Please enter a password'; 
	} elseif ( $_POST['password'] !== $_POST['confirm_password'] ) {
		$errors['password'] = 'Passwords do not match';
	}
	else {
		$salt = generateSalt( $_POST['username'] );
		$password = generateHash( $salt, $_POST['password'] );
	}

	$telephone = sanitize( $_POST['telephone'] );
	$telephone = mysqli_real_escape_string( $dbc, $telephone );

	$postcode = sanitize( $_POST['postcode'] );
	$postcode = mysqli_real_escape_string( $dbc, $postcode );


	if ( empty( $errors ) ) {
		$db_insert = "INSERT INTO users VALUES ( NULL, '$name', '$email', '$username', '$password', '$telephone', '$postcode' )";

		mysqli_query( $dbc, $db_insert ); // performs query on db

		header( 'Location: login.php' );
	}

}

require( 'header.php' );

?>

<h1>Register</h1>
<form action="register.php" method="post" class="form-horizontal">

<?php if ( !empty ( $errors ) ) : ?>
	<div class="alert alert-error">
		<button type="button" class="close" data-dismiss="alert">×</button>
		<p>
		<?php
		foreach ( $errors as $msg ) {
			echo $msg .'<br />';
		}
		?>
		</p>
	</div>
<?php elseif ( empty( $errors ) && isset( $_POST['submitForm'] )  ) : ?>
		<p>Thank you for completing the form.</p>
<?php endif; ?>

	<div class="control-group">
   	<label class="control-label" for="name">Name *</label>
   	<div class="controls">
      	<input type="text" id="name" name="name" placeholder="Email" value="<?php echo isset($_POST['name']) ? $_POST['name'] : ""; ?>">
   	</div>
  	</div>

  	<div class="control-group">
   	<label class="control-label" for="email">Email *</label>
   	<div class="controls">
      	<input type="text" id="email" name="email" placeholder="Email" value="<?php echo isset($_POST['email']) ? $_POST['email'] : ""; ?>">
    	</div>
  	</div>

   <div class="control-group">
   	<label class="control-label" for="username">Username *</label>
   	<div class="controls">
      	<input type="text" id="username" name="username" placeholder="Username" value="<?php echo isset($_POST['username']) ? $_POST['username'] : ""; ?>">
    	</div>
  	</div>

   <div class="control-group">
    	<label class="control-label" for="username">Password *</label>
    	<div class="controls">
      	<input type="password" id="password" name="password" placeholder="Password" value="<?php echo isset($_POST['password']) ? $_POST['password'] : ""; ?>">
    	</div>
  	</div>

   <div class="control-group">
   	<label class="control-label" for="confirm_password">Confirm Password *</label>
   	<div class="controls">
      	<input type="password" id="cofirm_password" name="confirm_password" placeholder="Confirm Password" value="<?php echo isset($_POST['confirm_password']) ? $_POST['confirm_password'] : ""; ?>">
    	</div>
  	</div>

  	<div class="control-group">
   	<label class="control-label" for="telephone">Telephone</label>
   	<div class="controls">
     		<input type="text" id="telephone" name="telephone" placeholder="Telephone" value="<?php echo isset($_POST['telephone']) ? $_POST['telephone'] : ""; ?>">
   	</div>
  	</div>

  	<div class="control-group">
   	<label class="control-label" for="postcode">Postcode</label>
   	<div class="controls">
     		<input type="text" id="postcode" name="postcode" placeholder="Postcode" value="<?php echo isset($_POST['postcode']) ? $_POST['postcode'] : ""; ?>">
   	</div>
  	</div>

	<div class="control-group">
		<div class="controls">
			<button type="submit" class="btn btn-large btn-primary" name="submitForm">Register</button>
		</div>
	</div>
</form>

<?php require( 'footer.php' ); ?>

 

 

functions.php

<?php

function generateSalt( $username ) {
	$salt = '$2a$10$';
	$salt = $salt . md5(strtolower( $username ));
	return $salt;
}

function generateHash( $salt, $password ) {
	$hash = crypt( $password, $salt );
	$hash = substr($hash, 29);
	return $hash;
}

function sanitize( $input ) {
 return htmlspecialchars(trim( $input ));
}

 

view-users.php - when displaying data from the database, do I need to run it through mysqli_real_escape_string(); before outputting to the user? How would that be done, just on the $result variable? Also with the delete user, I'm using the GET method, is this ok? This is opening the page which I've put the code below this block.

 

<?php

session_start();
if ( !isset( $_SESSION['username'] ) ) {
	header ( 'Location: login.php' );
} else {
	// set time-out period (in seconds)
	$inactive = 300;

	if (isset($_SESSION["timeout"])) {
	    // calculate the session's "time to live"
	    $sessionTTL = time() - $_SESSION["timeout"];
	    if ($sessionTTL > $inactive) {
	        session_destroy();
	        header( 'Location: logout.php' );
	    }
	}
	$_SESSION["timeout"] = time();
}

require( 'db-connection.php' );
require( 'functions.php' );

// $query = "SELECT * FROM users";
$query = "SELECT id, name, email, username, telephone, postcode FROM users";
$result = mysqli_query( $dbc, $query );

require( 'header.php' );

?>
<p><a href="logout.php">Logout</a></p>
<table class="table table-striped">
	<tr>
		<th>ID</th>
		<th>Name</th>
		<th>Email</th>
		<th>Username</th>
		<th>Telephone</th>
		<th>Postcode</th>
		<th>Edit User</th>
	</tr>
	<?php while ( $row = mysqli_fetch_array( $result, MYSQLI_ASSOC ) ) : ?>
	<?php $username = $row['username']; ?>
	<tr>
		<td><?php echo $row['id']; ?></td>
		<td><?php echo $row['name']; ?></td>
		<td><?php echo $row['email']; ?></td>
		<td><?php echo $row['username']; ?></td>
		<td><?php echo $row['telephone']; ?></td>
		<td><?php echo $row['postcode']; ?></td>>
		<td>
			<div class="btn-group">
			   <a class="btn" href="edit-user.php?username=<?php echo $username; ?>"><i class="icon icon-edit"></i></a>
			   <a class="btn" href="delete-user.php?username=<?php echo $username; ?>"><i class="icon icon-trash"></i></a>
			</div>
		</td>
	</tr>
	<?php endwhile; ?>
</table>
<p><?php printf("Select returned %d rows.\n", mysqli_num_rows($result)); ?></p>

<?php require( 'footer.php' ); ?>

 

delete-user.php

 

<?php
session_start();
if ( !isset( $_SESSION['username'] ) ) {
	header ( 'Location: login.php' );
} 

require( 'db-connection.php' );
require( 'functions.php' );

if ( isset( $_GET['username'] ) ) {
	$username = $_GET['username'];

	$query = "DELETE FROM users WHERE username = '$username'";
	$result = mysqli_query( $dbc, $query );

	header( 'Location: view-users.php' );
}

mysql_close( $dbc );

Thanks in advance.

[davidannis]

Haven't had a chance to look at all of the code but why:

function generateHash( $salt, $password ) {
	$hash = crypt( $password, $salt );
	$hash = substr($hash, 29);
	return $hash;

in generate hash not use the whole result? Disk space is cheap and an extra 100 bytes makes things more secure.

[ignace]

davidannis variables are not stored on disk, they are stored in memory and the extra bytes do not make it more or less secure.

[davidannis]

Ignace,

 

Not to be argumentative with a moderator but:

 

variables are not stored on disk, they are stored in memory

that is true until the OP stores the variable we're talking about, $password, in a database here:

		$db_insert = "INSERT INTO users VALUES ( NULL, '$name', '$email', '$username', '$password', '$telephone', '$postcode' )";

		mysqli_query( $dbc, $db_insert ); // performs query on db

 

and the extra bytes do not make it more or less secure.

 

If I follow your argument to a logical extreme, after hashing my password I could save lots of space by just storing the first byte. Of course, in a brute force attack, the attacker would get something that hashed to the same first byte in half the time that if I took to get something that would be the same as if I stored two bytes...

[ignace]

Not to be argumentative with a moderator

My rank on this forum should not be of any concern, speak freely

that is true until the OP stores the variable we're talking about, $password, in a database here

Well I assumed you were talking about the snippet in your post.

If I follow your argument to a logical extreme, after hashing my password I could save lots of space by just storing the first byte. Of course, in a brute force attack, the attacker would get something that hashed to the same first byte in half the time that if I took to get something that would be the same as if I stored two bytes...

I misread, and I agree he should use the entire hash not shop it off at 29 characters.

[bytephp]

Thanks for the advise guys. The reason for doing this is to remove the first 29 characters (the salt) from the hash and return what is left, which is the actual password that will be stored in the database. The salt is used and generated in real time based on the username and password. More details on how and why this is done can be found here.

 

Anyone able to provide feedback on other areas please? My main concern is how I'm deleting users and if I can make the code can be refactored by storing more bits in a function.

[ignace]

* You should put an exit(); after every header('Location: ..');

* You should use mysqli everywhere and don't mix them.

* I don't think it's okay for anyone being logged in to just delete any account (even his own).

* Use prepared statements.

[davidannis]

I've done a little research and suggest that you be very careful about using your own salt instead of the blowfish salt and removing the first n characters of the hash. Here's why I think so:

The blowfish generated salt is meant to be random, so that it is unlikely that two entries in the database are hashed with the same salt. I do not believe that an MD5 hash will be any more random than the one generated by blowfish.

More importantly, removing the first characters of the salt also removes some information that you may want later. Here's why

A description of what the start of the hash is

 

$2a$10$vI8aWBnW3fID.ZQ4/zo1G.q1lRps.9cGLcZEiGDMVr5yUP1KUOYTa

• 2a identifies the bcrypt algorithm version that was used.
• 10 is the cost factor; 2¹⁰ iterations of the key derivation function are used (which is not enough, by the way. I'd recommend a cost of 12 or more.)
• vI8aWBnW3fID.ZQ4/zo1G.q1lRps.9cGLcZEiGDMVr5yUP1KUOYTa is the salt and the cipher text, concatenated and encoded in a modified Base-64. The first 22 characters decode to a 16-byte value for the salt. The remaining characters are cipher text to be compared for authentication.
• $ are used as delimiters for the header section of the hash.

Now if you remove this you make it harder for an attacker to try to get the salt back, but you also lose the algorithm version and the cost that the password was encoded with. Now, if the recommended cost (the number of iterations that the hash goes through) changes you are stuck. You can't do new passwords with the new cost and gradually migrate your users as they change passwords. If you upgrade php and the default cost changes you may suddenly find your code no longer works for stored passwords.

 

Similarly, you can't change the algorithm (in fact the $2a$ in my example from stack overflow is deprecated to fix a security issue). If you have 1,000 passwords using $2a$ and add one using the new $2y$ you don't know which method to use.

 

In your shoes, I'd choose the most secure algorithm the community has come up with (which you have) and use it as is, not try to improve it since I recognize that I would need a lot of expertise that I don't have to be sure that I'm not messing things up that will bite me later.

[davidannis]

I realize that my post above would gain a lot from a paragraph on why "giving away the salt" (which actually isn't what you're doing by leaving the first 29 characters) isn't that big a deal but I have hungry kids and a dinner that will take too long to cook. I'll come back with an extended comment later.

[davidannis]

Kids fed, bathed, and sent off to bed, so now I have a chance to explain why I don't think giving away the salt matters. This is my understanding of how it works.

 

When you hash a password, you are not encrypting it - you are calculating a value based on that password which is unlikely to be duplicated by performing the same calculations on a different password. You can, however, have another password that produces the same value (that's called a collision). Since more than one value can produce the same result it is not possible to reverse or decrypt the passwords. The issue is not hackers decrypting the password it is that they try brute force attacks, trying lots of common or short combinations to see if they can get something that produces the same hashed value.

 

Let's take an example of a file which falls into a hacker's hands that has a username and hashed password under 3 scenarios:

 

If Joe Hacker gets an unsalted password file he can look at the values in that file and find common ones (let's say .3% of 10,000 users use "password" as their password). Joe has databases with dictionaries hashed using all the common algorithms. He sees 30 hashes are !@#$%^&* and finds in his database that sha1 produces that as it's hash for the password "password". He realizes he has an unsalted sha1 hashed file. Sally used "superior" as her password, which was unique among the 10,000 users but Joe sees that hashed to "123456789" and looking in his table of sha1 hashes he sees that the password superior will hash to that value. By using an unsalted table it made it easy for Joe.

 

In scenario 2 Joe gets the file but every password was salted with the same set of characters. Joe sees 30 values that are identical but since that value is not in his common hash values database he needs to figure out the salt and rerun the dictionary. A daunting task. Nonetheless, he knows something about commonly used passwords and tries a few hundred of them against the thirty accounts with the common hash value. Once one lets him in he knows all 30 use the password "password". He can then run password with random salts through the algorithm until he figures out the salt. Then use the salt that he figured out to run the entire dictionary and Sally (with much more work) is still compromised.

 

In scenario 3 Joe gets the same file, hashed with a different salt for each user. He also gets a note saying that the salt is the username. He would have to run the dictionary against each account with a different salt. It would be time intensive and breaking one password does not help him break the next. Someone who used a secure password (long, not in a dictionary) is unlikely to have his account compromised by Joe.

 

Hash algorithms rely on the fact that it takes a long time to run lots of possible passwords through them. Using a unique salt means that hackers can't take the shortcut of running the dictionary once against all accounts in the database. If the hacker knows the salt for a particular account though, he can crack that account no faster than he could without knowing the salt, unless he only needs to try weak passwords because he still needs to try the same number of combinations of random characters to find one that gives him the right hash value.