Help to stop hackers!

[dachshund]

Hi,

 

Just received this email from a random person saying they were able to obtain the username and password for the site admin page from the MySql Database:

 

[07:33:33] [iNFO] testing if GET parameter 'id' is dynamic

[07:33:34] [iNFO] confirming that GET parameter 'id' is dynamic

[07:33:35] [iNFO] GET parameter 'id' is dynamic

[07:33:35] [iNFO] testing sql injection on GET parameter 'id' with 0 parenthesis

[07:33:35] [iNFO] testing unescaped numeric injection on GET parameter 'id'

[07:33:37] [iNFO] confirming unescaped numeric injection on GET parameter 'id'

[07:33:37] [iNFO] GET parameter 'id' is unescaped numeric injectable with 0 parenthesis

[07:33:37] [iNFO] testing for parenthesis on injectable parameter

[07:33:40] [iNFO] the injectable parameter requires 0 parenthesis

[07:33:40] [iNFO] testing MySQL

[07:33:41] [iNFO] confirming MySQL

[07:33:41] [iNFO] query: SELECT 0 FROM information_schema.TABLES LIMIT 0, 1

[07:33:41] [iNFO] retrieved: 0

[07:33:51] [iNFO] performed 13 queries in 9 seconds

[07:33:51] [iNFO] the back-end DBMS is MySQL

web server operating system: Linux Red Hat

web application technology: PHP 5.2.11, Apache 2.2.3

back-end DBMS: MySQL >= 5.0.0

 

 

+-------+------------------------            +-----------    +----------    +----------------------------------                      +--------+---------------        +

| admin | email | firstname | lastname | password                                          | userid | username        |

+-------+------------------------            +-----------    +----------    +----------------------------------                      +--------+---------------      +

| 1    |  blah | blah  | blah | blah | 1      | blah |

+-------+------------------------            +-----------    +----------    +----------------------------------                      +--------+---------------        +

 

 

blah MD5 : blah

 

--

 

 

I've changed all the details to 'blah' for the purpose of this post. Does anyone know how I can secure my PHP to stock this injection?

 

Thanks,

 

Jack

 

[QuickOldCar]

For the GET requests, do checking on the input, only allow the types of input that is expected, otherwise, die,exit or redirect them elsewhere.

 

Use mysql_real_escape_string() before the mysql query.

 

Look at sanitize as well

http://php.net/manual/en/filter.filters.sanitize.php

[PFMaBiSmAd]

Numerical data that is put into a query statement must be validated as a number or more simply cast it as a number in order to prevent sql injection. Alternatively, you can use prepared statements if you are using a database class that supports them (mysqli, PDO.)

 

Escaping a number has no effect because there are no quotes in the data to escape (you can inject sql that uses no quotes.)

[SaCH]

You can also use htmlentities() function to prevent certain kinds of sql attacks..

[dachshund]

ok, thanks.

 

so is it this part of the code that's vulnerable?

 

$sql = "SELECT * FROM content WHERE id LIKE $id LIMIT 1";

$result=mysql_query($sql);

