What's a good way to test my code for SQL injections?

**MutantJohn** · 09-11-2015

So, I'm trying to write a small user registration page using PHP and MySQL. I know, PHP AND MySQL, what is this? 1995? Ho ho ho!

Anyway, I'm curious as to how well I did with regards to security. Can anyone notice anything obviously apparent?

register.html :

Code:

<!DOCTYPE html>
<html>
<head>
	<title>ditacms User Registration</title>
</head>
<body>


	<p>Complete the registration form below</p>


	<form method="post" action="register.php" id="registration_form">
		
		<label>Username : </label>
		<input type="text" name="username" />
		<br/>


		<label>Email : </label>
		<input type="text" name="email" value="" />
		<br/>


		<label>Password : </label>
		<input type="text" name="password" value="" />
		<br/>


		<label>Re-Type Password : </label>
		<input type="text" name="confirm_password" value="" />
		<br/>


	</form>


	<button type="submit" form="registration_form">Register</button>


</body>
</html>

register.php

Code:

<!DOCTYPE html>
<html>
<head>
    <title>Registration Processing</title>
</head>
<body>


<?php


define( "EOL", "<br />\n" );


// data source name
define( "DSN", "mysql:host=localhost;dbname=ditacms;charset=utf8" );


define( "USER", "account_creator" );
define( "PASSWORD", "UrsaOwnsRoshan" );


function db_connect()
{
    try
    {
        $db = new PDO( DSN, USER, PASSWORD );
    }
    catch( PDOException $ex )
    {
        // echo $ex->getMessage();
        // echo $ex->getTraceAsString();
        echo "Attempt to connect to database failed!" . EOL;


        exit();
    }
    
    return $db;
}


function verify_post_register_params()
{
    $username = $_POST[ "username" ];
    $email = $_POST[ "email" ];
    $password = $_POST[ "password" ];
    $confirm_password = $_POST[ "confirm_password" ];


    // if the user left any field blank...
    if ( empty( $username ) ||
         empty( $email ) ||
         empty( $password ) ||
         empty( $confirm_password ) )
    {
        echo "Empty field found in form submission!" . EOL;
        echo "Please complete the form." . EOL;
        return false;
    }


    // if the passwords do not exactly match...
    if ( strcmp( $password, $confirm_password ) !== 0 )
    {
        echo "Password mismatch!";
        return false;
    }


    $username = filter_var( $username, FILTER_SANITIZE_STRING );
    $email = filter_var( $email, FILTER_SANITIZE_EMAIL );
    $password = filter_var( $password, FILTER_SANITIZE_STRING );


    if ( $username === false ||
         $email === false ||
         $password === false )
    {
        echo "Sanitization failed! Potential attack!!!" . EOL;
        return false;
    }


    if ( filter_var( $email, FILTER_VALIDATE_EMAIL ) === false )
    {
        echo "Invalid email address!" . EOL;
        return false;
    }


    $register = array( "username" => $username,
                       "email" => $email,
                       "password" => $password );


    return $register;
}


function user_exists( $db, $username )
{
    $query = $db->prepare( "SELECT username FROM `ditacms`.`members` WHERE username = :username" );
    $query->bindValue( ":username", $username, PDO::PARAM_STR );
    $query->execute();


    $rows = $query->fetchAll( PDO::FETCH_ASSOC );


    // if the rows returned are empty, the user
    // does NOT exist so return false
    if ( empty( $rows ) === true )
    {
        return false;
    }


    // if the rows returned are NOT empty, the
    // user DOES exist so return true
    else
    {
        echo "A user with that username already exists!" . EOL;
        return true;
    }
}


