Is there a way to make this more secure?

[Miggy64]

The more I look at this code the more i think to myself that there is some kind of security hole in it, but at other times I say that it'll do.

 

Here's the code in question:

 

part of my jquery script:

		if ( proceed ) {
			//console.log('All the conditions have been met.');
			var data = $('#registerForm input').serialize(); // Put form data into serialize format:

			/* Save Function by grabbing & sending data to register.php */
			$.post($('#registerForm').attr('action'), data , function(info) {
				console.log(info);
				//$('#result').text(info); // Display the result back when saved: 
			}); // End of Save:
			
		} else {
			console.log('There is a problem somewhere.');
		}

and my php file that the data is sent to:

if (isset($_POST['username'])) {
	$userType = 'public';
	$username = $_POST['username'];
	$realname = $_POST['realname'];
	$email = $_POST['email'];
	$password = password_hash(trim($_POST['password']), PASSWORD_BCRYPT, array("cost" => 15));

	$query = 'INSERT INTO users (userType, username, realname, email, password, dateAdded) VALUES (:userType, :username, :realname, :email, :password, NOW())';
	$stmt = $pdo->prepare($query);

	try 
	{
		$result = $stmt->execute(array(':userType' => $userType, ':username' => $username, ':realname' => $realname, ':email' => $email, ':password' => $password));
		if ($result) {
			echo 'Data Successfully Inserted!'; 
		}
	} 
	catch(PDOException $error) 
	{
		if (substr($error->getCode(), 0, 2) == SQL_CONSTRAINT_VIOLATION) {
			$errorMsg = 'The username already exists.';
		} else {
					throw $error; // some other error happened; just pass it on.	
				}		
			}
			
		}

Basically it takes the data from the registration form, validates it and then sends it to the register.php file to insert the data in the database table. I will be a long time before I go live with this, but I want to make this as secure as I can. An suggestions or help will be greatly appreciated.

 

Best Regards,

      John

[gristoi]

firstly you have to realise that you are sending the data using ajax / javascript, and as this is client side you can never make that aspect of it completely secure. What you need to focus on is ensuring that the php side is escaping and validating all of the data being passed to it. looking at the code you have you are going in the right direction by using prepared statements.

[Jacques1]

I'm impressed. It's very. very rare to see such excellent code outside of big projects: bcrypt, prepared statements, even a proper uniqueness check. Wow.

 

Only one thing: bcrypt has an input limit of 56 bytes, so you should validate that. Longer strings are outside of the specificiation, and everything after 72 bytes is truncated.

 

Besides that, I don't see any security issues in the code. What does your code for establishing a PDO connection look like? This is a common source for errors. 

 

 

 

@ gristoi: What does JavaScript have to do with anything? If he sent the data with a classical form, it would be client-side as well, so there's nothing special about Ajax requests.

Edited July 28, 2014 by Jacques1

[Psycho]

A few critiques:

 

1. That code is trimming the password. I see no logical reason to do that as it would only potentially reduce the strength of the password. A user may add spaces to the beginning or end of a common word to create their password, which would at least prevent a cursory dictionary attack or someone trying values that they know about the user.

 

2. That code does not trim the 'text' values. Contrary to passwords, I always trim other input from the user unless I have a specific reason not to. You don't appear to have any required field checks, but if you did, not trimming spaces would allow a person to get around those with just spaces.

 

3. There is a small hole in the logic. In the try block there is an if() check to verify there was a successful result. But, there is no else condition. It is assuming that if there is an error it would be handled by the catch block. If that's the case, then there is no need for the if() condition. But, if there is the possibility that the query does not fail, but does not produce the intended outcome it would produce no results whatsoever. You could consider changing that if() condition to a check of the affected rows to verify it is equal to 1 - then add an else condition to report an 'unknown' error.

[Miggy64]

Thanks for the great input people, as for my PDO connection class script looks as follows:

class Database {

    private $_connection;
    // Store the single instance.
    private static $_instance;

    // Get an instance of the Database.
    // @return Database: 
    public static function getInstance() {
        if (!self::$_instance) {
            self::$_instance = new self();
        }
        return self::$_instance;
    }

    // Constructor - Build the PDO Connection:
    public function __construct() {
        $db_options = array(
            PDO::ATTR_EMULATE_PREPARES => false                     // important! use actual prepared statements (default: emulate prepared statements)
            , PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION           // throw exceptions on errors (default: stay silent)
            , PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC      // fetch associative arrays (default: mixed arrays)
        );
        $this->_connection = new PDO('mysql:host=localhost;dbname=cuckoo;charset=utf8', 'username', 'password', $db_options);     
    }

