MYSQL/PHP update issue

[EmC]

I'll start by saying that I have been working with php and mysql for exactly 13 hours at this point.

I am trying to write a couple of pages that will allows my band students to register via the web on the first day of camp. Basic stuff like name, grade, instrument, etc. I have the basics down. I am sure my code is horrible and their are much better ways to go about what I am doing, but I have just been using some tutorials from here and there. I keep running into formatting issues and other things that I can't seem to resolve because one tutorial writes something differently than the another.

Here is what I can do already.

- Register a new user and insert the record into the DB. here (http://hchscarlsen.com/Database/data_entry.php)
(feel free to enter some fake data, it would make my life easier)
- Look up the user by last name then link to their specific record to attempt to update it. here (http://hchscarlsen.com/Database/admin_index.php)
(there are currently only two records one with a last name of 'last' and another with 'Doe')

Here is where I encounter an issue. All of the information is there and I can change it. When I click submit all of the changed data is passed, the only thing not happening is an actual update in the database and I am getting really frustrated. AGHHHH!

Here is my code for the update.php action on the button.
<?
require('/home1/hchscarl/unpw/unpw.php');

$id=$_POST['id'];
$last=$_POST['last'];
$first=$_POST['first'];
$dob=$_POST['dob'];
$address=$_POST['address'];
$city=$_POST['city'];
$state=$_POST['state'];
$zip=$_POST['zip'];
$phone=$_POST['phone'];
$email=$_POST['email'];
$shirt=$_POST['shirt'];
$guard1=$_POST['guard1'];
$guard1phone=$_POST['guard1phone'];
$guard1email=$_POST['guard1email'];
$guard1occ=$_POST['guard1occ'];
$guard2=$_POST['guard2'];
$guard2phone=$_POST['guard2phone'];
$guard2email=$_POST['guard2email'];
$guard2occ=$_POST['guard2occ'];
$instrument=$_POST['instrument'];
$brand=$_POST['brand'];
$model=$_POST['model'];
$serial=$_POST['serial'];
$con1=$_POST['con1'];
$con2=$_POST['con2'];
$com1=$_POST['com1'];
$com2=$_POST['com2'];
$grade=$_POST['grade'];

print $id;
print $last;
print $first;
print $dob;
print $address;
print $city;
print $state;
print $zip;
print $phone;
print $email;
print $shirt;
print $guard1;
print $guard1phone;
print $guard1email;
print $guard1occ;
print $guard2;
print $guard2phone;
print $guard2email;
print $guard2occ;
print $instrument;
print $brand;
print $model;
print $serial;
print $con1;
print $con2;
print $com1;
print $com2;
print $grade;



$query = "UPDATE info SET last='$last',first='$first', dob='$dob', address='$address', city='$city', state='$state', zip='$zip', phone='$phone', email='$email', shirt='$shirt', guard1='$guard1', guard1phone='$guard1phone', guard1email='$guard1email', guard1occ='$guard1occ',
guard2='$guard2', guard2phone='$guard2phone', guard2email='$guard2email', guard2occ='$guard2occ', instrument='$instrument', brand='$brand', model='$model', serial='$serial', con1='$con1', con2='$con2', com1='$com1', com2='$com2', grade='$grade WHERE id=$id";

mysql_query($query);

mysql_close();


?>

Basically what gives?:lol:

[Brad]

I believe you're missing a single quote here:
grade='$grade WHERE

which should be:
grade='$grade' WHERE

:)

Also, if someone hasn't already beat me to the punch regarding SQL injection, tomorrow evening I'l write out a nice lengthy explanation on that subject, and maybe throw in a few more general pointers.

[EmC]

Well isn't that a PITA. I guess sometimes it just takes another set of eyes. Thanks Brad!

[Gargoyle]

In preparation for what Brad is going to write for you, have a think if what will happen to your database if I faked your form and sent the following value for $_POST['id']...

99 OR 1=1

[chucker]

In other words: building SQL statements together from strings is bad practice and should be avoided — it can easily lead to dramatic security holes (as Gargoyle's post alludes to). At the very least, use mysql*escape*() methods, but in most cases, use parameterized queries / prepared statements.

http://bobby-tables.com/
http://bobby-tables.com/php.html

[Brad]

In preparation for what Brad is going to write for you, have a think if what will happen to your database if I faked your form and sent the following value for $_POST['id']...

99 OR 1=1

In other words: building SQL statements together from strings is bad practice and should be avoided — it can easily lead to dramatic security holes (as Gargoyle's post alludes to). At the very least, use mysql*escape*() methods, but in most cases, use parameterized queries / prepared statements.

http://bobby-tables.com/
http://bobby-tables.com/php.html

Exactly this. :)

And if you haven't clicked through to chucker's links yet:

http://imgs.xkcd.com/comics/exploits_of_a_mom.png

Sadly, this kind of thing happens far more frequently than it should. You read about some new website that got "hacked" and data was lost or stolen or manipulated by some way because the developers were naive enough to, well, just let it happen.

Basically, one of the first rules for dynamic web development is that you do not trust user input. Ever! You should never use form input for database queries or control logic or filesystem access without sanitizing it first.

What exactly does that mean?

"Sanitizing" the input basically means checking that you are given what (and only what) you expect to be given. Would you simply expect the "zip" value, for example, to be a numeric string of a certain length? Then you should verify all of those conditions (type=string, min<length<max, contains only numbers and maybe a dash) first before doing anything else with it.

With SQL, you have to be extra careful, and sometimes your own sanitizing checks may by insufficient. After all, if you have any relatively complicated set of inputs, you're probably going to miss some kind of checks on your first few attempts. Fortunately, most modern SQL databases support a feature called "variable binding" that adds an extra later of sanitizing for you. With variable bindings, the database query contains placeholders for the inputs, and you when supply those inputs separately, the database enforces certain type checks.

For example, take the following simplified version of your query:

$query = "UPDATE info SET last='$last', first='$first', address='$address' WHERE id=$id";

An attacker wouldn't know what your actual query or schema looks like, but he could try a few common/easy angles like the one Gargoyle mentioned. If I used Gargoyle's input, the executing query would look like this:

$query = "UPDATE info SET last='LastName', first='FirstName', address='Random Street' WHERE id=99 OR 1=1";

Because of the "OR 1=1" which will always evaluate to true, this would result in all rows being updated with my inputs!

Here's how the query would look with variable bindings:

$query = "UPDATE info SET last=?, first=?, address=? WHERE id=?";

Then you'd arrange your inputs and pass the query and the values to the appropriate functions to execute them. You could use PHP's raw mysqli (http://php.net/manual/en/book.mysqli.php) objects (using stmt_init, prepare, bind_param, and execute), or you could use an abstraction library like PDO (http://php.net/manual/en/book.pdo.code) or ADOdb (http://adodb.sourceforge.net/).

I'm most familiar with ADOdb; so, this is how I might do it:

$query = "UPDATE info SET last=?, first=?, address=? WHERE id=?";
$binds = array($first, $last, $address, $id); // these variables would have already been lightly sanitized
$db->Execute($query, $binds);

Does that make sense? :)

[EmC]

Wow. That's a lot. For the most part I understand the concept. Bad people can add things to my DB queries by inserting some unexpected things. I've gone back through and taken the user's input and passed it through the escape function for each value. So that it looks like this.

$last=mysql_real_escape_string($last);

Would this most likely be enough? Or does that do anything to help at all?

[Brad]

Yep, that's probably the quickest way to protect yourself from SQL injection. :)