views:

107

answers:

4

Hi,

I am using the get method to perform some operation like, approve, markasspam, delete, for commenting system. i know it is highly insecure to go this way but i cannot help it out. because the reason for using $_GET method is to perform the operation within the page itself using PHP_SELF, and FYI i am using the post method using checkbox to perform the operation too.

now for making it bit secure i want to randomize the number or generate the hash or something and then compare it, get the id and perform the operation

my current code is somewhat like this.

<?php 
if($approve == 1 ) 
{ 
    ?>
    <a href="<?php echo $_SERVER['PHP_SELF']."?approve=".$id; ?>">Unapprove</a>
    <?php 
} else 
{ 
    ?> 
    <a href="<?php echo $_SERVER['PHP_SELF']."?unapprove=".$id; ?>">Approve</a>
    <?php 
}
?> 
| <a href="<?php echo $_SERVER['PHP_SELF']."?spam=".$id; ?>">Spam</a> 
| <a class="edit-comments" href="edit-comments.php?id=<?php echo $id; ?>">Edit</a> 
| <a href="<?php echo $_SERVER['PHP_SELF']."?delete=".$id; ?>">Delete</a>

and i perform the operation using this code..

if(isset($_GET['approve'])) {
    $id = intval($_GET['approve']);
    $query = "UPDATE comments SET approve = '0' WHERE id = '$id'";
    $result = mysql_query($query);
}

if(isset($_GET['unapprove'])) {
    $id = intval($_GET['unapprove']);
    $query = "UPDATE comments SET approve = '1' WHERE id = '$id'";
    $result = mysql_query($query);
}

if(isset($_GET['delete'])) {
    $id = intval($_GET['delete']);
    $query = "DELETE FROM comments WHERE id = '$id'";
    $result = mysql_query($query);
}

if(isset($_GET['spam'])) {
    $id = intval($_GET['spam']);
    $query = "UPDATE comments SET spam = '1' WHERE id = '$id'";
    $result = mysql_query($query);
}

instead of using approve or unapprove or delete or spam, i want to randomize or hash that words and want it as lengthy as possible and then perform the operation.

how do i do it? what is your take on this?

EDIT: Please Note Only the Authenticated User i.e Admin will be able to perform this operation. even though it pass through authentication system i want to add more security even for admin. to avoid experiments or accident

the code is not exact it is just the sample to make you understand what i want to achieve.

+5  A: 

Whether you use GET or POST parameters here doesn't matter much in this context - what the script needs first is some sort of authentication. (After that is done, you can go into security details where GET is slightly less secure than POST - see the comments for details.)

I'd say you have two options:

  • Protecting the entire script using .htaccess - no changes needed to the script itself

  • Introducing PHP side user authentication and perform the operations only if a logged in user makes the request. Needs fundamental changes to the script but is most flexible.

Re your edit:

