tags:

views:

172

answers:

5

My profile.php displays all the user's postings,comments,pictures. If the user wants to delete, it sends the posting's id to the remove.php so it's like remove.php?action=removeposting&posting_id=2. If they want to remove a picture, it's remove.php?action=removepicture&picture_id=1.

Using the get data, I do a query to the database to display the info they want to delete and if they want to delete it, they click "yes". So the data is deleted via $POST NOT $GET to prevent cross-site request forgery.

My question is how do I make sure the GETs are not some javascript code, sql injection that will mess me up.

here is my remove.php

    //how do I make $action safe? 
    //should I use mysqli_real_escape_string?
    //use strip_tags()?
    $action=trim($_GET['action']);

    if (($action != 'removeposting') && ($action != 'removefriend') 
    && ($action != 'removecomment'))
    {
            header("Location: index.php");
            exit();
    }

if ($action == 'removeposting')
{
   //get the info and display it in a form. if user clicks "yes", deletes
}

if ($action =='removepicture')
{
   //remove pic
}

I know I can't be 100% safe, but what are some common defenses I can use.

EDIT

Do this to prevent xss
$oldaction=trim($_GET['action']);
$action=strip_tags($oldaction);

Then when I am 'recalling' the data back via POST, I would use 
$posting_id = mysqli_real_escape_string($dbc, trim($_POST['posting_id']));

        if ($action == 'removeposting')
{
    //get the posting id from the user  
    $getposting_id = htmlspecialchars(trim($_GET['posting_id']));

    //basic checks for the posting id
    if (empty($getposting_id)){
        //header ("Location: index.php");
        echo '<p>Sorry, no posting was specified for removal.</p>';
        exit();
    }

    if (!is_numeric($getposting_id))
    {   
        echo "Not an integer";
        exit();
    }
        //Also have check to see if the posting_id is the user's. If so, can delete
A: 

To prevent against any SQL injection you should use mysqli_real_escape_string OR you can use Prepared Statements that accomplish the same thing.

To prevent any javascript, you could use strip_tags in concert with htmlspecialchars.

webdestroya
okay thanks! I used those!
ggfan
@ggfan actually using strip_tags in concert with htmlspecialchars is quite stupid. there is nothing to strip after htmlspecialchars and contrary. one of these is enough.
Col. Shrapnel
and mysql(i)_real_escape_string is not enough to protect SQL
Col. Shrapnel
Which is why I suggested prepared statements. And HSC+StripTags works if you strip certain things (or to catch things like `'<'+'/script>'`
webdestroya
MRES is no less secure than prepareds, just if you know how to use it. but withouth full set of rules it is useless to mention at all. as for the strip-tags, it is still useless combined with HSC. **Why** to strip certain tags if you gonna convert `<>` into entities anyway?
Col. Shrapnel
A: 

I know I can't be 100% safe

ahahahaha :)

if your question regarding only action, and it is used only to determine the code to run, it IS safe by any means.

But some info for you:
POST doesn't prevent cross-site request forgery. unique token does.
You didn't post actual action code, so just to be sure - I hope you check user's id whan perform these actions

Col. Shrapnel
O that's a relief! Tho I didn't mention, I also happen to be passing in the posting_id and to check that I make sure if logged in, is their post.
ggfan
@ggfan you desperately need to *understand* what these injections and XSS are, not just to ask for some code. Without understanding you'll always fail.
Col. Shrapnel
yup, still trying to get my way through...it's only been 5 months since I know how to write hello in PHP :)
ggfan
@ggfan look. XSS works only when you print some date into browser. do you print $action? So, how XSS can be? SQL injection works only if you put the data into query. Do you put your action in SQL? If not - HOW SQL injection can be? Is it too hard to see such obvious things?
Col. Shrapnel
I don't print $action or the $getposting_id. I use the $getposting_id to get info out of the dbc, so guess need mysqli_real_espace_string there. I delete from the dbc, but is that an issue for sql injection?
ggfan
omg @ggfan you were asking for $action, not for $getposting_id. So, I am not telling what to do. I am tellig you what to THINK. As for the $getposting_id I have told you already: is_numeric is enough because number can contain no XSS nor sql code. As its only number. digits.
Col. Shrapnel
+5  A: 

Because you aren't storing $action and only using it in your conditional, it's not necessary to do all the trimming/stripping/escaping. The simple string comparisons is enough in terms of "safety," though I recommend using === instead of ==.

Alternatively, if you were storing a $_GET or $_POST value into an integer column of a MySQL database, for example, you could simply pass the value into intval() before storing it in the database. If you need to store plain text, just pass it through mysql_real_escape_string() before storing it. You can also use preg_match() or preg_replace() to make sure you are only storing valid values (different patterns for different uses, e.g. /^\d{5}(?:-?\d{4})?$/ for zip codes).

Kevin
actually, the last part - data validation - shouldn't be mentioned in a security context. it is different worlds and it is very important not to mix it. the rest of the answer is perfect. I'd just add that escaped strings must be enclosed in quotes. that's obvious, yes, but to be sure. Just because *_real_escape_string() works only in quoted strings, by design
Col. Shrapnel
`$_GET` and `$_POST` values are strings, so they should pass through `mysql_real_escape_string()` just fine.
Kevin
not always. there can be arrays too. But I am talking of strings anyway. Just failed to explain it well. I just want to add that `strings in the query must be enclosed in quotes`. That's obvious, yes, but deserved to mention. Because *_real_escape_string() will work only when resulting string enclosed in quotes in the query. Otherwise it won't help at all. And contrary, if you enclose string in quotes but don't use *_real_escape_string(), it won't help either. Because these 2 rules will work only together
Col. Shrapnel
A: 

$_GET is completely safe until you try to glue (concatenate) it to the other things in your program like:

  • SQL to be executed (in which case you should use mysqli_real_escape_string() or another escaping technique that is advised with the database that you are using)
  • HTML to be outputted (in which case you should pass it through htmlspecialchars() before concatenation or printing/echoing)
  • URL to be navigated to by the user (in which case you most likely should pass it through urlencode())
  • JavaScript to be executed by the browser (in which case you should pass it through json_encode())

If you want to have an url in javascript embedded in html that you want to be inserted in database then you should pass value that you receive from $_GET (also $_POST, $_COOKIE and other similar outside sources) through all those functions in order appropriate to this scenario, namely

mysql_real_escape_string(htmlspecialchars(json_encode(urlencode($_GET['val']))));

If the only thing you do is compare your $_GET['action'] to some strings then you are perfectly safe without doing any escaping at all.

Kamil Szot
A: 

Use the mysqli_real_escape_string as noted elsewhere. Also, of course this does nothing to prevent XSRF. You'll need tokens containing some entropy for that.

See also: http://www.owasp.org/index.php/Main_Page

Full Decent