Show Posts
1
Modifications / Re: SMFPets Version 0.2
« on: June 27, 2007, 05:56:34 am »
Sorry - your right I meant ALPHA 
Whoever coded this (i'm still new here) has got a great product off the ground and I would like to say that I am not criticising - just pointing out some things that could be improved either by the original author or by the community.
I've been looking over this script and it really needs some attention.... probably more than I can offer.
The immediate thing that is problematic is that *all* $_GETS are being pushed straight into SQL query's. That opens the door for SQL injection.
For instance..
Shop-Pets.php - lines 56 - 58
That code is taking a variable $_GET['pet'] from the URL and squirting it directly into mysql,
The thing I noticed about it is that its a number - so lets convert it to a number(integer) first at the top of the function.
As for non numbers (pet names etc) then other methods exist to clean them.
stripslashes($_POST['name']) should perhaps be used in place of $_POST['name'] on its own - or even in some cases htmlspecialchars($_POST['name'])
Luckily SMF does have a little protection for this... however it does mean that its easy to trip the script over and to lockup someones petshop.
Im not the most perfect programmer - so please don't take what I am saying as gospel, however I would suggest looking into sanitization methods.
Whoever coded this (i'm still new here) has got a great product off the ground and I would like to say that I am not criticising - just pointing out some things that could be improved either by the original author or by the community.
I've been looking over this script and it really needs some attention.... probably more than I can offer.
The immediate thing that is problematic is that *all* $_GETS are being pushed straight into SQL query's. That opens the door for SQL injection.
For instance..
Shop-Pets.php - lines 56 - 58
Code: [Select]
elseif (isset($_GET['pet'])) {
$result = db_query("SELECT breed FROM {$db_prefix}shop_pets WHERE pet_id = {$_GET['pet']}", __FILE__, __LINE__);
That code is taking a variable $_GET['pet'] from the URL and squirting it directly into mysql,
The thing I noticed about it is that its a number - so lets convert it to a number(integer) first at the top of the function.
Code: [Select]
$_GET['pet'] = (int) $_GET['pet'];
As for non numbers (pet names etc) then other methods exist to clean them.
stripslashes($_POST['name']) should perhaps be used in place of $_POST['name'] on its own - or even in some cases htmlspecialchars($_POST['name'])
Luckily SMF does have a little protection for this... however it does mean that its easy to trip the script over and to lockup someones petshop.
Im not the most perfect programmer - so please don't take what I am saying as gospel, however I would suggest looking into sanitization methods.
- that doesn't mean the product is dead.