Very Big Sql Injection Hole

aghhh! injection? in a premium modification? >.<
Example:
Warn.php?&do=ViewWarnings&id=1/

Input isnt escaped before being put into the sql query. Seriously, what the ****. A premium modification that doesn't even check the input

I made a Fix:
open warn.php

find:

// #######################################################################
// ######################## START MAIN SCRIPT ############################
// #######################################################################

Below that insert:

//SQL-safe modification
function safescape($key,&$value){
$value = mysql_escape_string($value);
}
$func = 'safescape';
array_walk(&$_GET,$func);
array_walk(&$_POST,$func);
//SQL safety mod done

[sv1cec]

Originally Posted by pimpery

aghhh! injection? in a premium modification? >.<
Example:
Warn.php?&do=ViewWarnings&id=1/

Input isnt escaped before being put into the sql query. Seriously, what the ****. A premium modification that doesn't even check the input

I made a Fix:
open warn.php

find:

// #######################################################################
// ######################## START MAIN SCRIPT ############################
// #######################################################################

Below that insert:

//SQL-safe modification
function safescape($key,&$value){
$value = mysql_escape_string($value);
}
$func = 'safescape';
array_walk(&$_GET,$func);
array_walk(&$_POST,$func);
//SQL safety mod done

Could you please elaborate? I am not sure I follow you.

[pimpery]

Originally Posted by sv1cec

Could you please elaborate? I am not sure I follow you.

what do u mean. therse an sql injection hole

look uptop at the example i posted.

[sv1cec]

Pimpery, not all of us have extreme experience in SQL or php or the works. From the example you show, I can assume two things :