It turns out your script is already protected. In that case I assume you are uncomfortable with incremental ID numbers turning up in the URLs, getting cached in the browser etc. etc. The usual solution to that is to generate a random key for each comment when it is created (in addition to the incremental ID). That key gets stored in a separate column (don't forget to add an index) and you'd match against that.

A step even further would be to create temporary hashes for every action, which is the ultimate protection against a number of outside attacks.

Re your edit about using one-time hashes:

I've never implemented one-time hashes in an admin interface yet so I have no experience with this, but I imagine that a very simple implementation would store action hashes in a separate table with the columns hash, record and action. Whenever your tool lists a number of records and outputs "delete / approve / unapprove" links, it would generate three record in the hash table for each comment: One for delete, one for approve, one for unapprove. The "delete / approve /unapprove" links would then, instead of the record ID and command, get the correct hash as the only parameter.

Add a time-out function for unused hashes (plus delete any hashes that were actually used) and you're done.

Pekka
@Pekka FYI this operation is performed only by authenticated user only. i want to avoid accidents and experiment that is why i want to make it more secure and yes i will be using .htaccess , but for now i need to know within this script is there any way to generate hash and compare using $_GET and perform the operation.
Ibrahim Azhar Armar
@Ibrahim What's stopping an unauthenticated user from typing in the URL `page.php?delete=1`?
NullUserException
POST is MUCH MORE secure, when we are talking about XSS attacks on your admin interface.
Anton N
@Anton How does POST stop XSS?
NullUserException
@Ibrahim I added a small update, but in that case you should elaborate on what you want some more.
Pekka
Using `$_GET` *is* less secure. Only idempotent operations should be accessible via GET, any operation that's actually modifying data should use POST requests. Just imagine Google or a similar spider accidentally getting ahold of your admin page...
deceze
@Pekka yes you got me right, i want something like temporary hashes rather then asking the script to check from the database. how do i make use of temporary hash in this?
Ibrahim Azhar Armar
@NullUserException - browsers send cross-site GET requests with ease. Sending cross-site POST request without user specific actions is nearly impossible (please, correct me if i'm wrong)
Anton N
@Anton You are right, but that's called CSRF (see my answer). XSS is a different vulnerability.
NullUserException
@Ibrahim I added my 2 cents on it
Pekka
@Pekka i got the logic behind your suggestion. i would like you to know that the admin interface will only be accessed from one place with authentication system. is it still insecure to go ahead with the code i am using.? is it vulnerable to hacker or any kinds of bots?
Ibrahim Azhar Armar
@Ibrahim If the system is used only by you or very few people, and it is protected using .htaccess authentication, I would say that will do fine. The CSRF issues we are talking about here (and which action hashes are a measure against) are necessary for applications that are used by many, or widely distributed, but a bit of overkill for a personal blog.
Pekka
If your friends have some very unharmful plugin, that just checks-in all links on page for future faster loading - then oops. All actions will be executed for all entries.
Anton N
@quantumSoup what I was referring to is the perception that POST requests are inherently more secure because the data is not visible. The situation when I answered this was that of an entirely unprotected script. I'll rephrase to reflect that.
Pekka
@Anton yeeeaah, but how high is the risk of that for a personal, closed-source comments administration system? Let's get some perspective here.
Pekka
@Pekka - sorry, i've just changed my comment to make it more real situation for small sites :) . Anyway - bad design, that assumes, that your app is not popular leads you to failure sooner or later.
Anton N
@Anton true. Even though you don't implement the most advanced stuff straight away, it is in any case commendable to think about security at an early stage, +1 for the asker and the question
Pekka
@Pekka thank you for the feedback, appreciation and of-course that debating. it helped me gain enormous knowledge . :)
Ibrahim Azhar Armar
I guess i will remove the delete operation so that my data is not destroyed. and i will be depending on POST to do the delete operation for me.
Ibrahim Azhar Armar
+1  A: 

You can do it that way, the $_GET is not the unsecure thing in your code. The unsecurity comes from you not checking wether the user is e.g. authorized to delete comments.

In your current code, anyone can delete anything at anytime and as often as they want.

If you have a wrapping code that ensures the if-statements postet by you are not executed if enter good reason here, then it's okay.

But you should try verifying, that the content of the parameters are really integers instead of just int_val'ing them and using them directly on the database.

On your edit

You should check your parameter is really an int. intval("test") will also return an integer, mostly 0.

You might consider regex for that, to verify the string only consists of numbers: preg_match('/[0-9]+/', $_GET['id']);

If so, you can perform the action.

ApoY2k
sorry i forgot to mention i have given the user authentication only the admin will be able to perform the operation.
Ibrahim Azhar Armar
"`$_GET` is not the unsecure thing in your code" -1
quantumSoup
@quantumSoup This question has evolved. At the time it was asked, the script in question seemed to give entirely unprotected access to all administration functions, in which case it really doesn't matter whether GET or POST is used. Downvoting answers when the question's context has changed entirely is a bit unfair - a simple comment would suffice.
Pekka
A: 

As deceze mentioned, GET requests should always be idempotent; eg: they should never change server state. GET should be used for queries only. You should go with POST on any operations that affect data on the server. This will save your page from havoc if it gets crawled by Google or what not.

Additionally, using GET (even with authentication) leaves your site vulnerable to cross-site request forgery (CSRF) attacks. Read the wikipedia page on ways to prevent that.

NullUserException
A: 

You shouldn't use GET for any operations that change data on server. NEVER. You use it only to get data.

If you can't use forms for operation buttons (because there is another form outside them) you should consider this design:

  • You use AJAX to perform POST requests to your server
  • In javascript-disabled environments you use GET links like user.php?action=delete, which shows you very simple form on a separate page. The header in the form asks: "Are you sure you want to delete user X?" and it has two buttons: 1) "Yes" - that submits POST request to operation script, 2) "No" - which sends user back to the page where he has been
Anton N
is there any JQuery plugin for that?
Ibrahim Azhar Armar
it's in JQuery without any plugins. Read docs in $.post() or $.ajax()
Anton N