Is this code SQL injection proof?

Question

I don't see anyway for a SQL attack to happen with its all hard coded.

<?php
        $db = mysql_connect('host', 'user', 'pass') or die('Could not connect: ' . mysql_error());
        mysql_select_db('DBNAME') or die('Could not select database');
        // Strings must be escaped to prevent SQL injection attack. 
        $name      = mysql_real_escape_string($_GET['name'], $db);
        $score     = mysql_real_escape_string($_GET['score'], $db);
        $QuestionN = mysql_real_escape_string($_GET['QuestionN'], $db);
        $hash      = $_GET['hash'];
        $num       = (int)$QuestionN;
        $secretKey = "mykey"; # Change this value to match the value stored in the client javascript below 
        $real_hash = md5($name . $score . $secretKey);
        if ($real_hash == $hash) {
            // Send variables for the MySQL database class. 
            if ($QuestionN == "1") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 1 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 1 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 1 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 1 ";
                }
            }
            if ($QuestionN == "2") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 2 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 2 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 2 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 2 ";
                }
            }
            if ($QuestionN == "3") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 3 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 3 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 3 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 3 ";
                }
            }
            if ($QuestionN == "4") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 4 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 4 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 4 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 4 ";
                }
            }
            if ($QuestionN == "5") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 5 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 5 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 5 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 5 ";
                }
            }
            if ($QuestionN == "6") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 6 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 6 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 6 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 6 ";
                }
            }
            if ($QuestionN == "7") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 7 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 7 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 7 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 7 ";
                }
            }
            if ($QuestionN == "8") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 8 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 8 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 8 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 8 ";
                }
            }
            if ($QuestionN == "9") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 9 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 9 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 9 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 9 ";
                }
            }
            if ($QuestionN == "10") {
                if ($score == "A") {
                    $query = " UPDATE Quiz1 SET A = ( A + 1) WHERE Question = 10 ";
                }
                if ($score == "B") {
                    $query = " UPDATE Quiz1 SET B = ( B + 1) WHERE Question = 10 ";
                }
                if ($score == "C") {
                    $query = " UPDATE Quiz1 SET C = ( C + 1) WHERE Question = 10 ";
                }
                if ($score == "D") {
                    $query = " UPDATE Quiz1 SET D = ( D + 1) WHERE Question = 10 ";
                }
            }
            $result = mysql_query($query) or die('Query failed: ' . mysql_error());
        }
    ?>

Matt Gibson · Answer 1 · 2012-07-21 23:25:59Z

Yes, it's secure because you aren't allowing any variable to be inserted as part of a query. However it is horribly inefficient as it violates the DRY principle: you are writing the same code out in a dozen different places.

A better (and perfectly secure by design) approach would be to use a prepared statement via a parameterised data object (PDO), which would allow all the sanitising of the data to happen automatically. See examples here: http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php

$query = "UPDATE Quiz1 SET :score1 = ( :score2 + 1) WHERE Question = :question ";
$statement = $pdo->prepare($query);
$params = array(
    'score1' => 'A',
    'score2' => 'A',
    'question' => 2
);
$statement->execute($params);
foreach ($statement as $row) {
     // Do stuff.
}

