Skip to:
Content
Pages
Categories
Search
Top
Bottom

Security Bug Report Contact

  • It seems that contact emails and mailing lists might not be monitored.

    Are there any developers here so we report the issue privately?

    Thanks.

Viewing 25 replies - 1 through 25 (of 70 total)

  • _ck_
    Participant

    @_ck_

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


    _ck_
    Participant

    @_ck_

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

    Done.

    I’ve sent it to your email at bbpress.org. Let me know after you’ve fixed this issue so we can update our finding status (‘unfixed’ to ‘fixed’).

    Done.

    I’ve sent it to your email at bbpress.org. Let me know after you’ve fixed this issue so we can update our finding status (‘unfixed’ to ‘fixed’).


    _ck_
    Participant

    @_ck_

    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.


    _ck_
    Participant

    @_ck_

    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.

    We learnt bbpress is somewhat linked with wordpress.

    However, according to our testing result on wp latest version, it is certainly not vulnerable to this issue.

    We learnt bbpress is somewhat linked with wordpress.

    However, according to our testing result on wp latest version, it is certainly not vulnerable to this issue.


    John James Jacoby
    Keymaster

    @johnjamesjacoby

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


    John James Jacoby
    Keymaster

    @johnjamesjacoby

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

    Glad to hear it, John.

    Merry X’mas from YGN Ethical Hacker Group,

    http://yehg.net

    Glad to hear it, John.

    Merry X’mas from YGN Ethical Hacker Group,

    http://yehg.net


    John James Jacoby
    Keymaster

    @johnjamesjacoby

    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.


    John James Jacoby
    Keymaster

    @johnjamesjacoby

    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.


    _ck_
    Participant

    @_ck_

    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.


    _ck_
    Participant

    @_ck_

    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.


    John James Jacoby
    Keymaster

    @johnjamesjacoby

    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.


    John James Jacoby
    Keymaster

    @johnjamesjacoby

    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.


    _ck_
    Participant

    @_ck_

    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.


    _ck_
    Participant

    @_ck_

    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.


    _ck_
    Participant

    @_ck_

    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.


    _ck_
    Participant

    @_ck_

    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_

    +1 for _ck_


    John James Jacoby
    Keymaster

    @johnjamesjacoby

    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.

Viewing 25 replies - 1 through 25 (of 70 total)
  • The topic ‘Security Bug Report Contact’ is closed to new replies.
Skip to toolbar