SQL injections - how to stop

[moosey_man1988]

Hi guys

 

I've been working on a project for a while now, after getting everything functional so far, i decided its time to start locking the system down, and my first task is SQL injection prevention

I really am having a hard time understand the whole sql injection thing, im pretty sure it where someone can manipulate the query so they can steal information from the Database?

 

I followed a guide but i want to be sure for my first query, Is this SQL injection proof?

<?php

//This section is for adding a new user

if (isset($_POST['NewUserSubmit'])){
//lets get the POST information

$firstname = $_POST['FName'];
$LastName = $_POST['LName'];
$email = $_POST['EmailAddress'];
$password = $_POST['Password'];
$ContactNo = $_POST['ContactNumber'];
$userLevel = $_POST['userLevel'];
$userName = $firstname.".".$LastName;

//change the password to MD5

$password = md5($password);


// okay lets see if the user exists

$selectUserName = "SELECT * FROM MC_users WHERE username='".$userName."'";

$selectResult = $pdo->prepare($selectUserName);

$selectResult->execute();

//is this username already exists tell them!
if ($selectResult->rowCount() > 0){

echo "<div class='alert alert-danger fade in'><strong> This user Already Exists please try a different user First and Last Name</strong><button class='close' data-dismiss='alert' aria-label='dismiss'</button>dismiss</div>";
	
} else {//else create the user

// This worked so lets place the variables into the pdo query using an array

$insertUserQuery = "INSERT INTO MC_users (firstname,lastname,username,secret,userLevel,email,contact) VALUES (:firstname,:LastName,:userName,:password,:userLevel,:email,:ContactNo)";

$Result = $pdo->prepare($insertUserQuery);

if ($Result->execute(array(':firstname'=>$firstname,
					   ':LastName'=>$LastName,
					   ':userName'=>$userName,
					   ':password'=>$password,
					   ':userLevel'=>$userLevel,
					   ':email'=>$email,
					   ':ContactNo'=>$ContactNo  
					   ))){
echo "<div class='alert alert-success fade in'><strong>User ".$userName." has been created</strong><button class='close' data-dismiss='alert' aria-label='dismiss'</button>dismiss</div>";						   
					   }

					   
}

}

?>

any help is greatly appreciated And i thank everyone in advance for your help.

 

Mooseh Man

[maxxd]

The insert statement is good - you're using a prepared statement. Do the same with the select statement. Also, don't use md5() for your password - MD5 has been pretty much obsolete for quite a while now due to the fact that it's not secure. Use password_hash() and password_verify().

 

Those are a couple things that jumped out at me on a quick look.

[Ch0cu3r]

No, your first query is not secure. You should be binding the $userName value to a placeholder in your select query, just like you did with your your second query.

 

You should not be using md5 to hash passwords, instead use password_hash

 

Also why are you using the users first and last names as their username? Not everyone in the word has unique names, alot of people that are not related share the same names. Why not just let the user pick there own username?

[Jacques1]

Besides the obvious security vulnerabilities already mentioned, your uniqueness check doesn't work in a highly parallel environment like a web application. If two processes request an unused name at the same time, your SELECT query tells both of them that they can have the name. In the worst case, you end up with a duplicate name despite your check.

 

Only the database system can enforce unique values: Set up a UNIQUE constraint on the username column and simply try to insert the row. If that fails due to a constraint violation, you know the name is already taken. See this example implementation.

 

Since you specially asked about security-related problems, I strongly recommend you learn the basics before you jump to the code. Security is not a trial-and-error business.

[Muddy_Funster]

There is also absolutely no reason to be using SELECT * for this, all you need is to select the count of results that come back matching the criteria.  

 

[Rule of thumb] : Never expose more information than you need to.

[moosey_man1988]

Firstly I'd like to say thank you for all your comments, I know there is a lot to be improved in this code I'll try break this down a bit

 

Jacques:-  The database has a unique key on the username, so if it fails it will tell you due to the database query failure. I've done pretty much exactly what you just said Jacques thankfully my sql understanding is better than my php,

 

Ch0cu3r / maxxd: - Thank you for the comment on the first bit, I am facepalming as to why i didnt bind the username to a placeholder aswell...

 

Muddy :- For md5 i'll explain below, . Also This "Needs" to have a unique username in order for me to do that I must select whats already there surely not, if not show me some magic .

 

Just to explain I am quite newbie to php, In order for me to provide "secure" code, I must understand it before hence why this is a dev project sitting in a little box unknown to the world lol .

 

The functionality of the query itself is not a problem, I understand MD5 is pretty much obsolete, Im looking to swap this login at some point to "salt?".

 

But one thing at a time, Learning in my books is done in small steps, once each step is complete, I hope to end up with completely Accurate code from beginning to end of a script

 

I just want to say again thank you to everyone for your quick responses and accurate answers, I love this forum and its community which has provided me with so much help from day one.

Edited November 23, 2015 by moosey_man1988

[Psycho]

Also, by default, PDO only emulates prepared statements. You should ensure you are turning off emulation in your connection to the DB. See this post by Jacques1 for an example:

 

http://forums.phpfreaks.com/topic/298661-help-needed-with-signuplogin-page/?do=findComment&comment=1523598

[moosey_man1988]

Hi Psycho yep don't worry that's already been done in my connection php file

 

okay because of the security issue with MD5 I've re written a couple of pages how does this look?

<?php

//This section is for adding a new user

