2

Currently i have a working PHP edit script, which allows users to edit ads they have posted, but I have realized that users can modify the ?id= number to bring up another set of data then edit someone else data and save it in the database.

Is there any way I can make it so that when the user clicks on their advert they have posted to edit, it is only their own ads that they access to, that they wont be able to edit other peoples ads by adjusting the id?= and a way of protecting the form from manipulation?

Here is my current code:

<?php
/* 
EDIT.PHP
Allows user to edit specific entry in database
*/

// creates the edit record form
// since this form is used multiple times in this file, I have made it a function that is      easily reusable
function renderForm($id, $fname, $lname, $contact, $price, $error)
{
?>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<title>Edit Record</title>
 <link rel="stylesheet" type="text/css" href="stylesheet.css">

 <style type="text/css">

#page-wrap                  {
position:absolute;
top: 206px;
left: 288px;
width: 50%;
text-align:left;
background-color:#FFF;
padding: 10px;
border-radius: 10px;
box-shadow: 1px 2px 2px #888888;
     }
     
    </style>
    <script type = "text/javascript">

    function myfunction(url)
    {
    window.location.href = url;
    }
   </script>


</head>
<body>

  <div class="container">

  <div id="imagelogo" onclick = "window.location.href = 'index.html'" > 

  <p> Buy and sell stuff around University</p>
   </div>

   <ul id="navigation" name="navigation">
  <li id="nav-home"><a href="index.html">Home</a></li>
  <li id="nav-search"><a href="search.php">Search</a></li>
  <li id="nav-selling"><a href="#">Selling</a></li>
  <li id="nav-buying"><a href="#">Buying</a></li>
   <li id="nav-FAQ"><a href="#">FAQ</a></li>
  <li id="nav-contact"><a href="#">Contact</a></li>

  <p>&nbsp;</p>
  <p>&nbsp;</p>
  <p>&nbsp;</p>
  <p>Sponsors</p>
  </ul>
  <div id="account">

  <?php
  if( isset( $_SESSION['username'] ) ){

  echo "<a href='securedpage1.php'>My Account</a><img src='images/uni-icon.png' width='30'        height='18' style='vertical-align: middle;'/>";

  }else{

 echo "<a href='login.php' >Login</a><img src='images/uni-icon.png' width='30' height='18'    style='vertical-align: middle;'/>";
}
?>

</div>

<div id="registerlogout">
<?php
if( isset( $_SESSION['username'] ) ){
echo "<a href='logout.php'>Logout</a>";

}else{

echo "<a href='register.php'> Register</a>";
}
?>
</div>

<div id="social">
<img src="images/fb-logo.png" width="22" height="20" />     

 <img src="images/twitter-logo.png" width="24" height="25" />
  </div>

 <div id="page-wrap">
 <?php 
 // if there are any errors, display them
 if ($error != '')
 {
 echo '<div style="padding:4px; border:1px solid red; color:red;">'.$error.'</div>';
 }
 ?> 

 <form action="" method="post">
 <input type="hidden" name="id" value="<?php echo $id; ?>"/>
 <div>
 <strong>Ad Title: *</strong> <input type="text" name="fname" style="width: 60%; box-    sizing: border-box; -moz-box-sizing: border-box; -webkit-box-sizing: border-box;"value="<?php      echo $fname; ?>"/><br/>
  <strong>Description: *</strong> <textarea name="lname" cols="45" rows="5"><?php echo     $lname; ?></textarea><br/>
 <strong>Contact*</strong> <input type="text" name="contact"  style="width: 60%; box-    sizing: border-box; -moz-box-sizing: border-box; -webkit-box-sizing: border-box;" value="<?php     echo $contact; ?>"/><br/>
<strong>Price*</strong> <input type="text" name="price"  style="width: 60%; box-sizing:    border-box; -moz-box-sizing: border-box; -webkit-box-sizing: border-box;" value="<?php echo    $price; ?>"/><br/>
 <p>* Required</p>
 <input type="submit" name="submit" value="Submit">
 </div>
 </form>
 </div>
 </div>
 </body>
 </html> 
 <?php
  }

