[the1corrupted]

Hello! I have a web application that I'm hard-coding from scratch. I've used no one else's code, and I'm beginning to be concerned about security. I have read that using PDO and mysql prepared statements are the most secure way to go, but I seem to never implement it correctly. Currently, my code uses the old procedural method, but if I want to do PDO, I would have to do it in a procedural syntax or convert my main function library to an OOP class library.

Right now, I'm familiar with the mysqli procedure (which is so much more amazing than the old mysql). For example, I use mysqli_real_escape_string() quite frequently. However, I seem to be unable to exact a bound param query.

$stmt=mysqli_prepare($link, "SELECT `qty` FROM `usr_inv` WHERE `inv_id`=? AND `usr_id`=?");
mysqli_stmt_bind_param($stmt, "ii", $inv_id, $usr_id);
mysqli_stmt_execute($stmt);

What would this look like with PDO? How would PDO be implemented correctly? Say I want to connect to the database ONCE and disconnect when I'm done with everything. How would that work? Could someone give me a quick run-down on how that would be implemented?

This post has been edited by the1corrupted: 23 April 2012 - 07:07 PM

[CTphpnwb]

Here's one way you could do it with PDO:

$dsn = "mysql:host=localhost;dbname=yourdatabasename";
$username = "databaseusername";
$password = "databasepassword";
$pdo = new PDO($dsn, $username, $password);
$query = "SELECT `qty` FROM `usr_inv` WHERE `inv_id`=? AND `usr_id`=?";
$stmt = $pdo->prepare($query);
$params = array($inv_id,$usr_id);
$stmt->execute($params);
foreach($stmt as $row) {
	echo $row['qty']."<br>";
}

Check out this tutorial for more info:
http://www.dreaminco...duction-to-pdo/

[the1corrupted]

Actually, just as I was reading this, I had a break through in formatting my code to use a more object-oriented approach, and I gotta say it's pretty darn cool. Since I like defining my own log files, and have a globally accessible method to do database queries and other necessary functions, I have my home-brew core.php. Now I get to call it core2.php for version 2.0!

