CODEWITHSUNDEEP
phpmysqlsql-injection
tbone
tbone

Reputation: 5865

Is this PHP/MySQL statement vulnerable to SQL injection?

Should be a simple question, I'm just not familiar with PHP syntax and I am wondering if the following code is safe from SQL injection attacks?:

private function _getAllIngredients($animal = null, $type = null) {
    $ingredients = null;
    if($animal != null && $type != null) {
        $query = 'SELECT id, name, brief_description, description, 
                         food_type, ingredient_type, image, price,
                         created_on, updated_on
                    FROM ingredient
                   WHERE food_type = \'' . $animal . '\'
                     AND ingredient_type =\'' . $type . '\';';
        $rows = $this->query($query);

        if(count($rows) > 0) {

etc, etc, etc

I've googled around a bit and it seems that injection safe code seems to look different than the WHERE food_type = \'' . $animal . '\' syntax used here.

Sorry, I don't know what version of PHP or MySQL that is being used here, or if any 3rd party libraries are being used, can anyone with expertise offer any input?

UPDATE

What purpose does the \ serve in the statement?: 

WHERE food_type = \'' . $animal . '\'

In my googling, I came across many references to mysql_real_escape_string...is this a function to protect from SQL Injection and other nastiness?

The class declaration is:

class DCIngredient extends SSDataController

So is it conceivable that mysql_real_escape_string is included in there?
Should I be asking to see the implementation of SSDataController?

Upvotes: 0

Views: 790

Answers (7)

Roy Rico
Roy Rico

Reputation: 3841

$animal can be a string which contains '; drop table blah; -- so yes, this is vunerable to SQL injection.

You should look into using prepared statements, where you bind parameters, so that injection cannot occur:

https://www.php.net/pdo.prepared-statements

Upvotes: 1

Peter Parker
Peter Parker

Reputation: 29735

Yes this code is vulnerable to SQL-Injection.

The "\" escapse only the quote character, otherwise PHP thinks the quote will end your (sql-)string.

Also as you deliver the whole SQL-String to the SSDataControler Class, it is not possible anymore to avoid the attack, if a prepared string has been injected.

So the class SSDataControler is broken (vulnerable) by design.

try something more safe like this:

$db_connection = new mysqli("host", "user", "pass", "db");
$statement = $db_connection->prepare("SELECT id, name, brief_description, description, 
                     food_type, ingredient_type, image, price,
                     created_on, updated_on
                FROM ingredient
               WHERE food_type = ?
                 AND ingredient_type = ?;';");
$statement->bind_param("s", $animal);
$statement->bind_param("s", $type);
$statement->execute();

by using the bind method, you can specify the type of your parameter(s for string, i for integer, etc) and you will never think about sql injection again

Upvotes: 1

Tyler Carter
Tyler Carter

Reputation: 61597

Re: UPDATE

The \'' serves to enclose the variable in quotes, so that when it goes through to SQL, nothing is broken by whitespace. IE: Where Something=Cold Turkey would end with an error, where Something='Cold Turkey' would not.

Furthermore, the class extension won't affect how you put together MySQL Statements.

Upvotes: 0

Alex S
Alex S

Reputation: 26061

If its a private function, kind of. If you're concerned about people with access to the source injecting SQL, there are simpler ways to accomplish their goal. I recommend passing the arguments to mysql_real_escape_string() before use, just to be safe. Like this:

private function _getAllIngredients($animal = null, $type = null) {
        $animal = mysql_real_escape_string($animal);
        $type   = mysql_real_escape_string($type);
        ...
        }

To be extra safe, you might even pass it to htmlentities() with the ENT_QUOTES flag instead - that would also help safeguard against XSS type stuff, unless you're putting the contents of those DB entries into a <script> tag.

But the safest thing you can do is write your own sanitizing function, which would make the best of various techniques, and also allow you to easily protect against new threats which may arise in the future.

Upvotes: 0

Nico Burns
Nico Burns

Reputation: 17109

If the the user or any 3rd party has anyway of injecting a value of $animal into your system (which they probably do), then yes it is vunerable to sql injection.

The way to get around this would be to do

private function _getAllIngredients($animal = null, $type = null) {
    $ingredients = null;
    if($animal != null && $type != null) {
        $query = 'SELECT id, name, brief_description, description, 
                         food_type, ingredient_type, image, price,
                         created_on, updated_on
                    FROM ingredient
        $rows = $this->query($query);
        if(count($rows) > 0) {
        if($rows['animal'] == $animal && $rows['ingredient_type'] == $type) {

Note: I removed WHERE statements from sql and added if statements to loop

Upvotes: -2

Tyler Carter
Tyler Carter

Reputation: 61597

You might as well use mysql_real_escape_string anyway to get rid of anything that could do a drop table, or execute any other arbitrary code.

It doesn't matter where in the SQL statement you put the values, at any point it can have a ';' in it, which immediately ends the statement and starts a new one, which means the hacker can do almost anything he wants.

To be safe, just wrap your values in mysql_real_escape_string($variable). So:

WHERE Something='".mysql_real_escape_string($variable)."'

Upvotes: 1

Edward
Edward

Reputation: 1826

It is vulnerable. If I passed: '\'; DROP TABLE ingredient; SELECT \'' as $type, poof, goodbye ingredient table.

Upvotes: 0

Related Questions