// Inialize session
    session_start();


 // connect to the database
 include('conn.php');

 // check if the form has been submitted. If it has, process the form and save it to the    database
 if (isset($_POST['submit']))
  { 
 // confirm that the 'id' value is a valid integer before getting the form data
 if (is_numeric($_POST['id']))
 {
 // get form data, making sure it is valid
 $id = $_POST['id'];
 $fname = mysql_real_escape_string(htmlspecialchars($_POST['fname']));
 $lname = mysql_real_escape_string(htmlspecialchars($_POST['lname']));
 $contact = mysql_real_escape_string(htmlspecialchars($_POST['contact']));
 $price = mysql_real_escape_string(htmlspecialchars($_POST['price']));

 // check that firstname/lastname fields are both filled in
 if ($fname == '' || $lname == '' || $contact == '' || $price == '' )
 {
 // generate error message
 $error = 'ERROR: Please fill in all required fields!';

 //error, display form
 renderForm($id, $fname, $lname, $contact, $price, $error);
 }
else
 {
 // save the data to the database
 mysql_query("UPDATE people SET price='$price', contact='$contact', fname='$fname',      lname='$lname' WHERE id='$id'")
 or die(mysql_error()); 

 // once saved, redirect back to the view page
 header("Location: view.php"); 
 }
 }
 else
 {
 // if the 'id' isn't valid, display an error
 echo 'Error!';
 }
 }
 else
 // if the form hasn't been submitted, get the data from the db and display the form
 {

 // get the 'id' value from the URL (if it exists), making sure that it is valid (checing   that it is numeric/larger than 0)
 if (isset($_GET['id']) && is_numeric($_GET['id']) && $_GET['id'] > 0)
 {
 // query db
 $id = $_GET['id'];
 $result = mysql_query("SELECT * FROM people WHERE id=$id")
 or die(mysql_error()); 
$row = mysql_fetch_array($result);

 // check that the 'id' matches up with a row in the databse
 if($row)
 {

 // get data from db
 $fname = $row['fname'];
 $lname = $row['lname'];
 $contact = $row['contact'];
 $price = $row['price'];

 // show form
 renderForm($id, $fname, $lname, $contact, $price, '');
 }
 else
 // if no match, display result
 {
 echo "No results!";
 }
 }
 else
 // if the 'id' in the URL isn't valid, or if there is no 'id' value, display an error
 {
 echo 'Error!';
 }
 }
 ?>
user
  • 5,370
  • 8
  • 47
  • 75
neeko
  • 1,930
  • 8
  • 44
  • 67

5 Answers5

3

You need to record, in the database, the poster of each advert. This is just another column.

When an attempt is made to edit an advert (either for displaying the form or saving the result) you need to check that the owner of the advert matches the currently logged in user.

e.g. UPDATE adverts SET text=? WHERE id=? AND user=?

Quentin
  • 914,110
  • 126
  • 1,211
  • 1,335
  • thankyou for your quick reply! i have added this to my query but it doesnt seem to work $username = $_SESSION['username']; mysql_query("UPDATE people SET price='$price', contact='$contact', fname='$fname', lname='$lname' WHERE id='$id' and username='$username' ") or die(mysql_error()); – neeko Sep 06 '12 at 22:09
  • the issue with this is you must be using sessions. – Osman Sep 06 '12 at 22:24
  • You don't /have/ to use sessions (although they are convenient) but you do have to authenticate the user. – Quentin Sep 06 '12 at 22:25
  • Could you explain how i would check that the current user matches the user associated with the advert? (the username is collected when a user posts an ad) is it better to use another method than sessions? – neeko Sep 06 '12 at 22:27
  • You need some way to identify the user (e.g. a username and password) and some way to authorise the user (e.g. storing the username in a column in the adverts table). You could collect the username and password at the same time as you get the updated data for the advert. Or you could collect it in advance, give the user a time limited token to prove they are the same person who just gave you the credentials and then store the token and username somewhere (which is what a session does). A session is a perfectly find way to do it, especially if you are already handling logins with one. – Quentin Sep 06 '12 at 22:31
  • okay my current solution should be okay then as the user logs in to their account using user name and password, they then can view their ads only in their account, and when they edit i have included the WHERE username=$username, to check that the username in session matches the username in the column of the ads table, i have checked and now it wont let a user edit other peoples ads using the id! so thank you so much!! – neeko Sep 06 '12 at 22:38
  • I do have another question though, i am very new to all this and i would like to know how easy it is for someone to manipulate my site maliciously , for example for this form here could you give me some pointers on how i can improve security? many thanks again – neeko Sep 06 '12 at 22:39
  • dont use mysql when doing database calls, use a method called pdo. This prevents injection by sending the query and the data separately to the server then putting it all together. see: http://stackoverflow.com/questions/13569/mysqli-or-pdo-what-are-the-pros-and-cons – Osman Sep 06 '12 at 22:48
1

Set a session when they log in. Check if the session-username is the same as the username that is linked to the post they want to edit. If true, they can edit.

yoeriboven
  • 3,541
  • 3
  • 25
  • 40
0

I suggest you query the database to check that the id the user is requesting is a id that he/she is allowed to access.

Erik
  • 2,276
  • 1
  • 20
  • 20
0

Keep it server side, store the id in a database, and call that number, this will stop them from being able to edit it.

0

md5 the id number code to each account and add that to the query. Make sure that the code matches the one associated with the account (so md5 the id and make sure it matches the one in the database) then add the stuff. This way no one can change the number and edit other accounts posts. The md5 algorithm is specific to your sever and not predictable.