I've done the following with no return on errors (after reviewing the OOP and PDO manual more carefully). I've successfully created backtrace for my program, and I can execute simple queries with a prepared statement. Just out of curiosity though, would there be any security fixes on my Core class? Is this structured right? Though it functions, I always feel like that DSN is a jumbled mess. (I'd go on to fix my main functions, making Library extend Core or something to that effect)

I also have to say: I love PDO.

class Core {
	public static $_PDO;
	public static $_DEBUG=true;
	function __construct() {
		try {
			self::$_PDO=new PDO("mysql:dbname=mjosephm_game;host=127.0.0.1", "*****", "*****");
		} catch (PDOException $e) {
			die("DB Connection failed.<br />".$e->getMessage());
		}
		if (self::$_DEBUG) {
			self::append_log("Database connection instantiated.");
		}
	}

	public function append_log($msg) {
		$file="C:\\wamp\\www\\maelstrom\\.logs\\".date("Y-m-d")." Runtime.txt";
		if (file_exists($file)) {
			$mode="a";
		} else {
			$mode="w";
		}
		$log=fopen($file, $mode);
		if ($mode=="w") {
			fwrite($log, "BACKTRACE LOGGING BEGIN: ".date("Y-m-d H:i:s")."\r\n--------------------------------------------");
		}
		fwrite($log, "\r\n".date("H:i:s")." ".$msg);
		fclose($log);
	}

	public static function terminate($msg) {
		self::append_log("-----------------------------------\r\n  Process Terminated.\r\n\t".$msg."\r\n--------------------------------------------");
		die("<span class=\"error\">Crash detected and logged.</span>");
	}

	public static function query_do($sql, $params) {
		$stmt=self::$_PDO->prepare($sql);
		if (!$stmt) {
			self::terminate("Reason: Prepared Statement Failed!\r\n\t".$_PDO->errorInfo());
		}
		$stmt->execute($params);
	}
}

$core=new Core;
$sql="SELECT `user` FROM `users` WHERE `id`=?";
$params=array(1);
$core->query_do($sql, $params);

This post has been edited by Jstall: 24 April 2012 - 04:45 AM
Reason for edit:: Removed database credentials

[CTphpnwb]

At a glance it looks ok, but I think I'd pass a reference to a PDO object to the constructor instead of creating the connection within it. That way you could have a connection.php file that holds the dsn, user name, and password so you don't have to worry about changing that information when moving from test and live servers.

[the1corrupted]

Actually, I just made my test server mimic the settings of the remote server... This is the only project I'm working on for now, and if I ever get hired to code, I'll change it around. So you can pass a pdo object as a __construct input param? How extensible are objects? Are they like multidimensional arrays with functions (methods) they can execute just by looking a little funny?

[Dormilich]

CTphpnwb, on 24 April 2012 - 04:24 AM, said:

Here's one way you could do it with PDO:

$dsn = "mysql:host=localhost;dbname=yourdatabasename";
$username = "databaseusername";
$password = "databasepassword";
$pdo = new PDO($dsn, $username, $password);
$query = "SELECT `qty` FROM `usr_inv` WHERE `inv_id`=? AND `usr_id`=?";
$stmt = $pdo->prepare($query);
$params = array($inv_id,$usr_id);
$stmt->execute($params);
foreach($stmt as $row) {
	echo $row['qty']."<br>";
}

not quite exact as the parameters are integers. here you would need an explicit bind.

$dsn = "mysql:host=localhost;dbname=yourdatabasename";
$username = "databaseusername";
$password = "databasepassword";
$pdo = new PDO($dsn, $username, $password);
$query = "SELECT `qty` FROM `usr_inv` WHERE `inv_id`=? AND `usr_id`=?";
$stmt = $pdo->prepare($query);
$stmt->bindValue(1, $inv_id, PDO::PARAM_INT);
$stmt->bindValue(2, $usr_id, PDO::PARAM_INT);
$stmt->execute();
foreach($stmt as $row) {
	echo $row['qty']."<br>";
}

[the1corrupted]

I've run into issues when I use paramaters like that. The ints in my DB are unsigned, so I find myself using float. But beyond that, I have a central statement constructor that gets an undefined number of params of varying types. They could be strings, or intergers. Is an explicit bind far more secure than simply passing the array of parameters through the execute statement?

If so, how would I apply it to my statement builder and query executer that's at the heart of all my queries? (Because I'm super lazy) I'd rather right Core::query_do($sql, $params) rather than go through the list of either bindValue or even type the word "execute" when I want to exact a query. Not to mention, it makes it super simple to change how everything does a generic query.

private static function prep_stmt($sql) {
	$stmt=self::$_PDO->prepare($sql);
	if (!$stmt) {
		self::terminate("Reason: Prepared Statement Failed!\r\n\t".$_PDO->errorInfo());
	}
	return $stmt;
}
public static function query_do($sql, $params) {
	$stmt=self::prep_stmt($sql, $params);
	$stmt->execute($params);
}

Would I add this to my query_do method?

if (!is_array($params)) {
	self::terminate("Reason: Params expected as array, ".gettype($params)." given.");
} else {
	for ($x=0; $x<count($params); $x++) {
		switch(gettype($params[$x])) {
		case "interger":
			$stmt->bindValue(($x+1), $params[$x], PDO::PARAM_INT);
			break;
		# Etc.....
		}
	}
}

EDIT: And I don't know why, but whenever I type self::terminate, I always think it's funny.

This post has been edited by the1corrupted: 24 April 2012 - 05:48 AM

[CTphpnwb]

More secure? You're binding data, so none of it will be executed regardless of type. Other than helping with debugging I'm not sure what good declaring type is going to do. It could help with speed I suppose, but then again it might slow things down by giving the interpreter more to do.

[Dormilich]

the1corrupted, on 24 April 2012 - 02:44 PM, said:

Is an explicit bind far more secure than simply passing the array of parameters through the execute statement?

CTphpnwb, on 24 April 2012 - 03:13 PM, said:

You're binding data, so none of it will be executed regardless of type. Other than helping with debugging I'm not sure what good declaring type is going to do.

about lazy parameters, the Manual says (PDOStatement->execute()):

Quote

