[56K WARN (page 2)] Newbie needs help on SQL injection

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

sleazyfrank
Forum Commoner
Posts: 40
Joined: Fri Aug 19, 2005 3:59 am
Location: Horsham, West Sussex

[56K WARN (page 2)] Newbie needs help on SQL injection

Post by sleazyfrank »

Hi there - I'm in my first week of coding in php and it's going great so far. I'm developing a booking system for my company and there are two pages; one displays the courses and dates they are available in the year and the second is a generic booking page that, when you click the Book this course link on the first page which contains the name of the table, which half of the year the user is currently looking at and the record id, takes the querystring and uses it to construct a sql query to pull out the right data from the mysql db. Now it's working fine except a fellow programmer took a look at the booking page and said it was at risk from sql injection attacks.

I've had a look around and found some info on this and that you have to escape the characters so how but after that I am pretty lost.

This is the code:

Code: Select all

//get courseID
					$courseID = $_GET['courseID'];
					//get table for query
					$tableName = $_GET['table'];
					//get which half of the year
					$whichHalf = $_GET['whichHalf'];
					//set up query
					$query = "SELECT * FROM ". $tableName . " WHERE RecordID = " . $courseID;
					//echo $query . "<br>";
					//connect to database and retrieve record
					
					//connect to db
					mysql_connect($host,$username,$password);
					@mysql_select_db($database) or die("Unable to select database");
Obviously I've declared my server details but haven't posted them here. As you can see I GET three variables, the courseID, the tableName and whichHalf which is used to determine which half yearly set of months to get from the db. But as you can see my query is exposed. But all the tutorials and help I have seen on line about solving sql injections talk about queries that go SELECT * FROM sometablename WHERE RecordID = " $blahblah; - so does anyone have any ideas of how to make my page more secure and how to sort my query out so it's safe?

thanks for any comments or help

frank
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

its probebly a really bad idea to use get variables in a query becuase those can be realllly easily tampered with. using post would be best and then use mysql_real_escape_string() around the variables and you should be golden
sleazyfrank
Forum Commoner
Posts: 40
Joined: Fri Aug 19, 2005 3:59 am
Location: Horsham, West Sussex

Post by sleazyfrank »

Hi thanks for the reply - I am only in my 1st week of programming in PHP so a lot of what you said didn't mean much. Sorry! :) Could you possibly reexplain for my inexperienced brain or provide a code example?

Am I correct in doing my link as <a href="bookcourse.php?table=coursesUnixLinux&courseID=<? echo $link ?>&whichHalf=<? echo $whichHalf ?>Book course</a> ?

thanks!

frank
User avatar
dbevfat
Forum Contributor
Posts: 126
Joined: Tue Jun 28, 2005 2:47 pm
Location: Ljubljana, Slovenia

Post by dbevfat »

User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

your syntax looks fine except some missing semicolons.
a suggestion, try using urlencode on url variables.
ex:

Code: Select all

<a href="bookcourse.php?table=<?php echo urlencode("coursesUnixLinux"); ?>&courseID=<?php echo urlencode($link );>&whichHalf=<?php echo urlencode($whichHalf ); ?>">Book course</a> ? "
while using GET or POST, use urldecode($_GET["$var"])
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

Neither urldecode() nor mysql_real_escape_string() will remove the threat of SQL-injection. You want to allow only valid characters.

If you only want to allow alphanumeric characters use something like

Code: Select all

function filterAlphanumeric($string){
      return preg_replace("/[^a-zA-Z0-9]/", "", $string);
}
It strips any character that's not alphabetical or numerical from the string and returns it.

Using that function your code could look like

Code: Select all

//get courseID
               $courseID = filterAlphanumeric($_GET['courseID']);
               //get table for query
               $tableName = filterAlphanumeric($_GET['table']);
               //get which half of the year
               $whichHalf = filterAlphanumeric($_GET['whichHalf']);
               //set up query
               $query = "SELECT * FROM ". $tableName . " WHERE RecordID = " . $courseID;
               //echo $query . "<br>";
               //connect to database and retrieve record
               
               //connect to db
               mysql_connect($host,$username,$password);
               @mysql_select_db($database) or die("Unable to select database");
User avatar
raghavan20
DevNet Resident
Posts: 1451
Joined: Sat Jun 11, 2005 6:57 am
Location: London, UK
Contact:

Post by raghavan20 »

that preg_replace by patrick would help counter sq injection.
I was commenting only about the validity of his url syntax.
patrick, i hope you are replacing all chars other than the ranges, [a-z], [A-Z], 0-9. do all preg_replace do a global search or act on multiple occurences cos in javascript we use this /g modifier. :roll:
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

Using Javascript to prevent SQL injection is pointless, you can easily disable it. You need a serverside solution.
User avatar
neophyte
DevNet Resident
Posts: 1537
Joined: Tue Jan 20, 2004 4:58 pm
Location: Minnesota

Post by neophyte »

If you are expecting an integer in your query string, I like to use is_numeric() on it.
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post by shiznatix »

is_numeric() does not always give correct results becuase if there is any number anywhere in the string it returns true which makes no sence
sleazyfrank
Forum Commoner
Posts: 40
Joined: Fri Aug 19, 2005 3:59 am
Location: Horsham, West Sussex

Post by sleazyfrank »

LOL - hey people remember I am a PHP virgin here! Less than 7 days experience!! I've taken patrikG's code and I'm trying it out now to see what happens.

thanks!

Frank
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

sleazyfrank wrote:LOL - hey people remember I am a PHP virgin here! Less than 7 days experience!!
This is a discussion forum - things are being discussed here ;)
sleazyfrank
Forum Commoner
Posts: 40
Joined: Fri Aug 19, 2005 3:59 am
Location: Horsham, West Sussex

Post by sleazyfrank »

Oki doki - I'll leave you php experts to fight it out, but your code seems to be working fine, well it hasn't interefered with the form. I'll have to get it tested for sql injection. Cheers for help.

BTW I was looking for a good manual to get into php and mysql - I was looking at the visual quickpro 'PHP and MySQL for dynamic websites' by Larry Ullman. Thoughts?

ta

Frank

ps patrikG you're in sussex too??
User avatar
patrikG
DevNet Master
Posts: 4235
Joined: Thu Aug 15, 2002 5:53 am
Location: Sussex, UK

Post by patrikG »

sleazyfrank wrote:Cheers for help.
No worries. ;)
sleazyfrank wrote: BTW I was looking for a good manual to get into php and mysql - I was looking at the visual quickpro 'PHP and MySQL for dynamic websites' by Larry Ullman. Thoughts?
Unless it covers object oriented programming in good detail, I don't think any beginner's PHP book is worth looking at. I usually recommend Harry Fuecks "PHP Anthology".
sleazyfrank wrote:ps patrikG you're in sussex too??
Yup. And I used to work in Horsham for a couple of months some time ago... We always went to the Malt & Shovel :)
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

shiznatix wrote:is_numeric() does not always give correct results becuase if there is any number anywhere in the string it returns true which makes no sence
a-hem:

Code: Select all

[feyd@home]>php -r "var_export(is_numeric('asdf1234'));"
false
[feyd@home]> _
Post Reply