Alternate fix to injection code in comments

[rpgamersnet]

So, in another thread it was mentioned that the current fix may get the job done, its also filtering out good data. There must be some proper solution to handle incoming comment data securely. I thought I would start a discussion in regards to finding an alternate fix to the problem then the one currently available.

The problem: Users input data into comments that is executed and causes trouble.

Solution ideas: Escape incoming data so that it cannot execute? Allow only alphanumeric comment data and write the SQL statements so that they cannot be broken out?

I will be the first to admit I am not a professional coder, although I do write a lot of code myself. I haven't taken a long look at how the comments are currently handled, but plan to. Lets pool some ideas and help MrZeroPage come up with a more solid fix!

[Sarteck]

I do a few things.


First off, I almost ALWAYS use sprintf(). It's pretty awesome.

sprintf("SELECT * FROM %suser WHERE userid=%d",TABLE_PREFIX,$userid);Bam, you'll always get an integer. Also, query looks hella prettier. :3

Two, why not use vBulletin's built-in cleaning functions on data? That would solve a lot of it, wouldn't it?

Mind you, I'm a complete newbie to the scripting of this modification in particular, but I have successfully programmed a bunch of homebrewed mods for my own. I just want a disclaimer here that I could be completely off-base. X3

[stangger5]

Starting with 2.7.1+

To fix that exploit was to edit one line..

$ibforums->input['s_id'] = ibp_cleansql($ibforums->input['s_id']);
change to
$ibforums->input['s_id'] = intval($ibforums->input['s_id']);

Comment should be OK because of they way strings are put in the database. The problem was s_id was allowed to be a string when it was supposed to be an int, that is what allowed the exploit.

The ibp_cleansql function needs to be changed to accept a second argument that says what type of data it is (string or int) and clean it differently depending on what it is supposed to be.

vBulletin has built in cleaning functions too that can/should be used.

[Sarteck]

See? Perfect example of where sprintf() would be put to awesome use. :D Just use %d in your query and you're good to go.

[rpgamersnet]

Wait, so it wasn't the comments that were causing the problem, but the S_ID? My board personally was not hit by this exploit, so I did not have the details. :)

[Mark.B]

Wait, so it wasn't the comments that were causing the problem, but the S_ID? My board personally was not hit by this exploit, so I did not have the details. :)
I have a feeling that the code Stangger has posted was the fix for the exploit that was fixed in 2.7.1, rather than 2.7.2.

[stangger5]

I have a feeling that the code Stangger has posted was the fix for the exploit that was fixed in 2.7.1, rather than 2.7.2.

I didnt know anything about a exploit with 2.7.2..

[Mark.B]

I didnt know anything about a exploit with 2.7.2..
No I meant FIXED in 2.7.2.

[Hippy]

the only thing needed is what stangger posted above..

--------------- Added 1330552106 at 1330552106 ---------------

I have a feeling that the code Stangger has posted was the fix for the exploit that was fixed in 2.7.1, rather than 2.7.2.
I didnt know anything about a exploit with 2.7.2.. either :eek:

[stangger5]

No I meant FIXED in 2.7.2.

Had me going ...lol...

[Mark.B]

But the code Stangger has posted is NOT what changed in 2.7.2.

[Hippy]

stangger5 knows this mod better than anyone here.. so trust what he says ...
he has tested this out for the last few days...

[stangger5]

But the code Stangger has posted is NOT what changed in 2.7.2.

MrZ changed this:
2.7.1
$ibforums->input['s_id'] = ibp_cleansql($ibforums->input['s_id']);

to this:
2.7.2
$ibforums->input['s_id'] = intval(ibp_cleansql($ibforums->input['s_id']));

I have this:

$ibforums->input['s_id'] = intval($ibforums->input['s_id']);

MrZ`s code is tring to clean the int data .
I`m no guru like MrZ...:)

--------------- Added 1330558171 at 1330558171 ---------------