An array of values with as many elements as there are bound parameters in the SQL statement being executed. All values are treated as PDO::PARAM_STR.

if you can afford to pass integers as strings, so be it. but what if you have NULL or boolean values?

[CTphpnwb]

Hadn't thought of SQL's issues with it. Of course that's the same problem with a vanilla MySQL query where you have:

... WHERE id='".$id."'";

If there is no id that is zero (such as if id is auto incrementing) then you should be fine since you'll get no result, as you should. — Hmmm, turns out you get no result if there is an id of zero too. I suppose that's right as well since you shouldn't get a result with an invalid input.

[the1corrupted]

With the number of times I have to query the database, the interpreter can't be too heavy. It's a db-central application so the less clunky/faster I can make my query interpreter, the better.

Oh, and because I use unsigned INTs in the DB, what would happen if PDO got a float? I have to use floats to express the maximum number the unsigned INT can store (otherwise, I get about half the max). For example, I have to float the value: 4954967315. How would PDO handle this number?

[CTphpnwb]

I would test it and see. Then if it worked the way I expected on my test server I'd want to test it on the live server too, just in case there's a difference. If you're going to have that many IDs then you'd better know how it will behave.

[the1corrupted]

My functions appear to be accepting the float values just fine. Here's my little test method (which ironically, I would've wound up writing anyway)

I wonder though, if type-matching my variables is actually making these functions more or less secure or if its just headroom that's completely redundant.

I type-set all the user info when I call it in from the database, so it's not a string array. Floats are floats, ints are ints, strings are strings, etc.

public static function subtract_player_money($money, $usr_id=false) {
		if (Core::$_DEBUG) {
			$list=func_get_args();
			$num=func_num_args();
			$args="";
			for ($x=0; $x<$num; $x++) {
				$args.="\r\n\t(".gettype($list[$x]).") ".$list[$x];
			}
			Core::append_log("FUNCTION: ".__FUNCTION__."()\r\n  PARAMS: ".$args);
		}
		if (!is_float($money)) {
			Core::terminate("Float expected, ".gettype($money)." given.\r\n\tFUNCTION: ".__FUNCTION__);
		} else if (!is_float($usr_id) AND !is_bool($usr_id)) {
			Core::terminate("Float or boolean expected, ".gettype($usr_id)." given.\r\n\tFUNCTION: ".__FUNCTION__);
		}
		if ($usr_id) {
			$sql="SELECT `copper` FROM `users` WHERE `id`=?";
			$params=array($usr_id);
			$is_row=Core::is_row($sql, $params);
			if (!$is_row) {
				Core::terminate("User id does not exist on `users` table!\r\n\tID: ".$usr_id);
			}
			$data=Core::fetch_data($sql, $params);
		} else {
			$usr_id=$_SESSION['uid'];
			$data['copper']=self::$_PLAYER['copper'];
		}
		if ($data['copper']-$money<0) {
			Core::terminate("Net quantity less than zero.\r\n\t".$data['copper']." - ".$money." < 0");
		}
		$sql="UPDATE `users` SET `copper`=? WHERE `id`=?";
		$params=array(($data['copper']-$money), $usr_id);
		Core::query_do($sql, $params);
	}

[CTphpnwb]

Type matching has nothing to do with security. The old style mysql_* functions are insecure because they mix data with MySQL execution code. Prepared statements separate the two, eliminating that vulnerability.

[the1corrupted]

So what other vulnerabilities might I face now? Are there any? Should I instead focus my attention on data I get via _GET and _POST? Maybe type-match it so it becomes compatible with the DB while throwing out the bad stuff?

I know to be absolutely paranoid of ALL data you do not directly control. For example, if it's not a number, I like to throw it out. This snippet, I wrote, sniffs out if I got bad data when I want a number better than if I tried to push it through a function like int/floatval() and I can't test it with is_float or is_int because a variable with _POST will always be a string. Doesn't hurt to look for _GET either.

$test_float="500; SELECT * WHERE 1=1";
$compare=preg_replace("/[^0-9]/", "x", $test_float);
if ($test_float==$compare) {
	echo floatval($test_float)." == ".$compare;
} else {
	echo floatval($test_float)." != ".$compare;
}