    // Empty clone magic method to prevent duplication:
    private function __clone() {
        
    }

    // Get the PDO connection:    
    public function getConnection() {
        return $this->_connection;
    }

From what I have read online it's not the greatest and right now I have a mental block of the word(s) that describes the issue; however, I modified this script from a php book by Larry Ullman and assume that it will suffice. I think it's relatively secure.

[gristoi]

jaques1 ,I was just highlighting the fact that there is no way to 'secure' client side code, specifically javascript because this was posted in the 'Ajax help' section. And that all validation needed to be server side to help stop xss

Edited July 28, 2014 by gristoi

[Miggy64]

A few critiques:

 

1. That code is trimming the password. I see no logical reason to do that as it would only potentially reduce the strength of the password. A user may add spaces to the beginning or end of a common word to create their password, which would at least prevent a cursory dictionary attack or someone trying values that they know about the user.

 

2. That code does not trim the 'text' values. Contrary to passwords, I always trim other input from the user unless I have a specific reason not to. You don't appear to have any required field checks, but if you did, not trimming spaces would allow a person to get around those with just spaces.

 

3. There is a small hole in the logic. In the try block there is an if() check to verify there was a successful result. But, there is no else condition. It is assuming that if there is an error it would be handled by the catch block. If that's the case, then there is no need for the if() condition. But, if there is the possibility that the query does not fail, but does not produce the intended outcome it would produce no results whatsoever. You could consider changing that if() condition to a check of the affected rows to verify it is equal to 1 - then add an else condition to report an 'unknown' error.

If I'm understand number correctly it would be OK just to remove the if statement and echo back to the JavaScript for that is what the try-catch is doing in the first place?I just want to send a response back to the Javascript stating that the data was saved correctly.

I took out trim on the password, for the life of me I don't know why I put it in there in the first place. I will trim the other fields of spaces and I plan having some sort of field checks on them either by JavaScript or PHP in the future. Thanks for the critiques and suggestions.

[Jacques1]

From what I have read online it's not the greatest and right now I have a mental block of the word(s) that describes the issue; however, I modified this script from a php book by Larry Ullman and assume that it will suffice. I think it's relatively secure.

 

The code is fine. I was mainly concerned about PDO::ATTR_EMULATE_PREPARES being turned on (which is the default), but you did deactivate it.

 

You might want to use utfmb4 instead of utf8 as your character set. What MySQL calls “utf8” is only the Base Multilingual Plane (~65,000 characters) rather than the complete Unicode space (currently ~110,000 characters). While the BMP is usually sufficient, most people aren't even aware that they only get a subset of Unicode.

 

 

 

jaques1 ,I was just highlighting the fact that there is no way to 'secure' client side code, specifically javascript because this was posted in the 'Ajax help' section. And that all validation needed to be server side to help stop xss

 

Well, of course all checks need to be done server-side, but this has nothing to do with Ajax or JavaScript in particular. You'd have the exact same problem with a classical HTML form, so the comment seemed to be somewhat out of place. It sounded like if there was something inherently insecure about Ajax-base registration (which of course isn't the case).

[Jacques1]

The if ($result) check is indeed unnecessary (altough this is somewhat nitpicky). It seems you're confusing the classical return-value-based error handling with the new exception-based error handling. In the classical system, you'd indeed check the return value of the execute() method to see if the query succeeded or failed. However, you're using the exception-based system in your code, so PHP will throw an exception in case of an error rather than make execute() return false. In other words, the return value is irrelevant, because the success or failure is determined by the control flow.

 

Since we're already nitpicking, you should replace the human-readable feedback with machine-readable feedback. As you've already said, the entire purpose of the script is to give feedback to JavaScript, and a message like “Data Successfully Inserted!” is not very suitable for that.

 

You should use the HTTP response code to indicate the status: 2xx means success, 4xx means a client error, 5xx means a server error. You can give the exact cause as a message, but you should not use some hard-coded English text. Use error identifiers like “duplicate_entry”. Then you can map them to an actual message later (and use different languages, for example).

 

If you have a complex response, use JSON. This allows you to have an actual data structure which is easy to parse in JavaScript.

[Miggy64]

Thanks Jacques1, that has cleared a lot of things up and I learning something new everyday. HTTP Response codes makes a lot more sense when getting a response back from php. Now I know where to go, I will delve deeper into this area. Once again Thanks.

[Miggy64]

To bad you can't mark more than one as best answer for there were a lot of good answers. However, I just want to mark this as solved, for I feel I am now on the right path and plan on delving deeper in this matter now that I know which direction to take. Thanks for everyone's help and input.

Edited July 29, 2014 by Miggy64