bbPress.org

You can email me at the address on my donate page on my website.

Thanks, I got the report.

This is interesting.

Do you find that WordPress fails this test too? Because it uses a very similar routine.

I feel the problem should be addressed in function wp_sanitize_redirect which would solve it cross-platform.

Got the details and will patch asap. (Currently mobile and have limited Internet access due to visiting family in rural areas for holidays.)

Happy holidays to you too.

I am confident this potential exploit is now fixed, so if anyone wants to put some eyes on the bb-login.php from trunk would be helpful.

Maybe I am getting rusty, but looking at the diff, I don’t see it.

Basically it’s the same thing, shuffled around a bit and the same esc_url and esc_attr is being used at the end, which is the same pattern as the existing 1.0/1.1 code.

Are you sure the fix was not being caused by just having a more updated version of esc_url and esc_attr ?

Or was it just being solved by moving those two sanitizers up higher before bb_safe_redirect could ever be called. Because that was definitely an oversight on someone’s part (not me). Based on the comments I’d guess they thought their functionality was for display filtering and not actually sanitization.

Looking at esc_url though, its default is indeed meant for displaying urls by default, note the comment in the code “Replace ampersands and single quotes only when displaying.” and how the context is set by default to “display”.

Maybe test your solution with a url that contains an ampersand, it probably will not work?

Also, are you relying on the list of protocols to do the sanitization? Because that could be defeated too (and would prevent using relative urls that are legitimate).

ps. very minor but you have confusing indentation indicating nesting with the code at line 18 – which actually is not nested

Feel free to email me directly if that’s better.

In this topic is fine. Would rather have more eyes on this than less.

The solution involved the shuffling of things around, as well as this specific addition: https://trac.bbpress.org/browser/trunk/bb-login.php#L51

Basically if the esc’ed $re is now empty, fall back to the installation root. At first this seemed like a silly solution, but because the login always attempts to smart redirect, there isn’t a circumstance where it would naturally be empty. By moving the esc’s up and letting them filter out the baddies, it’s possible to end up with an empty $re.

Tested with ampersands and question marks and it appears to work fine. I’ll try more esoteric URL combinations and see if it breaks.

I indented that code because of the repeated empty( $re ) checks. I had a hard time keeping track of how many times it needed to repeat the same check and bumped them in for clarity. Not a common formatting technique but helpful to me at the time.

I am guessing but moving up esc_url only works because the vulnerable url fails the list of allowed protocols. This will break relative urls being passed (try using /forums/ for example without http)

But since relative is a rare case (I use relative but via plugin) I guess it’s acceptable.

Looks like WP’s emergency update for esc_url should also be ported to bbpress.

I am thinking we need to do a security update for 0.9 as well since many people (thousands) are locked into it.

+1 for _ck_

I’m in a tough spot with regards to internet access until I’m home from holiday on Jan 3; home or not, getting updated versions of 0.9.x and 1.0.x out asap is my top priority.