News:

SMF 2.1.4 has been released! Take it for a spin! Read more.

Main Menu

REPORT: Potentially "Fatal Flaw" in SMF's bbcode parser

Started by dougiefresh, July 31, 2015, 09:12:39 AM

Previous topic - Next topic

Ninja ZX-10RR

Not to be rude but this time... Did you even try to READ that code? Short answer: NO.

This drove me really mad, not for the error in itself because everyone makes mistakes and I don't blame dougie at all for it, as it's just a typo. However, if you 1) review the code 2) merge the pull request, you should  *at least* READ it before doing the above (and actually testing it doesn't hurt either, it takes literally one minute to apply the fix on a test forum).
So why am I being somewhat rude? Here you go, read it: function_param_order($message, &$parameters, &$out) and should obviously be function fix_param_order($message, &$parameters, &$out)
Nobody tested it - when I did, I had a white page.

Now you will blame me for attacking etc... But I am not attacking, I am just noticing issues and reporting them. And this one is a serious issue with the whole procedure.

Also... That code fails anyway, for other reasons.
Quote from: BeastMode topic=525177.msg3720020#msg3720020
It's so powerful that on this post and even in the two PMs you sent me,you still answered my question very quickly and you're apologizing for the delay. You're the #1 support I've probably ever encountered man, so much respect for that. Thank you, and get better soon.

I'll keep this in my siggy for a while just to remind me that someone appreciated what I did while others didn't.

♥ Jess ♥

STOP EDITING MY PROFILE

Kindred

well, actually, you ARE attacking, because it is obviously YOU who has not read closely enough.

He submitted a pull request.
All requests and submissions are reviewed before being accepted and included...

thank you for your suggestion on a correction, but get off your bloody high horse.
Слaва
Украинi

Please do not PM, IM or Email me with support questions.  You will get better and faster responses in the support boards.  Thank you.

"Loki is not evil, although he is certainly not a force for good. Loki is... complicated."

Ninja ZX-10RR

Quote from: Kindred on August 01, 2015, 04:05:09 PM
All requests and submissions are reviewed before being accepted and included...
And that's where the flaw is. Nobody reviewed it! Even I, being dumb at php coding, could see such things.

Quote from: Kindred on August 01, 2015, 04:05:09 PM
thank you for your suggestion on a correction,
You're welcome.

Quote from: Kindred on August 01, 2015, 04:05:09 PMbut get off your bloody high horse.
I just told the guys in charge to read what they do, before doing it, to prevent the old bugs that occurred with the latest versions etc. And someone saying multiple times you are better than his own self is not really on a high horse, but more someone who recognizes what he can do (suggest fixes/improvements, being support team elsewhere) and what he cannot do (code better than devs). ;)

Regards
Quote from: BeastMode topic=525177.msg3720020#msg3720020
It's so powerful that on this post and even in the two PMs you sent me,you still answered my question very quickly and you're apologizing for the delay. You're the #1 support I've probably ever encountered man, so much respect for that. Thank you, and get better soon.

I'll keep this in my siggy for a while just to remind me that someone appreciated what I did while others didn't.

♥ Jess ♥

STOP EDITING MY PROFILE

Suki

The code hasn't been merged, so claiming nobody has reviewed it is kinda premature. The code has been posted yesterday (my time) and I only got a chance to see it today, the rest of the devs will see it when they got time to see it too.  Please have some patient ;)

I still haven't have a look at the PR, will do when I have sufficient amount of time to properly review it and test it.

Thank you for pointing out that typo, it has been noted duly noted, thanks.
Disclaimer: unless otherwise stated, all my posts are personal and does not represent any views or opinions held by Simple Machines.

Ninja ZX-10RR

Aah, if that is the case then it's alright, thanks :) You know I was a bit like wtf ;D
Quote from: BeastMode topic=525177.msg3720020#msg3720020
It's so powerful that on this post and even in the two PMs you sent me,you still answered my question very quickly and you're apologizing for the delay. You're the #1 support I've probably ever encountered man, so much respect for that. Thank you, and get better soon.

I'll keep this in my siggy for a while just to remind me that someone appreciated what I did while others didn't.

♥ Jess ♥

STOP EDITING MY PROFILE

dougiefresh

 ::) Ah, crap!  Now that you pointed that out, I've fixed it in my mod and made a comment in the GitHub about what I meant to use instead of what I did....

