Is my DB script secure?

[jamesmpollard]

Been reading around and seen many people with outdated and depreciated DB wrappers, so I was wondering if mine is one of them? If so, is there anything I can do to change that. I want to be php7 ready and right on the front-line of it too!

 

<?php /** * Database management and access class * This is a very basic level of abstraction */if (!defined('ND')) {    exit('Direct access to this file is not permitted.');} $database_name = 'db'; class database {     private $connections = array();    private $activeConnection = 0;    private $queryCache = array();    private $dataCache = array();    private $last;    private $QuerySTR;     public function __construct() {            }     /**     * Create a new database connection     * @param String database hostname     * @param String database username     * @param String database password     * @param String database we are using     * @return int the id of the new connection     */    public function newConnection($host, $user, $password, $database) {        $this->connections[] = new mysqli($host, $user, $password, $database);        $connection_id = count($this->connections) - 1;        if (mysqli_connect_errno()) {            trigger_error('Error connecting to host. ' . $this->connections[$connection_id]->connect_error, E_USER_ERROR);        }         return $connection_id;    }     /**     * Close the active connection     * @return void     */    public function closeConnection() {        $this->connections[$this->activeConnection]->close();    }        /**     * Change which database connection is actively used for the next operation     * @param int the new connection id     * @return void     */    public function setActiveConnection(int $new) {        $this->activeConnection = $new;    }     /**     * Store a query in the query cache for processing later     * @param String the query string     * @return the pointed to the query in the cache     */    public function cacheQuery($queryStr) {        if (!$result = $this->connections[$this->activeConnection]->query($queryStr)) {            trigger_error('Error executing and caching query: ' . $this->connections[$this->activeConnection]->error, E_USER_ERROR);            return -1;        } else {            $this->queryCache[] = $result;            return count($this->queryCache) - 1;        }    }     /**     * Get the number of rows from the cache     * @param int the query cache pointer     * @return int the number of rows     */    public function numRowsFromCache($cache_id) {        return $this->queryCache[$cache_id]->num_rows;    }     /**     * Get the rows from a cached query     * @param int the query cache pointer     * @return array the row     */    public function resultsFromCache($cache_id) {        return $this->queryCache[$cache_id]->fetch_array(MYSQLI_ASSOC);    }        public function SQLCacheResultCount($CID) {        return $this->queryCache[$CID]->field_count;    }     /**     * Store some data in a cache for later     * @param array the data     * @return int the pointed to the array in the data cache     */    public function cacheData($data) {        $this->dataCache[] = $data;        return count($this->dataCache) - 1;    }        public function DataCacheResultCount() {        return count($this->dataCache);    }     /**     * Get data from the data cache     * @param int data cache pointed     * @return array the data     */    public function dataFromCache($cache_id) {        return $this->dataCache[$cache_id];    }     /**     * Delete records from the database     * @param String the table to remove rows from     * @param String the condition for which rows are to be removed     * @param int the number of rows to be removed     * @return void     */    public function deleteRecords($table, $condition, $limit) {        $limit = ( $limit == '' ) ? '' : ' LIMIT ' . $limit;        $delete = "DELETE FROM {$table} WHERE {$condition} {$limit}";        $this->executeQuery($delete);    }     /**     * Update records in the database     * @param String the table     * @param array of changes field => value     * @param String the condition     * @return bool     */    public function updateRecords($table, $changes, $condition) {        $update = "UPDATE " . $table . " SET ";                foreach ($changes as $field => $value) {            $update .= "`" . $field . "`='{$value}',";        }         $update = substr($update, 0, -1);        if ($condition != '') {            $update .= "WHERE " . $condition;        }         $this->executeQuery($update);         return true;    }     /**     * Insert records into the database     * @param String the database table     * @param array data to insert field => value     * @return bool     */    public function insertRecords($table, $data) {        // setup some variables for fields and values        $fields = "";        $values = "";         // populate them        foreach ($data as $f => $v) {             $f = $this->sanitizeData($f);            $fields .= "`$f`,";             $v = $this->sanitizeData($v);            $values .= ( is_numeric($v) && ( intval($v) == $v ) ) ? $v . "," : "'$v',";        }         // remove our trailing ,        $fields = substr($fields, 0, -1);        // remove our trailing ,        $values = substr($values, 0, -1);         $insert = "INSERT INTO $table ({$fields}) VALUES({$values})";        $this->executeQuery($insert);        return true;    }     public function selectRecords($table, $selection = '*', array $where = null, $limit = null, $orderby = null, $groupby = null, $order = null) {        $q = 'SELECT ';         if (is_array($selection)) {            foreach ($selection as $SK) {                $q .= $SK . ', ';            }             $q = substr($q, 0, (strlen($q) - 2));        } else if (is_string($selection)) {            $q .= $selection;        } else {            return false;        }         $q .= ' FROM `' . $table . '`';         if (!is_null($where) && is_array($where)) {            $q .= ' WHERE ';             foreach ($where as $WK => $WV) {                $q .= $WK . ' = \'' . $this->sanitizeData($WV) . '\', ';            }             $q = substr($q, 0, (strlen($q) - 2));        }         if (!is_null($groupby)) {            $q .= ' GROUP BY `' . $groupby . '`';        }         if (!is_null($orderby)) {            $q .= ' ORDER BY `' . $orderby . '`';             if (!is_null($order)) {                $q .= ' ' . $order;            } else {                $q .= ' ASC';            }        }         if (!is_null($limit) && is_numeric($limit)) {            $q .= ' LIMIT ' . $limit;        }         $this->executeQuery($q);        return $this->getRows();    }     /**     * Execute a query string     * @param String the query     * @return void     */    public function executeQuery($queryStr) {        $this->QuerySTR = $queryStr;        if (!$result = $this->connections[$this->activeConnection]->query($queryStr)) {            trigger_error('Error executing query: ' . $this->connections[$this->activeConnection]->error, E_USER_ERROR);        } else {            $this->last = $result;        }    }     /**     * Get the rows from the most recently executed query, excluding cached queries     * @return array      */    public function getRows() {        if (method_exists('mysqli_result', 'fetch_all')) {            $results = $this->last->fetch_all(MYSQLI_ASSOC);        } else {            for ($results = array(); $tmp = $this->last->fetch_array(MYSQLI_ASSOC)                 $results[] = $tmp;        }         return $results;    }     /**     * Gets the number of affected rows from the previous query     * @return int the number of affected rows     */    public function affectedRows() {        return $this->connections[$this->activeConnection]->affected_rows;    }     /**     * Sanitize data     * @param String the data to be sanitized     * @return String the sanitized data     */    public function sanitizeData($data) {        return $this->connections[$this->activeConnection]->real_escape_string($data);    }     /**     * Deconstruct the object     * close all of the database connections     */    public function __deconstruct() {        foreach ($this->connections as $connection) {            $connection->close();        }    } } ?>

 

Many thanks

James

[QuickOldCar]

Yours will be ok.

The mysql_* functions are deprecated and will be removed in php7, you are using mysqli_*.

 

If you wanted to can use PDO instead and also prepared statements.

[jamesmpollard]

Yours will be ok.

The mysql_* functions are deprecated and will be removed in php7, you are using mysqli_*.

 

If you wanted to can use PDO instead and also prepared statements.

Excellent. Thank you for getting back to me. I was considering it, but it would mean a whole re-write wouldn't it?

 

Is there anything I could do to the existing script that could beef up security or even performance?

[QuickOldCar]

Your code is pretty basic, not much to optimize, php7 is supposed to be faster, we'll see about that.

You can use prepared statements in mysqli as well.

 

Did you happen to create indexes in mysql on any WHERE/AND/OR values will be using? That could speed the queries up.

 

Can check what you need and see slow queries.

 

https://dev.mysql.com/doc/refman/5.7/en/slow-query-log.html

show status like 'Slow_queries';

show variables like '%slow_query%';

 

EXPLAIN

ANALYZE TABLE

[Jacques1]

This is not secure. At all.

 

There's not even a consistent approach to security: Sometimes you do escape the input values^[1], sometimes you just stuff them right into the query string^[2]. Sometimes you do try to escape identifiers^[3] (using the wrong procedure^[4]), sometimes you just stuff them right into the query string^[5]. And then you append entire strings to your query with no checks whatsoever^[6].

 

So this is full of SQL injection vulnerabilities and won't even withstand the most basic attacks. Unfortunately, the code cannot really be fixed either, because a lot of problems are on a conceptual level. For example, if you accept an entire WHERE clause through a parameter, you just can't make it secure (unless you're willing to implement a full-blown SQL parser). And escaping identifiers is incredibly hard. The developers of Doctrine, one of the most sophisticated database abstraction layers in PHP, eventually gave up. Do you think you can do better?

 

This begs the question if the class even makes sense: You'll need a lot more knowledge, attention and effort to get this halfway secure, and then your users again need a lot of knowledge, attention and effort to avoid the inevitable gaps. For what? A bit of syntax sugar? Meh ...

 

I understand that database abstraction layers look like a good idea, but implementing them correctly is incredibly hard, and using them correctly is even harder. It's virtually impossible to create real abstraction, so everybody ends up with some half-assed query builder for expert users who know exactly what they can and cannot do with that.

 

Personally, I'd stick to good old mysqli_query().

 

 

 

[1] Example: $v in insertRecords()

[2] Example: $value in updateRecords()

[3] Example: $f in insertRecords()

[4] mysqli_real_escape_string() is strictly for escaping MySQL string literals (hence the name). You cannot escape identifiers with it.

[5] Actually, most identifiers are entirely unprotected. For example, $table in all query methods or $WK in selectRecords().

[6] Example: $condition in deleteRecords() or $selection in selectRecords().

[seandisanti]

Excellent. Thank you for getting back to me. I was considering it, but it would mean a whole re-write wouldn't it?

 

Is there anything I could do to the existing script that could beef up security or even performance?

 

Personally I'd say to switch to PDO since you're going to be doing a fair bit of rewriting anyway. You don't really even need a wrapper to be honest, but making use of prepared statements and bound parameters, as well as the quote function could definitely help tighten things up a bit. I always suggest that people interested in security improvements take a trip to http://pentesteracademy.com or at least http://securitytube.net Both sites are slightly more geared towards offense than defense, but the best way to learn how to defend your site is to know how to properly attack it.

[benanamen]

The funny thing about people talking about their app security is that their server itself is almost always insecure. I mean just basic stuff like being vulnerable to click-jacking, exposing php and apache version, not setting XSS protection etc. 99.9999% of all websites I check can be click-jacked and it is a simple 2 second fix.

 

+1 on using PDO