Security issues in PHP/Sql

Question

I'm a beginner. I wrote this and I have been told my find() method had security vulnerabilty. I thougt that the if else of my queryType method prevented from Sql injections attempts.

    public function queryType(string $sql, array $attributes = null)
    {
        ## instance of Database singleton
        $this->Database = Database::getInstance();

        if($attributes !== null) {
            ## if attr, prepared request
            $query = $this->Database->prepare($sql);
            $query->execute($attributes);
            return $query;
        }else{
            ## else, simple request
            return $this->Database->query($sql);
        }
    }

    ## SECURITY ISSUE /!\ ?? queryType if/else doesn't prevent ??

    public function find(int $id)
    {
        return $this->queryType("SELECT * FROM {$this->table} WHERE id = $id")->fetch();
    }

How can I solve it ?

your `queryType` method is, this allows a raw query to be executed in there, and your `find` doesn't utilize placeholders, albeit with a type hint, it's not being used properly. and be careful with `{$this->table}`, if this is user supplied, you'll need to whitelist it with your tables or just define this — Kevin, Jul 21 '22 at 14:31

IMSoP · Answer 1 · 2022-07-21T13:50:04.197

Code only provides security if you actually use it. Because you're not passing any parameters, the query shown will fall into the "else, simple request" case, so the fact that queryType supports parameters is irrelevant. It's like fitting a heavy iron bolt to your door, but never sliding it into place - it won't keep anyone out.

In this case, $id is guaranteed to be an int, so the risk is minimal, but you should get in the habit of always using parameters: $this->queryType("SELECT * FROM {$this->table} WHERE id = ?", [$id])

However, you have another dynamic part which can't be a parameter: $this->table If there is any way for the user to choose that value, it would be a huge security vulnerability. If this is for some kind of "ORM" where you have a class for each table, it would probably be sensible to use a class constant instead (e.g. $tableName = static::TABLE_NAME;); that way, sub-classes can define a value, but it will never be calculated at run-time, so you can't accidentally put a user-controlled value into it.

I've written elsewhere that frameworks protect against SQL injection like a toothbrush protects against dental cavities. — Bill Karwin, Jul 28 '22 at 18:06

Security issues in PHP/Sql

1 Answers1