1. The problems is that anyone can see the admin's warnings.
2. The slash at the end may allow hackers to insert code (for this I am not sure, but having spend some time reading articles I got containing the "SQL injection" phrase here, that's what I assume.

I am going to spend some more time today, figuring out this whole issue, but what I fail to understand is the function you provided.

So, please, instead of just posting a warning thread, saying VERY BIG HOLE WOLF, WOLF, you could spend some minutes helping me out understand how to close the hole and how to use that code. As I said, not all of us were born with that knowledge.

Rgds

[Soup]

In the Warning System (v3.1.9) in Warn.php, on line 332 you are running a potentially unsafe query (for example if a user enters a non-numerical input as the id variable). If you enter a string that doesn't start with a number (such as "foo") as the id the code will catch the problem on line 328-330, however if the string entered starts with a number (such as "3foo") then it will pass through the check fine.

In order to fix this:

Before line 328-330 add:

PHP Code:

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


or globalize the variable:

PHP Code:

globalize($_GET,array('id' => INT)); 


and then you use $id instead of $_GET['id'].

[sv1cec]

Originally Posted by Soup

In the Warning System (v3.1.9) in Warn.php, on line 332 you are running a potentially unsafe query (for example if a user enters a non-numerical input as the id variable). If you enter a string that doesn't start with a number (such as "foo") as the id the code will catch the problem on line 328-330, however if the string entered starts with a number (such as "3foo") then it will pass through the check fine.

In order to fix this:

Before line 328-330 add:

PHP Code:

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


or globalize the variable:

PHP Code:

globalize($_GET,array('id' => INT)); 


and then you use $id instead of $_GET['id'].

Soup,

Thanks for the details, much appreciated. I have already been working on this, since this morning, and I have globalized all the _GET or _POST variables. But I was not aware that you can use kist $id instead of $_GET['id']

So any variable entered in the globalize function, can then be used with the part withing the brackets, that's what I figure out from looking at the globalize code. Am I correct? And how do you differentiate if you have a _GET['do'] and a _POST['do']?

Also, for STR variables, except from globalizing them (maybe using STR_NOHTML), is it also necessary to addslashes to them?

Again, thanks for the input, sincerely appreciated.

[Soup]

Originally Posted by sv1cec

Soup,

Thanks for the details, much appreciated. I have already been working on this, since this morning, and I have globalized all the _GET or _POST variables. But I was not aware that you can use kist $id instead of $_GET['id']

So any variable entered in the globalize function, can then be used with the part withing the brackets, that's what I figure out from looking at the globalize code. Am I correct? And how do you differentiate if you have a _GET['do'] and a _POST['do']?

That is correct. It would be the same as writing

PHP Code:

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


The only way to check if it is _POST or _GET would be to check per if statements which one of them is set, however I don't think it is ever necessary to do so.

If you only want input from _GET, you use $_GET as the first parameter of globalize(), if you only want input from _POST you use $_POST. If you want input from both and don't care which one is used, you can use $_REQUEST.

Originally Posted by sv1cec

Also, for STR variables, except from globalizing them (maybe using STR_NOHTML), is it also necessary to addslashes to them?

Again, thanks for the input, sincerely appreciated.

Yes. STR basically leaves the input untouched (besides calling trim()), and STR_NOHTML converts all HTML tags and double quotes (") into 'safe' equivalents, however the 'dangerous' (in terms of SQL Injections) single quotes (') are untouched. So if you are running a query, you will need to run addslashes() on it.

[sv1cec]

Originally Posted by Soup

That is correct. It would be the same as writing

PHP Code:

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


The only way to check if it is _POST or _GET would be to check per if statements which one of them is set, however I don't think it is ever necessary to do so.

If you only want input from _GET, you use $_GET as the first parameter of globalize(), if you only want input from _POST you use $_POST. If you want input from both and don't care which one is used, you can use $_REQUEST.



Yes. STR basically leaves the input untouched (besides calling trim()), and STR_NOHTML converts all HTML tags and double quotes (") into 'safe' equivalents, however the 'dangerous' (in terms of SQL Injections) single quotes (') are untouched. So if you are running a query, you will need to run addslashes() on it.

One more question, if you do not mind. Can the globalize be located at the top of the file, right after requst or request_once, or shall it be right above the _GET in the various segments of the file?

Also, I tried echoing an _GET variable, before and after the globalize command, and indeed the contents are different. Can I still use the _GET['xxx'] after the globalize, or is it a must that I should use $xxx?


Tnx again

[Soup]

Originally Posted by sv1cec

One more question, if you do not mind. Can the globalize be located at the top of the file, right after requst or request_once, or shall it be right above the _GET in the various segments of the file?

Also, I tried echoing an _GET variable, before and after the globalize command, and indeed the contents are different. Can I still use the _GET['xxx'] after the globalize, or is it a must that I should use $xxx?


Tnx again

Typically you're going to have a globalize() line inside each 'do' section, where it globalizes all the variables that you need for that 'do' section. You could globalize it all at the beginning (after including global.php) and everything will work just fine, but it is not 'clean' coding and will add a slight (negligible in my opinion) load for intializing variables don't exist otherwise (though that most likely will not have a noticeable effect, it's just the principle of it )

The first parameter of globalize is passed along as a reference, so the original variable is changed and you could continue to use $_GET['foo'] after globalizing it. However it is usually much less to write if you just use $foo instead and will make the code a bit more nicer looking.

[sv1cec]

Originally Posted by Soup

Typically you're going to have a globalize() line inside each 'do' section, where it globalizes all the variables that you need for that 'do' section. You could globalize it all at the beginning (after including global.php) and everything will work just fine, but it is not 'clean' coding and will add a slight (negligible in my opinion) load for intializing variables don't exist otherwise (though that most likely will not have a noticeable effect, it's just the principle of it )

The first parameter of globalize is passed along as a reference, so the original variable is changed and you could continue to use $_GET['foo'] after globalizing it. However it is usually much less to write if you just use $foo instead and will make the code a bit more nicer looking.

Soup,

Many thanks for this quite interesting tuitorial. Really very very helpful.

I have applied your suggestions, and I'll release a new version of AWS in a while.

Kind regards and many thanks