PHP Database Class: Defending the data

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

PHP Database Class: Defending the data

Post by evstevemd »

Hi,
I'm new to this forum and I have a question.
I have written my class on top of PDO and uses PDOStatement (prepare/execute). Now what I'm asking is, what else to do, as far as database is concerned, to make my database very secure (assuming all other attacks in an app like XSS have been mitigated). Should I add escaping of strings before preparing it?
Thanks :D
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

Not having to escape data when using prepared statements is one of their major selling points. What you can do from now on for your DB security is never to use variables in the strings you prepare, you should only pass data in bind/execute.
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

Mordred wrote:Not having to escape data when using prepared statements is one of their major selling points.
Hi Mordred,
should I escape variables with PDO::quote? PHP Manual recommends I use the PDO::PreparedStatement and says it is better. Have I missed your point?
PHP Manual wrote:PDO::quote() places quotes around the input string (if required) and escapes special characters within the input string, using a quoting style appropriate to the underlying driver.

If you are using this function to build SQL statements, you are strongly recommended to use PDO::prepare() to prepare SQL statements with bound parameters instead of using PDO::quote() to interpolate user input into an SQL statement. Prepared statements with bound parameters are not only more portable, more convenient, immune to SQL injection, but are often much faster to execute than interpolated queries, as both the server and client side can cache a compiled form of the query.

Not all PDO drivers implement this method (notably PDO_ODBC). Consider using prepared statements instead.
Mordred wrote:What you can do from now on for your DB security is never to use variables in the strings you prepare, you should only pass data in bind/execute.
Thanks for comment. Here is sample function

Code: Select all

public function fetch_one($table, $id){
       //some code here       
       $stmt= $this->conn->prepare("SELECT * FROM $table WHERE $id_col[0]=:id");            
       $stmt->execute(array(":id"=>$id_val[0]));
       $this->resultset = $stmt->fetch(PDO::FETCH_ASSOC);    
    }
I will move the $table into execute! Thanks
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

evstevemd wrote:

Code: Select all

public function fetch_one($table, $id){
       //some code here       
       $stmt= $this->conn->prepare("SELECT * FROM $table WHERE $id_col[0]=:id");            
       $stmt->execute(array(":id"=>$id_val[0]));
       $this->resultset = $stmt->fetch(PDO::FETCH_ASSOC);    
    }
I will move the $table into execute! Thanks
I found that table name cannot be bound so I made a function to validate table name that it does only contain Aa-Zz 0-9 and characters -_

Code: Select all

 private function validate_table($table) {
        //validates that the table contains no character than A-Z 0-9  _- 
        $regex = '/^[a-zA-Z0-9_-]+$/';
        if (preg_match($regex, $table)) {
            return true;
        } else {
            return false;
        }
    }
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

evstevemd wrote:Mordred wrote:Not having to escape data when using prepared statements is one of their major selling points.

Hi Mordred,should I escape variables with PDO::quote? PHP Manual recommends I use the PDO::PreparedStatement and says it is better. Have I missed your point?
Nonono, you didn't parse my English (sorry): the point of prepared statements is that you don't have to escape data.

I found that table name cannot be bound ...
Aaaand you've found precisely what the problem with prepared statements is: they are a performance optimization and not a security measure. If you want dynamic names in a query you're back to square one in the minefield with the biggest boots the army could find for you: you have to step bloody carefully now .

Your situation is a bit different, since I don't think you should do any of the things you're doing -- not 100% sure since the code you pasted couldn't possibly work, but guessing your intentions here. I hope neither the table name nor the column name can possibly come from user input, since then you'll have a nice case of sql injection. Your validation function is not 100% true (check http://dev.mysql.com/doc/refman/5.1/en/identifiers.html) but it's not a security issue.
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

[0-9,a-z,A-Z$_]
Mordred wrote: Nonono, you didn't parse my English (sorry): the point of prepared statements is that you don't have to escape data.
Sorry, parser error ;)
Mordred wrote:Aaaand you've found precisely what the problem with prepared statements is: they are a performance optimization and not a security measure. If you want dynamic names in a query you're back to square one in the minefield with the biggest boots the army could find for you: you have to step bloody carefully now .
although it is dynamic query I use bind (with exception of a table). The Tables is checked before appended to query so there is no way for SQL injection through the variable.
Mordred wrote:Your situation is a bit different, since I don't think you should do any of the things you're doing -- not 100% sure since the code you pasted couldn't possibly work, but guessing your intentions here. I hope neither the table name nor the column name can possibly come from user input, since then you'll have a nice case of sql injection. Your validation function is not 100% true (check http://dev.mysql.com/doc/refman/5.1/en/identifiers.html) but it's not a security issue.
I have a self made PHP MVC framework I use and it uses this DB layer with models to fetch data from DB. So the Model is the one specifying table name and other necessary data. So it is not user who supply directly but perhaps indirectly. So a table is specified in each Model (blogg model have blog table, forum model have forum table et al)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

The most common situation in which you can have a vulnerable PDO code is when you want to have a sortable query, with the sort parameters given by the user.
Since you can't bind the column name to sort on, you need to include it dynamically in the query, and you need to take care to check if this is indeed a valid column, etc.
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

Mordred wrote:The most common situation in which you can have a vulnerable PDO code is when you want to have a sortable query, with the sort parameters given by the user.
Since you can't bind the column name to sort on, you need to include it dynamically in the query, and you need to take care to check if this is indeed a valid column, etc.
Ok, now back to my case: Since table variable is not a threat (comes from model not user and is well validated and restricted to a given characters) and all other variables are bound to preparestatement, is there anything more I can do to protect data?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

$id_col[0]?
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

Mordred wrote:$id_col[0]?
you caught me, red handed :crazy:
Ok, since this is column name, should limiting characters to [a-zA-Z0-9_$] be enough to prevent injection (since no quote is allowed) or should I take another measure?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

The correct way to handle user input as column names is to check agains a whitelist.
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

Mordred wrote:The correct way to handle user input as column names is to check agains a whitelist.
You mean regex like I do on table validation above?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

whitelist = a list (array) of possible permitted values.
Everything not in the whitelist is considered evil.
evstevemd
Forum Newbie
Posts: 11
Joined: Sat Jun 11, 2011 10:45 pm

Re: PHP Database Class: Defending the data

Post by evstevemd »

Mordred wrote:The correct way to handle user input as column names is to check agains a whitelist.
Hi Mordred,
Do you mean I fetch all columns in a table and make a white list? Wouldn't that reduce perfomance?
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: PHP Database Class: Defending the data

Post by Mordred »

You can just hardcode them in your PHP source.
Post Reply