@Ninja ZX-10RR: I never claimed to be perfect.  I make mistakes, just like everybody else.  Yup, sad to say, I missed that one!   :-[  Funny, my copy of Sources/Subs.php worked fine with the modified code, but I don't know how a small part of it disappeared from the edit for mod.  I made my commit based on the mod I wrote....

Joshua Dickerson

#26
Without testing it out, I don't think this works the same as the original. I don't think the following works:

[bbc param="This is a test message" /]

[bbc param="[josh]" /]

[url=http://www.simplemachines.org/community/index.php?action=something;boards[]=1;boards[]=2]Some link[/url]
Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

Joshua Dickerson

#27
Without testing it, I think this saves a lot of RAM without losing any functionality. It will allow you to get a lot more parameters but not infinite.

In Subs.php, find:
// This is long, but it makes things much easier and cleaner.
if (!empty($possible['parameters']))
{
$preg = array();
foreach ($possible['parameters'] as $p => $info)
$preg[] = '(\s+' . $p . '=' . (empty($info['quoted']) ? '' : '"') . (isset($info['match']) ? $info['match'] : '(.+?)') . (empty($info['quoted']) ? '' : '"') . ')' . (empty($info['optional']) ? '' : '?');

// Okay, this may look ugly and it is, but it's not going to happen much and it is the best way
// of allowing any order of parameters but still parsing them right.
$match = false;
$orders = permute($preg);
foreach ($orders as $p)
if (preg_match('~^' . implode('', $p) . '\]~i', substr($message, $pos1 - 1), $matches) != 0)
{
$match = true;
break;
}

// Didn't match our parameter list, try the next possible.
if (!$match)
continue;


Replace // This is long, but it makes things much easier and cleaner.
if (!empty($possible['parameters']))
{
// Didn't match our parameter list, try the next possible.
if (!find_bbc_parameters($possible['parameters'], substr($message, $pos1 - 1), $matches))
continue;


Add this to Subs.php
/**
* Find the correct order for the BBC parameters in a string
*
* @param array $parameters
* @param string $message_part
* @param array &$matches
*
* @return bool
*/
function find_bbc_parameters(array $parameters, $message_part, &$matches)
{
$preg = array();
foreach ($parameters as $p => $info)
{
$preg[] = '(\s+' . $p . '=' . (empty($info['quoted']) ? '' : '"') . (isset($info['match']) ? $info['match'] : '(.+?)') . (empty($info['quoted']) ? '' : '"') . ')' . (empty($info['optional']) ? '' : '?');
}

// Okay, this may look ugly and it is, but it's not going to happen much and it is the best way
// of allowing any order of parameters but still parsing them right.
$match = false;
$orders = permute(array_keys($preg));

foreach ($orders as $keys)
{
$match_string = '~^';
foreach ($keys as $key)
{
$match_string .= $preg[$key];
}
$match_string .= '\]~i';

if (preg_match($match_string, $message_part), $matches) != 0)
{
$match = true;
break;
}
}

return $match;
}


As two added benefits, it gets rid of the $orders array and doesn't do so much string copying with the substr() in the loop.

marg edit: fixed the code as per Joshua's next post
Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

Joshua Dickerson

SMF won't let me edit my post. At the find_bbc_matches call: find_bbc_matches should be find_bbc_parameters and $possible should be $possible['parameters']
Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

dougiefresh

#29
Submitted a new PR and commit....

@Joshua: I think you're missing the whole point.  The issue is the PERMUTE function!  Please go back and read the first post....  ::) :P  And yes, I did attempt using the permute function with array_keys.  That actually produced a LARGER array than with the parameter regex's, which I truly don't understand....

dougiefresh

Quote from: Joshua Dickerson on August 02, 2015, 12:34:52 PM
[url=http://www.simplemachines.org/community/index.php?action=something;boards[]=1;boards[]=2]Some link[/url]
I tested this with both my code and SMF's original code.  Neither renders the link as it should....

Quote from: Joshua Dickerson on August 02, 2015, 12:34:52 PM
[bbc param="This is a test message" /]

[bbc param="[josh]" /]

You're going to have to provide an actual example of what you're referring to that I can actually test against.

Illori

maybe

[code=replace]text[/code] would be an example of what he is saying. you can also put replace in quotes.

margarett

Just to add a bit more to this...

The issue is also being discussed @elkarte.net and Spuds noticed an important "detail"
http://www.elkarte.net/community/index.php?topic=2805.msg19158#msg19158

It seems that some permutations are already not calculated and you should have blown PHP's memory limit wayyyy before the 14th parameter...

I would agree with their assertion: basically fix the wrong permutations and limit the number of parameters to 5 or 6. While your findings are good, I'm not sure if anyone will ever use that many parameters and just the fact that you allow that many memory to be drained (optimized or not) is an *excellent* way of allowing a DoS in your forum...
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

dougiefresh

#33
I've fixed the fix_param_order function so that it correctly deals with situations like this:
[tag="abc]def[esdf]"]
The new code reads like this:
// Rearranges all parameters to be in the right order.  Returns TRUE if no parameters are leftover.
function fix_param_order($message, &$parameters, &$out)
{
$test = substr($message, 0, $pos = strpos($message, ']'));
while (substr_count($test, '"') % 2 !== 0)
{
$pos += ($pos1 = strpos(substr($message, $pos), '"'));
if ($pos1 === false)
break;
$test = substr($message, 0, ($pos += strpos(substr($message, $pos), ']')));
}
$params = explode(' ', $test);
unset($params[0]);
$order = array();
$out = $old = '';
foreach ($params as $param)
{
if (strpos($param, '=') === false)
$order[$old] .= ' ' . $param;
else
$order[$old = substr($param, 0, strpos($param, '='))] = substr($param, strpos($param, '=') + 1);
}
foreach ($parameters as $key => $ignore)
{
$out .= (isset($order[$key]) ? ' ' . $key . '=' . $order[$key] : '');
unset($order[$key]);
}
return count($order) == 0;
}


Quote from: margarett on August 03, 2015, 08:07:34 AM
I would agree with their assertion: basically fix the wrong permutations and limit the number of parameters to 5 or 6. While your findings are good, I'm not sure if anyone will ever use that many parameters and just the fact that you allow that many memory to be drained (optimized or not) is an *excellent* way of allowing a DoS in your forum...
I frankly disagree, but not interested in arguing about it right now.  Yeah, DoS's are not desirable in any way, and frankly, I had no idea that it was a problem until it was reported.  I fixed my mod as soon as I could, because obviously it's not a good thing to crash a forum  :P

BTW, My mod has been doing the "rearrange the parameters" thing for a while in order to correctly process the message IDs parameter at the end of the tag usage.....  Seems like a good thing to try...  O:)  So I did a parse_bbc fix on the Post & PM ILA mod to do the rearranging of the parameters (again), then run a single regex to check to see if that parameter mix is found.  Works....

EDIT: I wanna say this.  My solution (if I can ever get it working to y'all's satisfaction) is a cheaper alternative, memory and time-wise, to permutating a number of parameters then test to see which permutation is the correct one.  Granted, the performance difference on tags with few parameters doesn't make much of a difference, but it makes a difference the more parameters a tag has.... Just my two cents....

live627

Quote
I would agree with their assertion: basically fix the wrong permutations and limit the number of parameters to 5 or 6.
+1

margarett

@dougie, much respect for your hard work and passion ;)

Yet, you need to understand that *any* change we could put into "production" regarding parse_bbc needs to *extremely* evaluated. It's probably the biggest, oldest and most important function in SMF's code... The fact that it hasn't been touched for *years* (which is not necessarily a good thing :P ) also confirms this.

The thing with the sheer number of parameters (and the memory loophole that exists here) is that... no one actually will use them. Heck, most people don't even know the BBCodes if there aren't buttons around, let alone add 6, 7, 8 parameters to it and do it correctly...
And by working the beast which is parse_bbc in order to "fix" an edge case, we might be messing many other cases which are now working great. It needs *serious* test and benchmarking before it ever gets to the code...
Overall that many parameters is (IMO) poor UI. It should be preferable to split a BBC in 2 or something like that, instead of stuffing an endless list or arguments which, quite frankly, no one will have the patience to use...
Se forem conduzir, não bebam. Se forem beber... CHAMEM-ME!!!! :D

QuoteOver 90% of all computer problems can be traced back to the interface between the keyboard and the chair

Colin

You could simply keep track of the max amount of BBC code parameters and if a user passes in more than the max then simply throw an error before trying to evaluate and find a match. I think Dougie's solution is something we should adopt, regardless if it is used often now.
"If everybody is thinking alike, then somebody is not thinking." - Gen. George S. Patton Jr.

Colin

Joshua Dickerson

Quote from: dougiefresh on August 02, 2015, 04:50:45 PM
Submitted a new PR and commit....

@Joshua: I think you're missing the whole point.  The issue is the PERMUTE function!  Please go back and read the first post....  ::) :P  And yes, I did attempt using the permute function with array_keys.  That actually produced a LARGER array than with the parameter regex's, which I truly don't understand....
I didn't miss the point at all. The issue is with the RAM consumption and the time. The cause is the permute() function. Since you didn't provide a solution, I just tweaked the current, working solution to better handle RAM and the amount of iterations it is likely to do.

Good work on your solution. I would like to see it tested further but it looks good.
Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

dougiefresh

My Faster SMF BBCode Parser mod has been released, so maybe it will be tested more....  Btw, I've closed my pull requests until a better solution to the problem has been thoroughly tested and is satisfactory to the testers/users...

Joshua Dickerson

I don't have an SMF install right now, but should this line us " instead of "?
$tpos += ($pos1 = strpos(substr($message, $tpos), '"'));
Come work with me at Promenade Group



Need help? See the wiki. Want to help SMF? See the wiki!

Did you know you can help develop SMF? See us on Github.

How have you bettered the world today?

Advertisement: