Is this PHP/MySQL delete function secure?

Question

I have a setup where I am deleting entries from a table.

It is based on the querystring of the URL which I'm thinking might be a bad way to start anyway.

So if the URL is:

http://www.example.com/delete.php?id=123&ref=abc

And the php in delete.php is as follows:

$id=$_GET['id'];
$ref=$_GET['ref'];

$con = mysql_connect("blahblah","user","password");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }

mysql_select_db("test", $con);

mysql_query("DELETE FROM mytable WHERE id=" . $id . " AND ref='" . $ref . "'");

mysql_close($con);

Is there a way to make this more secure... or is this indeed in any way secure at all??

EDIT:

OK, so based on the feedback I've taken a new approach.

list.php contains a set of radiobuttons for each entry in the table - as follows:

$con = mysql_connect("localhost","username","password");
if (!$con)
  {
  die('Could not connect: ' . mysql_error());
  }

mysql_select_db("db", $con);

$result = mysql_query("SELECT * FROM myTable");

echo "<form name='wer' id='wer' action='delete.php' method='post' >";

echo "<table border='1'>";

while($row = mysql_fetch_array($result))
  {
  echo "<tr>";
  echo "<td>" . $row['title'] . "</td>";
  echo "<td><input type='radio' name='test1' value='" . $row['id'] . "' /></td>";
  echo "</tr>";
  }
echo "</table>";
echo "<input type='submit' name='submit' value='Submit' />";
echo "</form>";

mysql_close($con);

And delete.php looks like this:

function check_input($value) {
    if (get_magic_quotes_gpc()) {
        $value = stripslashes($value);
    }
    if (!is_numeric($value)) {
        $value = "'" . mysql_real_escape_string($value) . "'";
    }
    return $value;
}

$con = mysql_connect("localhost","user","password");

if (!$con) {
    die('Could not connect: ' . mysql_error());
}

$varID = check_input($_POST["id"]);

mysql_select_db("db", $con);

$sql="DELETE FROM myTable WHERE id IN (" . $varID . ")";

if (!mysql_query($sql,$con)) {
    die('Error: ' . mysql_error());
}

mysql_close($con);

header("Location: list.php");

Is this a better way to go about it?

Answer 1

1. You have a SQL injection vulnerability since you don't sanitize the GET parameters you put into your query. The attacker can use that to delete all elements in your table.
   The clean solution to this is using prepared Statements.
   The quick and dirty solution is putting them in quotation marks and running them through mysql_real_escape_string.
2. Even if you fix that part, if the attacker can guess a valid id/ref pair he can delete that entry.
3. If a parameter is an integer, then why don't you make its type integer too? Something like $id=intval($_GET['id'])

Answer 2

GET is considered a safe method and should not have any side effects:

In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe".

In your case your script might be vulnerable to Cross-Site Request Forgery. You should better use POST instead and consider some kind of authentication and authorization check before deleting.

Additionally, since you use the passed parameters unaudited and unmodified, you are also vulnerable to SQL Injections.