while($rows=mysql_fetch_array($result)){

[PFMaBiSmAd]

If the $id variable being put into that query is not being properly filtered/validated/cast, then yes a hacker can inject sql into that select query to also dump the complete contents of any of your database tables.

[dachshund]

ok, this is what I have now:

 

$id = mysql_real_escape_string($_GET['id']);
if ($id != (int)$id) {
echo "Invalid ID";
}else {
$sql = "UPDATE content SET views=views+1 WHERE id='$id'";
mysql_query($sql) or die (mysql_error());
}

$sql = "SELECT * FROM content WHERE id LIKE $id LIMIT 1";
$result=mysql_query($sql);
while($rows=mysql_fetch_array($result)){

 

is there anything else I should do?

[SaCH]

try my code

 

$id = mysql_real_escape_string($_GET['id']);
$id = trim(htmlentities($id));
if(!ctype_digit($id)) {
echo "Invalid ID";
}
else {
$sql = mysql_query("UPDATE content SET views=views+1 WHERE id='$id'") or die (mysql_error());
}

 

[Pikachu2000]

There is no need to use htmlentities on a value that hasn't been validated, much less on one that's expected to be numbers only. trim and ctype_digit() are sufficient. There is no need to quote a validated numeric value in the query string either.

[Monkuar]

ok, this is what I have now:

 

$id = mysql_real_escape_string($_GET['id']);
if ($id != (int)$id) {
echo "Invalid ID";
}else {
$sql = "UPDATE content SET views=views+1 WHERE id='$id'";
mysql_query($sql) or die (mysql_error());
}

$sql = "SELECT * FROM content WHERE id LIKE $id LIMIT 1";
$result=mysql_query($sql);
while($rows=mysql_fetch_array($result)){

 

is there anything else I should do?

 

you need to cast your integer's properly

 

do

 

intval($id)

 

 

[PFMaBiSmAd]

You probably also want to prevent the SELECT query from executing. Your current code only prevents the UPDATE query from executing.

[Pikachu2000]

cytpe_digit() combined with trim is a better solution, IMO. intval() will return 0 for 'somerandomstring' or 0.567, and 1 for '1fatwoman' whereas ctype_digit() will return FALSE.

[Monkuar]

Here I edited your code bro, should be fine like this

 

<?php


$id = intval($_GET['id']);
if ($id < 1){echo "No....";exit;}
if ($id != (int)$id) {
echo "Invalid ID";
}else {
$sql = 'UPDATE content SET views=views+1 WHERE id='.mysql_real_escape_string($id).'';
mysql_query($sql) or die (mysql_error());
}

$sql = 'SELECT * FROM content WHERE id LIKE '.mysql_real_escape_string($id).' LIMIT 1';
$result=mysql_query($sql);
while($rows=mysql_fetch_array($result)){

?>

[Pikachu2000]

There is absolutely no reason to use mysql_real_escape_string() on a value that has been validated as all numeric characters.

[SaCH]

I think it will be the right code

 

<?php
$id = $_GET['id'];
if (!trim(ctype_digit($id)) {
echo "Invalid ID";
}
else {
$sql = "UPDATE content SET views=views+1 WHERE id=$id'';
mysql_query($sql) or die (mysql_error());

$sql1 = "SELECT * FROM content WHERE id LIKE $id LIMIT 1";
$result=mysql_query($sql1);
while($rows=mysql_fetch_array($result)){

//something.

}
}

?>

[premiso]

try my code

 

Why are you using htmlentities on a numerical value? That makes no sense at all. If the ID is numerical and a integer, just static cast it to an INT and you are good.

 

$id = (int)$_GET['id'];

$sql = ....

 

If the value is string then you may want to use entities, but it all depends on where the data is going and how it is going to be handled. mysql_real_escape_string is enough to prevent an inject without much else needed. The htmlentities would be to prevent XSS attacks.

 

That is plenty to prevent injection of an integer, nothing else needed.

[dachshund]

so instead of:

 

$id = mysql_real_escape_string($_GET['id']);

$id = trim(htmlentities($id));

 

i can just have:

 

$id = (int)$_GET['id'];

 

and it will be just as safe?

 

[PFMaBiSmAd]

will be just as safe?

 

Since mysql_real_escape_string and htmlentities won't stop the type of sql injection that occurred in your case, the answer would be that casting the id value to an integer will be a lot safer.

[SaCH]

will be just as safe?

 

Since mysql_real_escape_string and htmlentities won't stop the type of sql injection that occurred in your case, the answer would be that casting the id value to an integer will be a lot safer.

 

You mean that mysql_real_escape() string is only make sense in the case of a string. Right ?

[premiso]

You mean that mysql_real_escape() string is only make sense in the case of a string. Right ?

 

The actual name of the function is "mysql_real_escape_string()". So yes, it does only make sense in the case for a string. You can escape other data, but it is better to validate that data with what it should be vs doing a "catch all". Or even better yet, to just use Prepared statements and not have to worry about escaping.

[SaCH]

You mean that mysql_real_escape() string is only make sense in the case of a string. Right ?

 

The actual name of the function is "mysql_real_escape_string()". So yes, it does only make sense in the case for a string. You can escape other data, but it is better to validate that data with what it should be vs doing a "catch all". Or even better yet, to just use Prepared statements and not have to worry about escaping.

 

Oh sorry for my spell mistakes i was mean mysql_real_escape_string() but i made the mistakes due to my fast typing..