Small security question

[fortnox007]

HI all,

 

I have a book with some nice examples, but often i wonder if they are that secure for displaying and using outside the production area.

 

One of them is this.

A form is created by using a while loop that gets data(email addresses) from a database and shows them with check boxes. after that someone can select the e-mailaddress they don't like and delete them from the database.

here is some code:

<?php
        //....
        $result = mysqli_query($dbc,$query);

        while ($row = mysqli_fetch_array($result)){
            echo '<input type="checkbox" value"'.$row['id'].'"name="todelete[]"/>';
            echo $row['firstname'];
        }

        //.........deleting part
        if (isset($_POST['submit'])){
        foreach($_POST['todelete'] as $delete_id){
            
            $query = "DELETE FROM email_list WHERE ID = $delete_id";
                mysqli_query ($dbc, $query)
                        or die ('error querying databse');
                       
            }
        }
        //....
?>

 

I have two questions:

-> is this a smart way of deleting stuff? since you are going to use multiple queries instead of 1 in the for each loop.

-> besides not using mysqli_real_escape_string, isn't this application allowing someone to alter the POST-array (todelete) to any value he likes? At least that's what i think can happen. If anyone knows a nice way to do this more secure , I would love to here it, because i don't really trust the html array created.

 

Thanks in advance!

 

[the182guy]

1) Yes there is a more efficient way of deleting multiple records from a table by using the IN clause. Although just deleting a few items as you are in that script is no problem.

 

DELETE from stuff WHERE id IN (1, 9, 33, 44)

 

2) You don't even need mysql_real_escape_string() here, because the ID's are integers and cannot contain any letters or punctuation you can just case them as int's. That will force the value as a number (if it was a valid number in the first place), or if not it will force it to 0. Example

 

foreach($_POST['todelete'] as $delete_id){
     $id = (int)$delete_id;

    // $id is now safe to put in the query, you could check it is > 0 before adding it though

[fortnox007]

Thanks allot for your clear and swift answers.

I just knew there would be a smarter and efficient way.

 

One small question though. The concern I had about the possibility of someone being able to alter the number in the input box is that a valid concern or is it just me being to panicky. Because i thought if you make create an array in html by using:  name ="todelete[]"

someone could very well, alter the number ones the checkbox is displayed. Sould I maybe do this with a session array or something else?

[KevinM1]

-> besides not using mysqli_real_escape_string, isn't this application allowing someone to alter the POST-array (todelete) to any value he likes? At least that's what i think can happen. If anyone knows a nice way to do this more secure , I would love to here it, because i don't really trust the html array created.

 

Thanks in advance!

 

 

Well, mysql_real_escape_string isn't an issue here since these ids aren't strings.

 

Your other point is correct, however.  If left in the wild like this, anyone would be able to delete these records simply by sending integers to the script.  That's why we have administration areas - dangerous bits of script like this are hidden behind some form of access control to keep the barbarians out.

[fortnox007]

Yes i realise, that this script is not barbarian proof and should be kept in a vault with raging monkeys with AK-47's around it  , But in case I Would have something like a Poll, I assume they use this technique too.  (ofc. for updating and not deleting).

Let me put it in a different way. What would be a good way to make sure altering the html var, would not have effect on the result. Having that said isn't it better to use something different than this html var?

 

-edit:  hmm maybe by testing before executing the query if the id is in the array?

[rwwd]

My understanding is that the $_POST array is technically a string, therefore you need a way of checking that the received data is numerical, preg_match would be the best way of securing an answer to that; but there are other methods that don't need the regexp patterns, but to be honest regexp or even to a certain extent - typecasting would be the most efficient way of doing that.

 

Rw 

[Andy-H]

-> is this a smart way of deleting stuff? since you are going to use multiple queries instead of 1 in the for each loop.

<?php
        //....
        $result = mysqli_query($dbc,$query);
        $valid = array();

        while ($row = mysqli_fetch_array($result)){
            echo '<input type="checkbox" value"'.$row['id'].'" name="todelete[]"/>';
            echo $row['firstname'];
            
            $valid[] = $row['id'];
        }

        //.........deleting part
        if (isset($_POST['submit'])){
            if ( empty(array_diff($valid, $_POST['todelete'])) )
            {

               $ids = "( " . explode(', ', $_POST['todelete']) . " )";
               $query = "DELETE FROM email_list WHERE ID IN " . $ids;
                   mysqli_query ($dbc, $query)
                           or die ('error querying databse');
            }
            else
            {
                die("Editing postdata isn't clever!");
            }
        }
        //....
?>

 

That would be more efficient.

 

//Edit:

 

Added security against editing form values.

[fortnox007]

ty ty That looks awesome! and I will really use it. Also everyone thanks for helping.

 

The only question I still have and maybe i just can't explain what i am thinking of is this.

Say my query limits the number of rows to display like rows 1 till 10. The query that is used doesn't check in the end if the value was in the previous "limited range of results".

so in case

I have a table with a lot of rows and only show rows 1 till 10 in frontend. someone could well change the number and despite validating and stuff delete row 76.

|  ID        |      email                |

---------------------------------------

|  0          |  lalala@jsjas.com  |

|  1          |  jdadha@hsa.org  |

| .......      |  .........................  |

|  76        |  valuable@abc.org|  <---- someone could delete this despite validating and stuff.

----------------------------------------

 

So I would like to know how to make sure the stuff returned from the input is what i first show. That's the only thing i am still searching for. If someone knows

 

- edit, if someone knows I love to hear it if not i am gonna  find out and post it : )

 

