0

I have a registration form on my site and I figured I should protect from SQL injection. I can't have that table being dropped maliciously.

Using POST, I collect the input from the form, check it, and then add it to the database. My code is below. I've been testing the form, and though the form is submitted successfully, the table is filled with an empty row...not the data from the form.

What's going on here?

<?php

        $type = $_POST['type']; // a dropdown
        $color = $_POST['color']; // a dropdown 
        $name = mysql_real_escape_string($_POST['name']);
        $address = mysql_real_escape_string($_POST['address']);
        $city = mysql_real_escape_string($_POST['city']);
        $state = $_POST['state']; // a dropdown
        $zip = mysql_real_escape_string($_POST['zip']);
        $phone = mysql_real_escape_string($_POST['phone']);
        $email = mysql_real_escape_string($_POST['email']);
        $where = mysql_real_escape_string($_POST['where']);
        $price = mysql_real_escape_string($_POST['price']);
        $use = mysql_real_escape_string($_POST['use']);

        include 'php/Connect.php';
        $ct = new Connect();
        $con = $ct->connect();  

        if(check($email, $con)) {
            if(register($type, $color, $name, $address, $city, $state, $zip, $phone, $email, $where, $price, $use, $con)) {
                echo '<h1>Success!</h1><p>Thanks for registering your product. A confirmation email has been sent to '.$email.'.</p>';
            }
            else {
                echo '<h1>Error!</h1><p>There were errors processing your registration. Please try again.</p>'; 
            }
        }
        else {
            echo '<h1>Error!</h1><p>This product has already been registered.</p>';
        }

        function check($email, $con) {
            $query = "SELECT * FROM registrations WHERE email='$email'";
            $res = mysql_query($query, $con);
            if ($con) {
                $row = mysql_fetch_assoc($res);
                if($row) {
                    return false; // product registration exists    
                }
                else {
                    return true; // product registration does not exist
                }
            }
            else {
                return false; 
            }
        }

        function register($type, $color, $name, $address, $city, $state, $zip, $phone, $email, $where, $price, $use, $con) {
            $query = "INSERT INTO registrations VALUES ('$type', '$color', '$name', '$address', '$city', '$state', '$zip', '$phone', '$email', '$where', '$price', '$use')";
            $res = mysql_query($query, $con);
            if (!$con) {
                return false;
            }
            else {
                mysql_close($con);
                return true; 
            }
        }
    ?>  
Rosmarine Popcorn
  • 10,761
  • 11
  • 59
  • 89
Jon
  • 3,154
  • 13
  • 53
  • 96
  • 3
    Please, don't use `mysql_*` functions for new code. They are no longer maintained and the community has begun the [deprecation process](http://goo.gl/KJveJ). See the [**red box**](http://goo.gl/GPmFd)? Instead you should learn about [prepared statements](http://goo.gl/vn8zQ) and use either [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli). If you can't decide, [this article](http://goo.gl/3gqF9) will help to choose. If you care to learn, [here is a good PDO tutorial](http://goo.gl/vFWnC). – PeeHaa Jul 12 '12 at 15:59
  • 1
    ^ using prepared statements and bound parameters you will not have to worry about SQLi. – PeeHaa Jul 12 '12 at 16:00
  • Try echoing out the SQL you're generating, to make sure it looks like what you're expecting. You should also try running that SQL directly in the database to see what happens. – andrewsi Jul 12 '12 at 16:01
  • Also check out this kinda related [question](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php). – PeeHaa Jul 12 '12 at 16:01
  • 1
    Also note that you cannot use `mysql_real_escape_string` before you [have a connection](http://php.net/manual/en/function.mysql-real-escape-string.php). – PeeHaa Jul 12 '12 at 16:03
  • Thanks PeeHaa...I had no idea. I'm still pretty new to MySQL and PHP, so I'll be sure to make this transition. Thanks. – Jon Jul 12 '12 at 16:06
  • @Jon yw. It sucks to write "ancient" code without knowing it. – PeeHaa Jul 12 '12 at 16:08
  • Is it as simple as replacing all of my mysql_* statements with mysqli_* ? – Jon Jul 12 '12 at 16:11

2 Answers2

0

Connect to database before using mysql_real_escape_string
However it is better to use the newer version

$connect=mysqli_connect(.......);  
mysqli_real_escape_string($connect,$string);
leet
  • 933
  • 1
  • 13
  • 27
0

Fixed it with PeeHaa's help. Here's the corrected code:

<?php

        include 'php/Connect.php';
        $ct = new Connect();
        $db = $ct->connect();

        $type = $_POST['type'];
        $color = $_POST['color'];
        $name = $_POST['name'];
        $address = $_POST['address'];
        $city = $_POST['city'];
        $state = $_POST['state'];
        $zip = $_POST['zip'];
        $phone = $_POST['phone'];
        $email = $_POST['email'];
        $where = $_POST['where'];
        $price = $_POST['price'];
        $use = $_POST['use'];   

        if(check($email, $db)) {
            if(register($type, $color, $name, $address, $city, $state, $zip, $phone, $email, $where, $price, $use, $db)) {
                echo '<h1>Success!</h1><p>Thanks for registering your product. A confirmation email has been sent to '.$email.'.</p>';
            }
            else {
                echo '<h1>Error!</h1><p>There were errors processing your registration. Please try again.</p>'; 
            }
        }
        else {
            echo '<h1>Error!</h1><p>This product has already been registered.</p>';
        }

        function check($email, $db) {

            $stmt = $db->prepare("SELECT * FROM registrations WHERE email=?");
            $stmt->execute(array($email));
            $rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
            if ($db) {
                if($rows) {
                    return false; // product registration exists    
                }
                else {
                    return true; // product registration does not exist
                }
            }
            else {
                return false; 
            }
        }

        function register($type, $color, $name, $address, $city, $state, $zip, $phone, $email, $where, $price, $use, $db) {

            $stmt = $db->prepare("INSERT INTO registrations VALUES(?,?,?,?,?,?,?,?,?,?,?,?)");
            $stmt->execute(array($type, $color, $name, $address, $city, $state, $zip, $phone, $email, $where, $price, $use));

            if (!$db) {
                return false;
            }
            else {
                mysql_close($db);
                return true; 
            }
        }
    ?>  
Jon
  • 3,154
  • 13
  • 53
  • 96