$hash = md5( $id );

use this to create the code and associate this with the account and use it like the id in addition to the id. This means when you create the account you need to create an md5 version of the id as a field in the database next to the id.

Change this like:

mysql_query("UPDATE people SET price='$price', contact='$contact', fname='$fname', lname='$lname' WHERE id='$id'") or die(mysql_error());

to

mysql_query("UPDATE people SET price='$price', contact='$contact', fname='$fname', lname='$lname' WHERE id='$id' and idCode='$hash'") or die(mysql_error());

Just make sure you have a field in the database called idCode because the md5 is an encryption that is not reversible.

Osman
  • 1,771
  • 4
  • 24
  • 47
  • thankyou for your reply, this sounds like a good solution! would the idCode hash have to be stored in the database when the ad is created? and how possible is it that some one crack the md5 hash and start editing other peoples accounts? – neeko Sep 06 '12 at 22:14
  • I have decided to make sure the user in session can only access the entries with their username attached, but i am considering adding an md5 hash just to be on the safe side, thanks! – neeko Sep 06 '12 at 22:20
  • cracking the md5 is very difficult an unless you have gold behind it I doubt anyone will try to hack it. Also you would have to store the md5 version of the id in the database where the id is stored in a field next to it because as i said the md5 is not reversible see this : http://php.net/manual/en/function.md5.php – Osman Sep 06 '12 at 22:20
  • It seems that you’re not familiar with [Kerckhoff’s principle](http://en.wikipedia.org/wiki/Kerckhoffs%27s_principle). – Gumbo Sep 06 '12 at 22:27
  • the brute force method would have to try a crap load of combinations with every id because the id is part of the query. Also he was concerned the people would change the id in the query, brute force is obviously not a concern of such a low level of initial security, this is not a login. The md5 is just another layer a hacker would have to work around, I never said secure your site with it? – Osman Sep 06 '12 at 22:32
  • @user860869 MD5 is quite fast and with todays home computers you can [hash about 335 MiB/s](http://www.cryptopp.com/benchmarks-amd64.html). If you now want to generate the MD5 hashes of all integers from 0 to 2^32-1, that would be 2^32·32 bits = 16 GiB of data that you can hash in 48.91 seconds. – Gumbo Sep 06 '12 at 22:42
  • again like i said this is not a login, his concern was people editing the simple id number in a url which would result in them editing another users information. Using md5 addresses this issue. because the id becomes unreadable. Obviously the asker has has more issues than brute force if he is using id to verify accounts when updating things in the database. – Osman Sep 06 '12 at 22:45
  • sorry im new to this, so say if i checked that the username is session matches with the username associated with the ad in the database to prevent them editing the id and gaining access to other users info (which is now working) and hashed the id at the some time, would this be a decent level of security? please could you explain what you mean by "the asker has has more issues than brute force if he is using id to verify accounts when updating things in the database." I am keen to listen and learn from any one trying to help out – neeko Sep 06 '12 at 22:54
  • I am not sure of the level of security you are trying to get, how important is it that no one edits other people information? If you are storing private information then it is critical and should only be done in the most secure of ways and you can never have too many methods to protect that information. However is you are just concerned your silly user might change the id number in the request then I assumed you just want to prevent something that simple and are not concerned about big picture security. – Osman Sep 06 '12 at 23:00
  • the advert data isn't private information, i just dont want a user to be able to access someone elses ad using the edit script and then change it to say something stupid as all ads are posted in their relevant section on the website – neeko Sep 06 '12 at 23:03
  • i do have a login setup, i use phpass to hash the users password when they sign up, then when the users log in, my log in script then hashes the password entered, checks it against the hash in the database and then eithers authorises or rejects – neeko Sep 06 '12 at 23:04
  • the way i do my login is on top of checking the password I use a code that i generate randomly which i store in the database and in a cookie. This code is temporary and helps prevent me from having to store the password anywhere thus keeping it safe in its little cell. So you can use a similar system, generate a random code that is not used for security in and of itself but you use it instead of the password every time you check the login. This hash code is temporary and expires after a set time unless the user logs out. Also on top of that I verify ip in a similar way – Osman Sep 06 '12 at 23:10
  • the ip can be spoofed however it is just another hoop someone will have to jump through, the ip of every session is logged when the user logs in preventing any other user from hijacking the current login session by stealing the cookies I have set for that session unless they also spoof the ip, there are a million other levels I could go through but this is not the place. Just look into a handful of systems that can secure the way you check the username and password and help store enough information in a secure way to verify the login every time you need to. – Osman Sep 06 '12 at 23:12
  • thank you for your help, much appreciated, am now currently looking how to implement this! – neeko Sep 06 '12 at 23:14