Is using PHP's Reflection Class to execute MySQL queries secure?

Question

Background

I have been in the process of developing a Content Management System from scratch. It is primarily being developed from scratch for experience reasons, but it will also be used down the road.

I developed a DatabaseManager class which extends the mysqli class in PHP. I am using it do MySQL queries and connections. A function I have developed passes both the SQL query string (which is parameterized) followed by an array of the correct values to be replaced.

TL;DR

Long story short, I'm using a ReflectionClass to bind the parameters to the SQL query and execute. I am curious if this is a secure approach, or is there another method that would be best seen fit?

The Method

This is the DatabaseManager::do_query() method:

function do_query($sql, $values){
    if(!isset($this->connect_error)){
        $num_vals = count($values);
        $i = 0;
        $type = "";
        while($i < $num_vals){
            if(is_int($values[$i]) == true)
                $type .= "i";
            elseif(is_string($values[$i]) == true)
                $type .= "s";
            $i++;
        }

        $i = 0;
        while($i < $num_vals){
            $values2[$i] = &$values[$i];
            $i++;
        }
        $values2 = array_merge(array($type), $values2);

        $expr = $this->prepare($sql);
        if($expr != false){
            $ref = new ReflectionClass('mysqli_stmt');
            $method = $ref->getMethod("bind_param");            
            $method->invokeArgs($expr, $values2);

            $expr->execute();
            return "Success";
        }else{
            $error_string = "Error: Query preparation resulted in an error. ";
            $error_string .= $this->error;
            __TGErrorHandler(TG_ERROR_DB_PREP);
            return $error_string;
        }

    }
}

I have not run into any direct errors by testing, and it seems to hold up against SQL injections, using prepared statements. Is there any underlying issues with the structure of this method, though?

P.S. I am handling SELECT statements in a different way. This will handle DELETE and INSERT statements primarily.

The `if($expr != false){}` executes a return, if `$exp` is true, and you return something, and if not, you always return something. You don't need an `else{}` (meaning only `else{}` not what is inside) But with PDO is easier the whole thing. — Elias Nicolas, Oct 09 '15 at 03:56

score 1 · Answer 1 · edited May 23 '17 at 10:26

First of all, you're on the right track. Only few, may be one out of hundred PHP users even come to the idea of using such a useful function.

Next, Reflection is just superfluous for this task, call_user_func_array() is a way to go. Even though it's a little tricky, it's a straightforward way, while Reflection is not.

Finally, to the security. I would remove automatic type detection and bind all the parameters as strings. It won't do any harm, while with your current approach there is a chance that database will return you bizarre results, if you happen to compare a number with a string from database.

Error handling in this function is questionable too.

score 0 · Answer 2 · answered Oct 08 '15 at 02:48

There's nothing inherently insecure about using reflection here, but it's completely unnecessary. You can accomplish the exact same thing without reflection using call_user_func_array:

if ($expr !== FALSE) {
    call_user_func_array([$expr, "bind_param"], $values2);
    $expr->execute();
    ...

Alternatively, consider using PDO instead of mysqli. PDOStatement::execute() can take an array of bind parameters as an argument. (Also, using PDO means your code may be portable to databases other than MySQL!)

Is using PHP's Reflection Class to execute MySQL queries secure?

Background

TL;DR

The Method

2 Answers2