You cannot parameterize entity names. (:score1 is not valid, since you'd essentially be doing SET 'A' = 'A' + 1, not SET A = A + 1.) — Corbin, Jul 22 '12 at 0:00
@Corbin Good catch - I missed that one. I'd edit to improve, but you've already posted some code that fixes the problem using an array of ascceptable values, so I'll leave it :) — Matt Gibson, Jul 22 '12 at 12:10

Corbin · Answer 2 · 2012-07-21 23:58:20Z

This code is secure, however, there are a lot of other issues with it.

In particular:

Don't assume array key exists

I've elaborated on this in various other posts, so here's a link.

In short, either use isset, empty or filter_input to ensure that you do not access an array key that does not exist.

Always be aware of context

if ($QuestionN == "1") {

Only use escaped data when you're actually putting it into the context where it needs to be escaped. Comparing or any kind of processing on escaped data doesn't make sense. For example, what if you had a $name that you needed to check the length of? O'Reilly would be 8 characters, but the MySQL escaped version, O\'Reilly would be incorrectly considered 9.

A ton of code repetition

Matt Gibson nailed it on this one, but his implementation is wrong (you cannot bind object names in prepared statements).

When working with entity and not data, instead of escaping, you'll want to always use whitelists (to prevent a user from using an incorrect or non-existent column, table, etc).

 $scoreColumn = (isset($_GET['score']) && is_string($_GET['score'])) ? $_GET['score'] : null;

$errors = array();

if (!in_array($scoreColumn, array('A', 'B', 'C', 'D'))) {
    //Uh oh!  This should be handled some how
    $errors[] = "Invalid score provided";
}

$question = (isset($_GET['question']) && is_string($_GET['question'])) ? (int) $_GET['question'] : null;

if ($question === null) {
    //Error: question not provided
    $errors[] = "No question provided";
} else if ($question < 1 || $question > 10) {
    //Error: invalid question provided
    $errors[] = "Invalid question provided";
}

if (!count($errors)) {
    $query = "UPDATE Quiz1 SET {$scoreColumn} = ({$scoreColumn} + 1) WHERE Question = {$question}";
}

mysql vs PDO

MySQL is an outdated, thin wrapped around MySQL's C API. You should be using either PDO or MySQLi (I strongly recommend PDO, but to each his own).

Schema

I suspect your schema could be changed to be more normalized. Hard to say with the information given, but I have a feeling that there's a few anti-patterns tucked away in your DB.

Thanks for all the help thus far, basically I need to send data from a small survey type app . Now this is my first exposure to PHP and MySql , I'm talking to my database from a C# script, but I don't allow ANY direct text input from the user- basically they hit a button, that button tells the app to send a few specific strings up and exicute the code above . Any help with PDO would be great ... Variables inside of Mysql queries has been an absolute nightmare . — Keithsoulasa, Jul 22 '12 at 3:35
This isn't working ether , do i have to change $question back into an int for the if < or > compare, then back into a string . So far I've been banging my head against the wall trying to submit variables inside of SQL queries — Keithsoulasa, Jul 22 '12 at 4:02
@user1539152 If C# application is connecting directly, that's a problem. I'm hoping you mean that the C# program makes calls to the script you posted in your original question? Anyway, $question needs to be an int for the inequalities, and it should also be an int when interpolating it into the string. Can you explain what problems you're having? Have you tried echoing out the query to make sure it contains what you expect it to? — Corbin, Jul 22 '12 at 21:43

Lotus · Answer 3 · 2012-07-21 23:35:15Z

up vote 0 down vote

Depends on if you actually use the variables containing the information you escaped.

// Strings must be escaped to prevent SQL injection attack. 
$name      = mysql_real_escape_string($_GET['name'], $db);
$score     = mysql_real_escape_string($_GET['score'], $db);
$QuestionN = mysql_real_escape_string($_GET['QuestionN'], $db);
$hash      = $_GET['hash'];

The problem with mysql_real_escape_string is that if you are using numeric values, they do not need to be quoted. If they are not quoted, you do not need a quote to break out from quotes. Thus, you can simply do: 0; drop table...

answered Jul 21 '12 at 23:35

Lotus
101

The problem isn't with mysql_real_escape_string; it's with people's misunderstanding of it. When working with numeric data, it should be interpolated into the query as numeric data, not as a string. mysql_real_escape_string is only meant to be used on strings. The easiest way to escape integers is to cast them to an integer in PHP. ($i = (int) $i; $q = "UPDATE ... SET c = {$i} WHERE ... ";) – Corbin Jul 21 '12 at 23:59

As stated before, the mysql_* functions are outdated and should not be used. – Bobby Jul 24 '12 at 13:02

add comment

asked	1 year ago
viewed	333 times
active	1 year ago

current community

your communities

more stack exchange communities

Is this code SQL injection proof?

3 Answers

Your Answer

Not the answer you're looking for? Browse other questions tagged php sql security or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

Is this code SQL injection proof?

3 Answers

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php sql security or ask your own question.

Linked

Related

Hot Network Questions