Database Security

[KnottyAlder]

I am creating a class to control my database access, and I am wondering what people think the security risks of the following code to be.

<?php
    function DBManager($host, $user, $pass, $db){
        $startTime = $this->getMicroTime();
        
        // Try to make a connection to the server
        if (!$this->connection = @mysql_connect($host,$user,$pass,true)){
            $this->errorCode = mysql_errno();
            $this->errorMsg  = mysql_error();
            return false;
        }
        
        // Now select the database
        if (!@mysql_select_db($db,$this->connection)){
            $this->errorCode = mysql_errno();
            $this->errorMsg  = mysql_error();
            @mysql_close($this->connection);
            return false;
        }

        $this->totalTime += $this->getMicroTime() - $startTime;
            
        return true;
    }
?> 

 

Usually I store my database connection script outside my public_html folder, but I will not be able to do so in this case.

 

Does anyone have any thoughts or perhaps a better way of coding this?

[Daniel0]

How would it be a security risk in respect to the database?

[mystery_man]

It is a very good idea to have a Class that provides an abstraction layer for your database functions.

 

I always use one when working on any dynamic site, here are the two main pro's:

1. It provides all the commonly used database functions you would need

2. It is generic, for MS SQL or other databases, just change the insides of the functions, and you have abstraction for easy reuse

 

If you have one for MySQL, PostGre, SQLite, MS SQL etc, your application is only working with the 'Database' class, and it cares not what goes on under the hood of it. It makes it super easy to port apps to other databases.

 

Here is the idea of what I use.

 

"//@" explains my code.

 

<?php


//=======MySQL Connection details============

//@ these can be kept in a config file, especially if there is plenty of other 'config' data
$host = "localhost";
$database = "dbname";
$user = "username";
$password = "password";

//@ keep your different settings at hand while in development - it saves me some time.
/* 
====Local Settings
$host = "localhost";
$database = "dbname";
$user = "username";
$password = "password";

====development server settings
$host = "localhost";
$database = "dbname";
$user = "username";
$password = "password";

====production server settings
$host = "localhost";
$database = "dbname";
$user = "username";
$password = "password";

*/
//=====================


//Database Constants
defined('DB_HOST') 		? null : define("DB_HOST", $host);
defined('DB_NAME') 		? null : define("DB_NAME", $database);
defined('DB_USER') 		? null : define("DB_USER", $user);
defined('DB_PASS') 		? null : define("DB_PASS", $password);


class MySQLDatabase{

	//attributes
	private $connection;
	public $last_query;
	private $magic_quotes_active;
	private $real_escape_string_exists;

public function __construct(){

	//@ open the connection at construction, so it is available from the start
	$this->open_connection();
	$this->magic_quotes_active = get_magic_quotes_gpc();
	$this->real_escape_string_exists = function_exists( "mysql_real_escape_string" );
}

public function open_connection(){
	$this->connection = mysql_connect(DB_HOST, DB_USER, DB_PASS); //open connection
	if(!$this->connection){
	die("Error opening MySQL Database connection: ". mysql_error());
	}else{
		$db_select = mysql_select_db(DB_NAME, $this->connection); //select database
		if (!$db_select){
			die("Database selection failed: " . mysql_error());
		}
	}
}

public function close_connection(){
		if(isset($this->connection)){
	mysql_close($this->connection);
	unset($this->connection);
	}
}

//@ to do a simple query, and return the resultset
public function query($sql){

	$this->last_query = $sql; //set last query
	$result = mysql_query($sql, $this->connection);
	  
		if (!$result) { //check for error

			//output the last query - usefull for debugging
			$output = "Last Query ".$this->last_query."<br>";
			$output .= "Database query failed: " . mysql_error() . "<br /><br />";
			die( $output );
		}		
	return $result;		
}

//@ this is some rather tricky, it just returns the escaped paramater
public function escape_value($value) {

	if($this->real_escape_string_exists) { 
		// undo any magic quote effects so mysql_real_escape_string can do the work. PHP v4.3.0 or higher
		if( $this->magic_quotes_active ){
		$value = stripslashes( $value ); 
		}
		$value = mysql_real_escape_string($value);
	} 
	else{
		// if magic quotes aren't already on then add slashes manually, before PHP v4.3.0
		if( !$this->magic_quotes_active ){
			$value = addslashes( $value );
		}
		// if magic quotes are active, then the slashes already exist
	}
	return $value;
}

//database neutral methods
public function fetch_array($result){
	 return mysql_fetch_array($result);
  	}

public function num_rows($result){
	return mysql_num_rows($result);
}

public function get_last_id(){
	return mysql_insert_id($this->connection); // get the last id inserted over the current db connection
}

public function affected_rows(){
	return mysql_affected_rows($this->connection);
}

public function insert_id() {
    // get the last id inserted over the current db connection
    return mysql_insert_id($this->connection);
  }

}

//@ make the $database and $db variables available for use.
$database = new MySQLDatabase();
$db =& $database;
?>

 

This approach becomes really effective when using an OOP design, in which you have clas modelling each of your data objects, which each in turn has a table. You can build the models inteligently so that they 'know how to' construct, update and create themselves (in and from the their tables in the DB) by making use of the database class.

 

It works for me!

 

Regards

 

[mystery_man]

Oh I got carried away... security. Just make sure all your data going into your database are escaped. If you use 'model' classes for your data objects (eg users, cars, photos, whatever) knowing how to update/create themselves using, for example, $user->save(); or $user->create(); , escape all the attributes in the create/save functions prior to insertion.

[Daniel0]

1. It provides all the commonly used database functions you would need

 

So does PHP's standard library.

 

2. It is generic, for MS SQL or other databases, just change the insides of the functions, and you have abstraction for easy reuse

 

That's somewhat of a pipe dream. Migrating to another database system is unlikely to be the trivial task you make it sound like. That is unless you restrict yourself to the very basic UPDATE/DELETE/INSERT statements of course.

[mystery_man]

mmm... pipe dream indeed - just burst my bubble  :'( I should have said:

 

1. It groups all of the commonly used db functions you commonly use together in one place

 

2. It is reasonably generic, and provides a bit of abstraction between databases as far as simple functionality is concerned

[Daniel0]

http://php.net/pdo

[corbin]

Hate to seem like we're picking on you, but I mean this advice in a helpful way.

 

 

Instead of using constants to store your DB config, it would be better idea to pass the config to the database constructor.  That way the database config is in the scope of the database connection, and the better reason is that then you could have multiple instances with unique configurations.