CODEWITHSUNDEEP
phpmysqlsecurityprepared-statement
mr mojo risin
mr mojo risin

Reputation: 555

Dynamic Prepared Statements - Does this open security holes?

This is similar to this question - Are Dynamic Prepared Statements Bad? (with php + mysqli), however since it is 4 years old I wanted to get a more upto date answer.

I've written a class which, although I haven't tested it on more copmlex sql queries, it has worked without fail on simple sql queries, however I'm not sure if doing so has bypassed one of the main reasons for prepared statements - security.

I have made use of the call_user_func_array which was easy enough with the bind_param statements however with the bind_result was a little trickier. I originally used get_result however the host I've gone with doesn't have mysqlnd available, but I managed to get around using the metadata. This is the full class I have written.

Do you think this is secure?

The passed in values are:

The class:

class crudModel {
    function ps($sql, $mysqli, $para) {
        //this function should work for just about any simple mysql statement
        //for more complicated stuff like joins, unions etc,. we will see
        if ($prep = $mysqli->prepare($sql)) {
            call_user_func_array(array($prep, 'bind_param'), $this->makeValuesRef($para, $mysqli));
            $prep->execute();
            $meta = $prep->result_metadata();
            while ($field = $meta->fetch_field()) {
                $parameters[] = &$row[$field->name];
            }
            call_user_func_array(array($prep, 'bind_result'), $parameters);
            while ($prep->fetch()) {
                foreach ($row as $key=>$val) {
                    $x[$key] = $val;
                }
                $data[] = $x;
            }
            return $data;
        }
    }

    function makeValuesRef($array, $mysqli) {
        $refs = array();
        foreach($array as $key => $value) {
            $array[$key] = $mysqli->real_escape_string($value); //i don't think escaping is necessary, but it can't hurt (??)
            $refs[$key] = &$array[$key];
        }
        return $refs;
    }
}

Upvotes: 1

Views: 266

Answers (1)

ircmaxell
ircmaxell

Reputation: 165261

What you're doing here isn't a dynamic prepared statement. You're just putting some syntatic sugar on top of the MySQLi API (which sucks).

In short, there aren't really any security concerns present from the code you've shown here. In fact, this sort of practice is quite good, because it makes it easier to verify that you're doing it correctly later (since the MySQLi API sucks).

So you're fine. I would worry about the areas you're generating the queries, and ensuring that you're not accidentally putting user-data into them without white-listing...

Upvotes: 1

Related Questions