function create_new_user( $db, $username, $email, $password )
{
    echo "Creating new user..." . EOL;


    $insert = $db->prepare( "INSERT INTO `ditacms`.`members` (username, email, password)
                             VALUES(:username, :email, :password)" );


    $hash = password_hash( $hash, PASSWORD_DEFAULT );


    $insert->bindValue( ":username", $username, PDO::PARAM_STR );
    $insert->bindValue( ":email", $email, PDO::PARAM_STR );
    $insert->bindValue( ":password", $hash, PDO::PARAM_STR );




    if ( $insert->execute() === false )
    {        
        echo "Insertion failure..." . EOL;
        return false;
    }
    else
    {
        echo "Successfully registered new account!" . EOL;
        return true;
    }
}




/*
 * main() loop
 */


echo "<p>Processing user registration request...</p>";


$register = verify_post_register_params();


if ( $register === false )
{
    echo "Bad POST parameters. Exiting script..." . EOL;   
}
else
{
    $db = db_connect();


    // if the user does NOT exist, create one
    if ( user_exists( $db, $register[ "username" ] ) === false )
    {
        create_new_user( $db, $register[ "username" ],
                              $register[ "email" ],
                              $register[ "password" ] );
    }    
}




?>


    <a href="/ditacms.com/register.html">Return to registration page</a>


    <br />


    <a href="/ditacms.com/">Return to homepage</a>


</body>
</html>

I hear that SQL injections can still happen with POST data so I'm just wondering if there's anything I obviously forgot or didn't even register.

Also, the account_creator MySQL account can only SELECT, INSERT and UPDATE.

**laserlight** · 09-11-2015

You're binding parameters with prepared statements, so you should be fine.

**MutantJohn** · 09-11-2015

Holy poop, that was fast O_o

I actually came back to this topic because some relevant info, the database table is setup like this :

Code:

mysql> DESCRIBE `ditacms`.`members`;
+----------+-------------+------+-----+---------+----------------+
| Field    | Type        | Null | Key | Default | Extra          |
+----------+-------------+------+-----+---------+----------------+
| id       | int(11)     | NO   | PRI | NULL    | auto_increment |
| username | varchar(30) | NO   |     | NULL    |                |
| email    | varchar(50) | NO   |     | NULL    |                |
| password | char(255)   | NO   |     | NULL    |                |
+----------+-------------+------+-----+---------+----------------+
4 rows in set (0.00 sec)

**laserlight** · 09-11-2015

You probably want a unique index on the username column.

**MutantJohn** · 09-11-2015

Done!!!

Code:

mysql> DESCRIBE `ditacms`.`members`;
+----------+-------------+------+-----+---------+----------------+
| Field    | Type        | Null | Key | Default | Extra          |
+----------+-------------+------+-----+---------+----------------+
| id       | int(11)     | NO   | PRI | NULL    | auto_increment |
| username | varchar(30) | NO   | UNI | NULL    |                |
| email    | varchar(50) | NO   |     | NULL    |                |
| password | char(255)   | NO   |     | NULL    |                |
+----------+-------------+------+-----+---------+----------------+
4 rows in set (0.00 sec)

Does this mean I should remove the code that does the query for an already existing username and just rely on the

Code:

$query->execute()

method to fail on its own?

Edit : Can we also just take a minute to appreciate how cool PHP's password hashing is? It'll automatically salt the hash and everything! I spent like two days reading about salts and hashing and then lo and behold, modern PHP has this built-in functionality. I was like, well, that was a fun read, I guess XD

**laserlight** · 09-11-2015

Originally Posted by MutantJohn

Does this mean I should remove the code that does the query for an already existing username and just rely on the

Code:

$query->execute()

method to fail on its own?

Yes, but because the SQL statement's execution could fail for other reasons, you should check the error code for a unique constraint violation, e.g., by writing a utility function to do so, which would help to reduce the problems with tight coupling with MySQL as the function could later check with respect to other database vendors.

In verify_post_register_params, you probably should not be using filter_var() with FILTER_SANITIZE_STRING on $password. At the most you should use trim(). After all, you are going to hash the password anyway.

In create_new_user, it looks like this:

PHP Code:


$hash = password_hash( $hash, PASSWORD_DEFAULT );

should have been:

PHP Code:


$hash = password_hash( $password, PASSWORD_DEFAULT );

**MutantJohn** · 09-11-2015

Oh my God, thank you for catching such a stupid bug lol. Oh man, there's no language on Earth that can prevent that kind of dozy programming T_T

I also implemented your other advice too! I've trim()'d the password and then I reset the create_new_user function to do this instead :

Code:

    if ( $insert->execute() === false )
    {        
        $arr = $insert->errorInfo();


        echo "Insertion failure..." . EOL;
        print_r( $arr );
        echo EOL;


        return false;
    }
    else
    {
        echo "Successfully registered new account!" . EOL;
        return true;
    }

That way my output looks more like this :

Code:

Insertion failure...
Array ( [0] => 23000 [1] => 1062 [2] => Duplicate entry 'Prelude2Entropy' for key 'username' )

It's not the most well-formatted but for now it'll do. It's also more expressive just in case any other database statement fails to execute.

Thank you for your advice, laser! I really appreciate it!

**Yarin** · 09-11-2015

Originally Posted by MutantJohn

Oh my God, thank you for catching such a stupid bug lol. Oh man, there's no language on Earth that can prevent that kind of dozy programming T_T

There is, actually. Any statically typed language that types its hashes, would likely have caught that.

**MutantJohn** · 09-11-2015

Is there even a statically typed language that could do the stuff I'm using PHP for there?

**Mario F.** · 09-11-2015

Originally Posted by MutantJohn

Is there even a statically typed language that could do the stuff I'm using PHP for there?

Sheesh, where do you want to start?

C (via CGI), Go, C# (via ASP.Net), Haskell (through Yesod), Perl of course.

**MutantJohn** · 09-11-2015

Hmm... Perl is statically typed? Neat.

But PHP is better. Shazam!

Lol jk. I love you, Mario.

**Mario F.** · 09-11-2015

Originally Posted by MutantJohn

But PHP is better. Shazam!

Make no mistake. It's better than any of the things I listed. But you were asking for statically typed languages.

Now, there may be an argument for C#. It's a much better language than PHP in every single way. Unfortunately it suffers from two big disadvantages:

1 - ASP.NET is Microsoft's. And in the current environment you probably will want to stay as far away from this company as possible.
2 - ASP.Net is Microsoft's. And that means the company that likes to introduce new technologies, have people invest their time learning them and then discontinue said technologies.
3 - ASP.Net is Microsoft's. Which means Windows servers are the only place where you can expect it to run without hiccups.
4 - ASP.Net is Microsoft's.

That's 4 disadvantages actually.

**stevesmithx** · 09-11-2015

While you can do this by inspecting the code for the necessary safeguards, you can also do some form of black box penetration testing using some free tools available out there. For example,
https://addons.mozilla.org/en-US/fir...sql-inject-me/

has all sorts of monkey inputs needed for SQL injection. However this approach is useful only if you do NOT have access to the source code (make sure you have the permission of the website owner in that case, it is illegal to do otherwise).

**Yarin** · 09-11-2015

Originally Posted by MutantJohn

Hmm... Perl is statically typed? Neat.

Lol. No.

Originally Posted by MutantJohn

But PHP is better. Shazam!

It's definitely easier, at least. But, it does just about everything else wrong.

**laserlight** · 09-12-2015

Originally Posted by MutantJohn

Oh my God, thank you for catching such a stupid bug lol. Oh man, there's no language on Earth that can prevent that kind of dozy programming T_T

Originally Posted by Yarin

There is, actually. Any statically typed language that types its hashes, would likely have caught that.

Actually, running that function with error_reporting set to E_ALL should have caught that: with a sufficiently high error reporting level, a notice would be issued informing you that $hash was undefined at that point of use. The current default excludes E_NOTICE, so it is up to you to configure it properly for development.

Thread: What's a good way to test my code for SQL injections?

Thread Tools

Search Thread

Display

What's a good way to test my code for SQL injections?

Similar Threads

code to test odd/even

Good way to prepare for an object oriented job test

please test my code!!!