The hardest errors to find are the things that aren’t there: A PHP example
In my defense, the actual program is longer than this …
<?php
require ‘connect.php’;
// Create variables ;
$id = strtoupper(trim($_REQUEST[‘id’])) ;
$application_date = $_REQUEST[‘yr_of_apply’] . “-” . $_REQUEST[‘month_of_apply’] .”-“. $_REQUEST[‘day_of_apply’] ;
$assessment_date = $_REQUEST[‘yr_of_assess’]. “-” . $_REQUEST[‘month_of_assess’] .”-“. $_REQUEST[‘day_of_assess’] ;
$eligibility_date = $_REQUEST[‘yr_eligible’].”-“.$_REQUEST[‘month_eligible’] .”-“. $_REQUEST[‘day_eligible’] ;
$ipe_date = $_REQUEST[‘yr_ipe’].”-“.$_REQUEST[‘month_ipe’] .”-“. $_REQUEST[‘day_ipe’] ;
$notify_rights = $_REQUEST[‘notify_rights’] ;
$vocational_goal = $_REQUEST[‘vocational_goal’] ;
// Get user info ;
$result = mysql_query(“UPDATE clients SET notify_rights = ‘$notify_rights’, vocational_goal = ‘$vocational_goal’ ,
application_date = ‘$application_date’ , assessment_date = ‘$assessment_date’
WHERE id = ‘$id'”)
or die(mysql_error()) ;
echo “<p>Your record was updated successfully.</p>” ;
?>
So, it didn’t work. All I wanted to do was connect to the SQL database, find the client’s id and update that record with the application information.
First problem I found was that I had tested this with a much smaller file with just a few columns and in my UPDATE statement it still had the table ‘test’ instead of ‘clients’. I was getting an error that said there was “an error in my SQL code”. Which is true. Since the table did exist, I wasn’t getting an error saying it wasn’t found. Ok, I fixed that
Second problem, the dates were actually entered in three different fields to make it easier for error checking – your year has to be within the current fiscal year, month between 1 and 12, no entering April 31. However, I needed those to BE dates for analyses we plan later. So, I just created the date variables. Problem solved.
Third problem, I realized I really did not want the password and other information required for connection in this program where anyone could see it. There is no personally identifiable information in here, but it’s just a bad habit to have your passwords and other data hanging out there. Hence the statement to require the connect.php script .
Fourth problem, the ID variable is not a number. It can be something like ABJ-001 , so it doesn’t match if it the case is not the same or if there are spaces. The strtoupper and trim functions fixed that.
Fifth problem, some of the dates were blank. I looked at them over and over to see if I had mismatched quotes . I even actually deleted and re-typed the statements. Nope, still, some of the dates were there and others were blank. Maybe they weren’t date format in the table definition? Nope, I checked and the missing dates and the dates that updated properly were all defined as the same format.
Well, maybe you spotted it already … As I said in the first problem, I had originally created and tested the script with a table named ‘test’ that had just a few columns. When I switched to my client table, I had forgotten to add all of the columns to the UPDATE statement. The problem wasn’t in creating the eligibility and ipe date fields. The problem was that I left those fields off of the update statement so they were never getting updated. Everything went through fine, I got a message saying my record was updated – because as far as PHP was concerned, it wasn’t an error. Maybe I only wanted to update some of the columns.
The moral of the story is this: Sometimes the problem is the code you DIDN’T write.
Quote: UPDATE clients SET notify_rights = ‘$notify_rights’, vocational_goal = ‘$vocational_goal’ , application_date = ‘$application_date’ , assessment_date = ‘$assessment_date’ WHERE id = ‘$id’
HORROR. What If I enter ‘; drop table clients;’ in a field?
The answer: PDO. Also Doctrine. Doctrine is your friend. But definitely PDO. It gives some structure to creating SQL from PHP variables.
http://php.net/manual/en/pdo.prepared-statements.php
Also if this is for your math game, I suggest using a framework, such as Slim or Symfony. Slim is easier to learn, Symfony does everything for you if you know how to ask nicely.
Paul, thanks for the comments.
No, this isn’t for our math game, it is for a database we are doing for a client.
I suppose someone could enter drop table clients as the ID variable – but that’s a risk I’m willing to take. Seriously, though, most of the fields are radio buttons and drop down menus.
As I said this is part of a larger program and I am adding the statements to catch possible problems next week. Of course some things will slip through.
I’m not familiar with Slim or Symfony. I’ll take a look. Thanks.
this made me laugh. as founder/creator/programmer blah blah of a db driven site i can relate . i’ve spent many baffling hours staring at code, double spacing code, triple spacing code, removing lines… but for the life of me couldn’t figure out why the db wasn’t reflecting my update…
oh look, i commented out the tsql for the stored proc…..
which would be — for mssql, and would have no effective color change in a front end editor. sigh.
Ha ha sickz, that is my life exactly.
I think you have been hiding in my office spying on me!
Dr. D, what is the language you are programming in?
This is php but sometimes I am using JavaScript, sometimes SAS, rarely SQL
AnnMaria,
Paul’s solution is a little extreme. You don’t need to learn an entire PHP framework or ORM model to protect yourself from these basic things.
All you need to do is spend 5 mins reading about SQL injection, and learn mysql_escape_string() or parameterized queries (the former is simpler).
Not to flog you on your own blog :), but to say it’s a risk you’re willing to take is, at the very least, pedestrian and laughable to someone who knows what they’re doing … and at worst quite dangerous (if _I_ was the client, I’d be pretty pissed if someone was that careless with my data, especially if I was paying for this).
I’d also move connect.php to a non-web addressable location, perhaps one directory up from the world addressable htdocs. That’s another rookie error asking for trouble.
This brings up an interesting point about self qualification of skills. It works to impress most people as long as someone who knows what they’re doing isn’t in the room.
Clint,
Thank you for your concern. I’m sure your clients are very happy with you.
As I said (twice now) this is a snippet of a larger program. As far as a risk I am willing to take – at this point there is nothing in the table but a few lines of fake data we made up for testing purposes, so if someone deletes the whole thing, I really don’t care .
When installed, we will have precautions in place, but no, security is not our primary concern. We don’t have personal information or financial data. We have a small set of data that we will use for reporting on means and frequencies of variables of interest to our client.
Actually, one of the reasons my clients like me and do pay the bills is that I create what they need and no more. I don’t know if they are impressed but they are usually satisfied because I deliver something that works to their specifications for a reasonable price. In 28 years in business, we haven’t had any problems with unauthorized access to data.
The degree of security is related to the degree of risk. Social security numbers, health and financial records -we bend over backwards. Small anonymous data sets on the number of people who came in each day and the type of services, not so much.
I believe Paul and I responded with regard to the essence of the original post: problems with the code.
You pinpointed security in problem #3 of your original post by moving the db connection string to an external file, but did not pinpoint the more obvious and accessible SQL injection problem (which would actually allow access to any database with the same connection string credentials, not just this database).
We expounded further on security because you had already mentioned it when discussing habits to practice. In this case, the stated moral does hold true: Sometimes the problem is the code you DIDN’T write.
And I appreciate your comments .