To get this thread back on track,,here is a very good read for the ones wanting to learn some of the vBulletin Input Cleaner..

Using the vBulletin Input Cleaner (https://vborg.vbsupport.ru/showthread.php?t=119372)

[rpgamersnet]

I guess my question was just if the other part that was added is needed, the looping replace function that removes SQL words from comments (but also removes good data). It is near the bottom of the 2.7.2 arcade.php ... needed or just playing it safe?

[stangger5]

I think,,its playing it safe...Which is not a bad thing these days...

I`m looking into using the vBulletin Input Cleaner instead of the ibp_cleansql..

[Hippy]

I guess my question was just if the other part that was added is needed, the looping replace function that removes SQL words from comments (but also removes good data). It is near the bottom of the 2.7.2 arcade.php ... needed or just playing it safe?

what good data is it removing ?

[rpgamersnet]

what good data is it removing ?

If you refer to this post: https://vborg.vbsupport.ru/showpost.php?p=2304191&postcount=5

The code I am asking about is the loop that removes all the SQL keywords from the comments. Most I'm sure won't come across in normal comments, but filtering out parts like "or" and "and" are going to catch and mess up standard comments, as given in the example on that post.

"I got the high score!" becomes "I got the high sce!"

"Got a great hand on the last round!" -> "Got a great h on the last round"

Some basic words will get filtered as well, not just the bad SQL data, which is why I suggested that maybe this fix is not the best solution. Code I am questioning is quoted here:


function recursive_str_ireplace($replacethis,$withthis,$int his)
{
while (1==1)
{
$inthis = str_ireplace($replacethis,$withthis,$inthis);
if(stristr($inthis, $replacethis) === FALSE)
{
RETURN $inthis;
}
}
RETURN $inthis;
}


// remove any SQL-commands
$sqlcomm[] = 'create';
$sqlcomm[] = 'database';
$sqlcomm[] = 'table';
$sqlcomm[] = 'insert';
$sqlcomm[] = 'update';
$sqlcomm[] = 'rename';
$sqlcomm[] = 'replace';
$sqlcomm[] = 'select';
$sqlcomm[] = 'handler';
$sqlcomm[] = 'delete';
$sqlcomm[] = 'truncate';
$sqlcomm[] = 'drop';
$sqlcomm[] = 'where';
$sqlcomm[] = 'or';
$sqlcomm[] = 'and';
$sqlcomm[] = 'values';
$sqlcomm[] = 'set';
$sqlcomm[] = 'password';
$sqlcomm[] = 'salt';
$sqlcomm[] = 'concat';
$sqlcomm[] = 'schema';
$value = recursive_str_ireplace($sqlcomm, '', $value);

Some recent threads have started to appear complaining of errors appearing, this new code is also the source of those new problems; the new recursive_str_ireplace loop to replace these parts of the comment field.... and any other field being filtered by the ibp_cleansql function.

[Hippy]

thanks .. do this https://vborg.vbsupport.ru/showpost.php?p=2304863&postcount=13
and pull the new code added out..
this will do the job.. but does not work on all servers..

stangger5 is going to work this out ..
I think code it to the way vb does it ..
but this is not set in stone ATM.. just a twinkle in the sky

[rpgamersnet]

thanks .. do this https://vborg.vbsupport.ru/showpost.php?p=2304863&postcount=13
and pull the new code added out..
this will do the job.. but does not work on all servers..

stangger5 is going to work this out ..
I think code it to the way vb does it ..
but this is not set in stone ATM.. just a twinkle in the sky

Yep I already made the change he noted :) If I knew more about the inner workings of VB I'd offer to try to be of more help, but I have never messed with mods much myself. Look forward to any fixes that might arise :)

Thanks to everyone for helping out! Great community this mod has. :D

[Hippy]

stannger5 can explain more about it but this is what I use since the other will kill wanted stuff...

[g7jgq]

If you refer to this post: https://vborg.vbsupport.ru/showpost.php?p=2304191&postcount=5

The code I am asking about is the loop that removes all the SQL keywords from the comments. Most I'm sure won't come across in normal comments, but filtering out parts like "or" and "and" are going to catch and mess up standard comments, as given in the example on that post.

"I got the high score!" becomes "I got the high sce!"

"Got a great hand on the last round!" -> "Got a great h on the last round"

Some basic words will get filtered as well, not just the bad SQL data, which is why I suggested that maybe this fix is not the best solution. Code I am questioning is quoted here:


function recursive_str_ireplace($replacethis,$withthis,$int his)
{
while (1==1)
{
$inthis = str_ireplace($replacethis,$withthis,$inthis);
if(stristr($inthis, $replacethis) === FALSE)
{
RETURN $inthis;
}
}
RETURN $inthis;
}


// remove any SQL-commands
$sqlcomm[] = 'create';
$sqlcomm[] = 'database';
$sqlcomm[] = 'table';
$sqlcomm[] = 'insert';
$sqlcomm[] = 'update';
$sqlcomm[] = 'rename';
$sqlcomm[] = 'replace';
$sqlcomm[] = 'select';
$sqlcomm[] = 'handler';
$sqlcomm[] = 'delete';
$sqlcomm[] = 'truncate';
$sqlcomm[] = 'drop';
$sqlcomm[] = 'where';
$sqlcomm[] = 'or';
$sqlcomm[] = 'and';
$sqlcomm[] = 'values';
$sqlcomm[] = 'set';
$sqlcomm[] = 'password';
$sqlcomm[] = 'salt';
$sqlcomm[] = 'concat';
$sqlcomm[] = 'schema';
$value = recursive_str_ireplace($sqlcomm, '', $value);

Some recent threads have started to appear complaining of errors appearing, this new code is also the source of those new problems; the new recursive_str_ireplace loop to replace these parts of the comment field.... and any other field being filtered by the ibp_cleansql function.

As I posted in another thread, before searching !!!!!!!!!! its also stripping the words out of game names which I suspect will break a lot of games.

When it gets the game name from the posted data

$game_name = ibp_cleansql($_POST['gname']);

A game such as wordrace will end up as wdrace

For now I have just modified the replacement list as follows, its NOT a good fix but at least all of the games will submit scores now :-)

$sqlcomm[] = 'create ';
$sqlcomm[] = 'database';
$sqlcomm[] = 'table';
$sqlcomm[] = 'insert';
$sqlcomm[] = 'update ';
$sqlcomm[] = 'rename';
$sqlcomm[] = 'replace ';
$sqlcomm[] = 'select ';
$sqlcomm[] = 'handler';
$sqlcomm[] = 'delete ';
$sqlcomm[] = 'truncate ';
$sqlcomm[] = 'drop ';
$sqlcomm[] = ' where ';
$sqlcomm[] = ' or ';
$sqlcomm[] = ' and ';
$sqlcomm[] = 'values';
$sqlcomm[] = ' set ';
$sqlcomm[] = 'password';
$sqlcomm[] = 'salt';
$sqlcomm[] = 'concat';
$sqlcomm[] = 'schema';

I know that won't solve the problem in comments but we don't really use comments. I am going to look at an alternative fix for this over the weekend

Cheers

Alex

[stangger5]

I know that won't solve the problem in comments but we don't really use comments. I am going to look at an alternative fix for this over the weekend

Cheers

Alex

Give this a try : https://vborg.vbsupport.ru/showpost.php?p=2307204&postcount=6

[g7jgq]

Give this a try : https://vborg.vbsupport.ru/showpost.php?p=2307204&postcount=6

Thanks for that.

Looking at that code it will do the same thing, the problem is you cannot get rid of SQL command by simply doing replaces in the posted data.

Cheers

Alex