[RESOLVED] Securing Some Php Code

[Moneyman2010mn]

I've been creating a script for a friend and was wondering whether or not this was perfectly secure. At another website they said it was but I'd like the opinions of a bunch of people. Here's the code.

<?php
	// Connect to database.
	$connection = mysql_connect("localhost", "***", "***") or die(mysql_error());
	$select_db = mysql_select_db("***") or die(mysql_error());

// Check for POST variables and if they do not exist, create them using a switch statement.
$sort = $_POST['sort'];
$order = $_POST['order'];
switch ($sort) {
    case "name":
    	$sort = name;
    	break;
    case "type":
    	$sort = type;
    	break;
    case "tileset":
    	$sort = tileset;
    	break;
    default:
    	$sort = name;
    	break;
}
switch ($order) {
	case "ASC":
		$order = ASC;
		break;
	case "DESC":
		$order = DESC;
		break;
}

// Show display options for growable buildings.
echo "<form action='index.php' method='POST'>";
echo "<b>Organize results by</b> <select name='sort'><option value='name'>Name</option><option value='type'>Developer Type</option><option value='tileset'>Tileset</option></select> ";
echo "<b>in</b> <select name='order'><option value='ASC'>Ascending Order</option><option value='DESC'>Descending Order</option></select> ";
echo "<input type='submit' value='Go!' />";
echo "</form>";

// Get growable buildings and display them in alphabetical order.
$sql = "SELECT *
    FROM growable_buildings
    ORDER BY $sort $order"; 
$sql_result = mysql_query($sql, $connection) or die(mysql());
echo "<table border='1'>";
echo "<tr><th>Name</th><th>Type</th><th>Tileset</th><th>Stage</th><th>Capacity</th></tr>";
while ($row = mysql_fetch_array($sql_result)) {
	$id = $row['id'];
	$name = $row['name'];
	$type = $row['type'];
	$tileset = $row['tileset'];
	$stage = $row['stage'];
	$capacity_normal = $row['capacity_normal'];
	echo "<tr><td><a href='viewbuilding.php?id=$id'>$name</a></td><td>$type</td><td>$tileset</td><td>$stage<td>$capacity_normal</td></tr>";
}

mysql_close();
?>

[bradgrafelman]

The values for $sort and $order should be strings, not constants, and you should check that these were actually POST'ed (using a function like [man]isset/man, for example) before you even try to use them. In fact, I wouldn't even use switch() statements at all. You could combine both switch statements as well as the two variable assignments into something like this:

$sort = ((isset($_POST['sort']) && in_array($_POST['sort'], array('name', 'type', 'tileset'))) ? $_POST['sort'] : 'name');
$order = ((isset($_POST['order']) && in_array(strtoupper($_POST['order']), array('ASC', 'DESC'))) ? strtoupper($_POST['order']) : '');

Also, I would recommend changing the "SELECT " statement to a list of column names that you use... SELECT usually wastes resources as it returns data that you don't use.

[MarkR]

Run it with error_reporting (E_ALL) and you will see some notices relating to what brad says.

Don't use bare words as strings. Don't use uninitialised variables.

Personally, I feel that "SELECT *" is actually ok in most cases (assuming, for instance, there are no huge blobs in the table).

Mark

[bradgrafelman]

MarkR wrote:

Personally, I feel that "SELECT " is actually ok in most cases (assuming, for instance, there are no huge blobs in the table).

For the average table, this is most likely true. But, I try to live by the "Better safe than sorry" philosophy... that, plus consideration given to scalability, have pushed me away from using the "SELECT " query.

[Moneyman2010mn]

Thanks. It worked perfectly fine. 🙂

Topic Resolved.