Very Big Sql Injection Hole

[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 :o

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]

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 :o

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]

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:

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


or globalize the variable:

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


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

[sv1cec]

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:

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


or globalize the variable:

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]

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 $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.

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]

That is correct. It would be the same as writing $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]

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]

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

[sirbutts]

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

pimpery did post an explanation on how to patch it. goto the first post blindy :nervous:
and how did he say WOLF WOLF....just because you cant code doesnt mean you have to take it out on him. He did explain why there's a big hole in the first post as well.
Input isnt escaped before being put into the sql query. Seriously, what the ****. A premium modification that doesn't even check the input

[j_86]

sirbutts;

Please recreate a simmilar warning system with more features than this one and then i'm sure the majority of the users here will consider reading what you posted :)

[pimpery]

sirbutts, thank you for backing me up. and, nubster, not everyone has a ++++load of time. and amount of time you spend on something, doesn't make you a better coder. he could suck but have a lot of time to do this kind of stuff.

[sv1cec]

sirbutts, thank you for backing me up. and, nubster, not everyone has a ++++load of time. and amount of time you spend on something, doesn't make you a better coder. he could suck but have a lot of time to do this kind of stuff.
Sirbutts and pimpery,

I may be speaking to the expert programmers on php and vBulletin, so apologies if my hack was not up to your level. As far as I remember, in the original hack's thread, I clearly said that I am no expert (like you two). Has it occured to you that this thread was the first time I heard the expression "SQL Injection"?

And in my answer to Pimpery, I asked specific questions. Pimpery provided a function, with no instructions on how to use it. Am I suppose to guess or to take the advise of experts? He did posted this thread, and I asked a question. He didn't bother answering, instead Soup jumped in and provided some explanation. Shall I consider Pimpery's attitude as "the arrogance of the experts"?

And yes Pimpery, I have a ****load of time in my hands. With two sites to maintain, two 3 year old kids to take care of, and a family of 5. You are right, the amount of time you invest in something doesn't make you a better coder, as much as the amount of time you live on this earth doesn't make you a better person, but some times it polishes your skills. I sincerely hope you are very very young.

In any case, the hole is closed, so that's history.