PHP deleting users from db keeps deleting everyone!

[black-ops]

Anyone can tell me where I am going wrong here =/

<?php	
	 if(!isset($cmd)){
		 $result = mysql_query("SELECT * FROM members WHERE username != '$myUsername' AND isAdmin = '0'");
			 while($row = mysql_fetch_array($result)){
					$id = $row['id'];
					$name = $row['username'];
							?>		
								<tr><td><? echo $row['username']; echo(" (<i> " . $row['status'] . "</i>)"); ?></td>
									<td>
										<a href="<?php echo "users.php?cmd=del&id=$id&user=$name" ?>"><img src="img/icons/action_add.gif" width="16" height="16" alt="add user"></img></a>
										<a href="#"><img src="img/icons/search.gif" width="16" height="16" alt="search users"></img></a>
										<a href="#"><img src="img/icons/file.gif" width="16" height="16" alt="back up user db"></img></a>
										<a href="#"><img src="img/icons/action_delete.gif" width="16" height="16" alt="delete user"></img></a>
										<a href="#"><img src="img/icons/application.gif" width="16" height="16" alt="mod rights"></img></a>
										<a href="#"><img src="img/icons/user.gif" width="16" height="16" alt="user profile"></img></a>
									</td>
								</tr>
							<?	
		//			}	
			 }   
		}
?>

<?
if($_GET["cmd"]=="del"){
$sql = "DELETE FROM members WHERE id = $id and username = $name";
$result = mysql_query($sql);
$row = null;
header("location: users.php");
}
?>

 

[Craig321]

<?
if($_GET["cmd"]=="del"){
$sql = "DELETE FROM members WHERE id = $id";
$result = mysql_query($sql);
$row = null;
header("location: users.php");
}
?>

Try that

 

[Swanster]

I think you need to change:

<?php     
     if(!isset($cmd)){

to:

<?php     
     if(!isset($_GET["cmd"])){

and:

$sql = "DELETE FROM members WHERE id = $id and username = $name";

to:

$sql = "DELETE FROM members WHERE id = $_GET["$id"] and username = $_GET["$name"]";

I've not really done much in PHP, but what I *think* is happening is when you click the link to delete a user, the the top section of the code still runs, so $id and $name are set to the last user returned, then the bottom part deletes that last user.

I suspect that the page is then somehow being reloaded, with cmd=del still in the query string, so the last user gets deleted again, and so on until all are gone. But I'm not really sure what's happening - I can't see how the redirect would work if a table has already been output? I assume there's some crucial code missing that explains this - as I can see the db connection isn't there for starters.

That's my theory for why it's deleting all users anyway; I'm sure someone good with PHP will be along soon who will have a better idea. I just can't see how else the SQL query could result in more than one user being deleted (other than SQL injection attacks - which the code appears to be vulnerable to), unless it's run more than once.

Also, assuming id is unique in the database (can't imagine it wouldn't be!), then as Craig321 pointed out, there's no need for $name in delete query,

 

[Ruddles]

OK, quick couple of notes on SQL injection (before we begin, I haven't used PHP or mysql in about 6 years so my syntax might be slightly off, however the general info is all gravy).

1) Escape anything that you're sending to the database with something like mysql_real_escape_string (http://php.net/manual/en/function.mysql-real-escape-string.php), else someone nasty will come along and delete everyone off your database. I'll show you how in a sec.

2) Never change the state of your system with a "GET", it makes attacks so much easier. If you change the state, use a form and use POST.


To attack your current system, I would use something like the following URL:
users.php?cmd=del&id=''%20or%201=1--

Here's how this would work. Here is your SQL:

DELETE FROM members WHERE id = $id and username = $name

If you were deleting "dave", with ID 1 your code would produce:

DELETE FROM members WHERE id = 1 and username = dave

However, if you use URL above you'll get:

DELETE FROM members WHERE id = '' or 1=1-- and username =

-- is a comment, so eveything after it ("and username = ") is ignored. Now the query reads "Delete from members where the username is empty or 1=1". 1=1 is always true so every row will be deleted.

 

[Craig321]

Well, SQL injection and security is a whole other story, which the OP will hopefully read into before making anything public/live.

You also mustn't forget checks such as (in this case):
- Checking to make sure $id is in fact a number
- Checking to make sure $id isset(), or !empty()

Be creative. Run as many checks as you can that will help and you'll be fine (don't get stupid though else it'll just slow the code down in the long run).

I also recommend you read this page created by robmiller: http://php.robm.me.uk/

Also, another few functions written by robmiller which I've included below - they really help make avoiding injection attacks easy:

function remove_magic_quotes($array) {
    foreach ($array as $k => $v) {
        if (is_array($v)) {
            $array[$k] = remove_magic_quotes($v);
        } else {
            $array[$k] = stripslashes($v);
        }
    }
    return $array;
}
if (get_magic_quotes_gpc()) {
    $_GET    = remove_magic_quotes($_GET);
    $_POST   = remove_magic_quotes($_POST);
    $_COOKIE = remove_magic_quotes($_COOKIE);
}

function quote_smart($value)
{
    // Stripslashes
    if (get_magic_quotes_gpc()) {
	    $value = stripslashes($value);
    }
    // Quote if not integer
    if (!is_numeric($value)) {
	    $value = "'" . mysql_real_escape_string($value) . "'";
    }
    return $value;
}

Then do:

$sql_cookie = sprintf("SELECT * FROM blah WHERE username = %s", quote_smart($id));

Oh, and black-ops, get into the habit of indenting, dammit!