if (isset($_POST['NewUserSubmit'])){
//lets get the POST information

$firstname = $_POST['FName'];
$LastName = $_POST['LName'];
$email = $_POST['EmailAddress'];
$password = $_POST['Password'];
$ContactNo = $_POST['ContactNumber'];
$userLevel = $_POST['userLevel'];
$userName = $firstname.".".$LastName;

//change the password to MD5

$password = password_hash($password,PASSWORD_DEFAULT);


// okay lets see if the user exists

$selectUserName = "SELECT * FROM MC_users WHERE username=:userName";

$selectResult = $pdo->prepare($selectUserName);
$selectResult->bindParam(':userName',$userName);
$selectResult->execute();

//is this username already exists tell them!
if ($selectResult->rowCount() > 0){

echo "<div class='alert alert-danger fade in'><strong> This user Already Exists please try a different user First and Last Name</strong><button class='close' data-dismiss='alert' aria-label='dismiss'</button>dismiss</div>";
	
} else {//else create the user

// This worked so lets place the variables into the database/mysqli_connection

$insertUserQuery = "INSERT INTO MC_users (firstname,lastname,username,secret,userLevel,email,contact) VALUES (:firstname,:LastName,:userName,:password,:userLevel,:email,:ContactNo)";

$Result = $pdo->prepare($insertUserQuery);

if ($Result->execute(array(':firstname'=>$firstname,
					   ':LastName'=>$LastName,
					   ':userName'=>$userName,
					   ':password'=>$password,
					   ':userLevel'=>$userLevel,
					   ':email'=>$email,
					   ':ContactNo'=>$ContactNo  
					   ))){
echo "<div class='alert alert-success fade in'><strong>User ".$userName." has been created</strong><button class='close' data-dismiss='alert' aria-label='dismiss'</button>dismiss</div>";						   
					   }

					   
}

}

?>

And the new login page to support this, it is all functional But I'd prefer to know its secure

<?php 

require ('../Functions/functions.php');
require('../database/pdo_connection.php');

if (isset($_POST['Login'])) {
//Set session timeout to 2 weeks
session_set_cookie_params(1*7*24*60*60);

session_start();
$error=''; // Currently A Blank Error Message

$username=$_POST['userName'];

//Grab the hash
$selectPasswordHash = "SELECT username,secret FROM MC_users WHERE email=email=:username OR username=:username";
$hashresult= $pdo->prepare($selectPasswordHash);
$hashresult->bindParam(':username', $username);
$hashresult->execute();

$row = $hashresult->fetch(PDO::FETCH_ASSOC);

$hash = $row['secret'];
//got the hash

//lets verify it
if (password_verify($_POST['password'],$hash) === true){
//login correct	
$login_user = $row['username'];
//Set the Session
$_SESSION['login_user']=$login_user;
// Redirect to dashboard.php
header ("Location: ../dashboard.php"); 

} else {

$error = 'Username or Password in invalid';
$error2 = 'Try Logging in with your email instead';

}

}
//Lastly if the user is logged in, point them back to the Dashboard
if(isset($_SESSION['login_user'])){
header("location: ../dashboard.php");}


?>

Thanks everyone, I really am very grateful for all of the help given

[Psycho]

Jacques:-  The database has a unique key on the username, so if it fails it will tell you due to the database query failure. I've done pretty much exactly what you just said Jacques thankfully my sql understanding is better than my php,

 

Jacques1 point is that the SELECT statement to check for a duplicate is faulty. Instead, you should just attempt the INSERT. Then, if the INSERT fails due to a duplicate constraint, you can then inform the user that the name is already taken. The current process has a hole in the logic. Granted it is very small, but it leaves the possibility for a race condition where two requests are made at the same time such that both SELECT queries pass, but then both try to do an INSERT with the same value. One will succeed and the other will fail - with no proper error handling. I believe the error code for a unique constraint is 1062. So, you would check for that error code after attempting the INSERT.

 

Also, you should almost always trim() user submitted values. A password would be one example that it would not make sense to do so. You currently have no validation of the user submitted values: e.g. ensuring required fields have values, that the values contain content appropriate for the context, etc. I assume you plan on adding that, but you would need to trim them before doing that. I would also add some logic to setting the variables from the post values to handle if a field is not passed. E.g.

 

 

$firstname = isset($_POST['FName']) ? trim($_POST['FName']) : false;;

 

Otherwise you will get warnings if a field isn't included.

[moosey_man1988]

Ah! I see! thanks guys i'll fix that up right away

[Muddy_Funster]

While you really should be using the insert rather than a select for this occasion, to elaborate more on what I said about the SELECT * :

 

When you use the SELECT * you are returning the data from every column in the table, asides from this not being best practice for transactional or relational queries, this means that you are exposing all that data through the query.  What would be safer would be to run a count query such as:

SELECT COUNT(*) as nameNum FROM tableName WHERE userName = :uName

This means that you are not exposing any real data from the table to the query result, then rather than checking the count of the result, just check the value:

$result = $selectResult->fetchAll(PDO::FETCH_ASSOC);
if($result[0]['nameNum'] === 0){
//good to go
}
else{
//oh dear team!
}

By doing this you don't pull unnecessary data out in your query and reduce risk from "man in the middle" style data sniffing as well as prevents you from having redundant data floating about your app.

 

Its similar to only selecting the unique ID for a user when checking for login success, as that's normally all you need to reference for persistence.

 

Hope that makes some sense, and good luck with the rest of your project

Edited November 24, 2015 by Muddy_Funster

[moosey_man1988]

Hi Muddy thanks for that, but i do need some of the data so maybe i will limit it down, each user that logs in have a "loginLevel" which is in the session restricting or allowing them to certain area's.

 

Thank you everyone for your help I will like all the answers and mark then best on who was straight to the point

 

Thank you