What about this code introduces an SQL injection vulnerability?

Question

This code was taken from another post at Notice: Undefined offset: 0 in where it was pointed out that there is an SQL injection vulnerability. What about this code is vulnerable, how could it be exploited, and how can it be made safe?

<?php 
include("config.php"); 

function getAllVotes($id) 
{ 
    $votes = array(); 
    $q = "SELECT * FROM entries WHERE id = $id"; 
    $r = mysql_query($q); 
    if(mysql_num_rows($r)==1)//id found in the table 
    { 
        $row = mysql_fetch_assoc($r); 
        $votes[0] = $row['votes_up']; 
        $votes[1] = $row['votes_down']; 
    } 
    return $votes; 
} 

function getEffectiveVotes($id) 
{ 
        $votes = getAllVotes($id); 
        $effectiveVote = $votes[0] - $votes[1];    //ERROR THROWN HERE
        return $effectiveVote; 
} 

$id = $_POST['id']; 
$action = $_POST['action']; 

//get the current votes 
$cur_votes = getAllVotes($id); 

//ok, now update the votes 

if($action=='vote_up') //voting up 
{ 

    $votes_up = $cur_votes[0]+1;     //AND ERROR THROWN HERE


    $q = "UPDATE threads SET votes_up = $votes_up WHERE id = $id"; 
} 
elseif($action=='vote_down')
{ 
    $votes_down = $cur_votes[1]+1; 
    $q = "UPDATE threads SET votes_down = $votes_down WHERE id = $id"; 
} 

$r = mysql_query($q); 
if($r)
{ 
    $effectiveVote = getEffectiveVotes($id); 
    echo $effectiveVote." votes"; 
} 
elseif(!$r) //voting failed 
{ 
    echo "Failed!"; 
} 
?>

this: ` WHERE id = $id` . What if I were to pass "$id = 17; DROP TABLE entries" ? You can read more about sql injection [here](http://bobby-tables.com/). You can avoid it by using [prepared statements.](https://dev.mysql.com/doc/refman/5.0/en/sql-syntax-prepared-statements.html) There are also other techniques, like casting $id to integer if you know it should always be an integer. — devlin carnate, Oct 18 '15 at 02:32
See [exploits of a mom](http://imgs.xkcd.com/comics/exploits_of_a_mom.png) — user3791372, Oct 18 '15 at 02:34
Well done @devlincarnate. I forgot the name "prepared statements" in my answer. Also, for providing the alternate solution (casting). — andrewgu, Oct 18 '15 at 02:34

score 0 · Answer 1 · answered Oct 18 '15 at 02:31

The code doesn't use mysqli or PDO to safely execute the SQL statement. Both mysqli and PDO send the statement and user-defined variables to the server separately, where they are interpreted and acted upon. The code instead allows people to add on their own SQL code (an SQL injection) to the statement through the $_POST['id'] variable.

For example, if I were to POST the following as the id, you would lose your table:

avalidid; DROP TABLE entries

What about this code introduces an SQL injection vulnerability?

1 Answers1