-edit2, hmm I just checkout the array_diff function, maybe you allready gave the answer to the above XD. I am going to check it out. Anyways sorry if i am a bit noob

[Andy-H]

The code I posted above does exactly that,

<?php
        //....
        $result = mysqli_query($dbc,$query);
        $valid = array(); //create an empty array

        while ($row = mysqli_fetch_array($result)){
            echo '<input type="checkbox" value"'.$row['id'].'" name="todelete[]"/>';
            echo $row['firstname'];
            
            $valid[] = $row['id']; //populate it with each id we pull from sql and display.
        }

        //.........deleting part
        if (isset($_POST['submit'])){
            //verify that the posted id's don't differ from the $valid array populated with the correct values
            //using array_diff and checking if the returned array is empty.
            if ( empty(array_diff($valid, $_POST['todelete'])) )
            {
               //create a bracket-enclosed, comma-seperated string of the posted id's.
               $ids = "( " . explode(', ', $_POST['todelete']) . " )";
               //use the in clause to delete rows with an id 'IN' the comma-seperated list.
               $query = "DELETE FROM email_list WHERE ID IN " . $ids;
                   mysqli_query ($dbc, $query)
                           or die ('error querying databse');
            }
            else
            {
                /*
                 * if the array returned wasn't empty kill script and tell the offender that you
                 * know whatthey're up to
                 */
                die("Editing postdata isn't clever!");
            }
        }
        //....
?>

[fortnox007]

The code I posted above does exactly that,

<?php
        //....
        $result = mysqli_query($dbc,$query);
        $valid = array(); //create an empty array

        while ($row = mysqli_fetch_array($result)){
            echo '<input type="checkbox" value"'.$row['id'].'" name="todelete[]"/>';
            echo $row['firstname'];
            
            $valid[] = $row['id']; //populate it with each id we pull from sql and display.
        }

        //.........deleting part
        if (isset($_POST['submit'])){
            //verify that the posted id's don't differ from the $valid array populated with the correct values
            //using array_diff and checking if the returned array is empty.
            if ( empty(array_diff($valid, $_POST['todelete'])) )
            {
               //create a bracket-enclosed, comma-seperated string of the posted id's.
               $ids = "( " . explode(', ', $_POST['todelete']) . " )";
               //use the in clause to delete rows with an id 'IN' the comma-seperated list.
               $query = "DELETE FROM email_list WHERE ID IN " . $ids;
                   mysqli_query ($dbc, $query)
                           or die ('error querying databse');
            }
            else
            {
                /*
                 * if the array returned wasn't empty kill script and tell the offender that you
                 * know whatthey're up to
                 */
                die("Editing postdata isn't clever!");
            }
        }
        //....
?>

 

Yes i just found out, you set  $valid[] = $row['id']; as reference. Sorry i never worked with that awesome function. They should build you a statue!!, and all of the above

[rwwd]

if ( empty(array_diff($valid, $_POST['todelete'])) )

 

The word we are after gentlemen is "Nifty", I like this, though I doubt it's bullet proof. For now though, apart from a fresh cool beer from me fridge, that'll do the trick.

 

Have fun.

Rw