Ticket #751 (closed defect: fixed)

Opened 16 months ago

Last modified 14 months ago

unstripped slashes in activity links

Reported by: buzz_lightyear Owned by: apeatling
Priority: blocker Milestone:
Component: Keywords: activity streams, kses, has-patch
Cc: djpaul@…, buzz_lightyear@…

Description

Changeset 1484:
add_filter( 'bp_get_activity_content', 'wp_filter_kses' );

this line causes unstripped slashes in Activity stream URLs.

commenting it out, everything is back to normal.

Change History

  Changed 16 months ago by buzz_lightyear

Solution: Change order of filters so the "stripslashes_deep" is the last one.

<?php

/* Apply WordPress defined filters */
add_filter( 'bp_get_activity_content', 'wptexturize' );

add_filter( 'bp_get_activity_content', 'convert_smilies' );

add_filter( 'bp_get_activity_content', 'convert_chars' );

add_filter( 'bp_get_activity_content', 'wpautop' );

add_filter( 'bp_get_activity_content', 'make_clickable' );

add_filter( 'bp_get_activity_content', 'wp_filter_kses' );

add_filter( 'bp_get_activity_content', 'stripslashes_deep' );

?>

  Changed 16 months ago by DJPaul

  • cc djpaul@… added

1484 hardens against XSS attacks. Any fix needs to ensure that it doesn't reintroduce the security issue.

follow-up: ↓ 4   Changed 15 months ago by DJPaul

  • priority changed from major to blocker

in reply to: ↑ 3   Changed 15 months ago by buzz_lightyear

Replying to DJPaul:

+1 for that, however fix is very easy

  Changed 15 months ago by DJPaul

How then? :)
I mean if I understand this, kses calls addslashes. Stripslashes_deep removes the slashes. So when the kses filter protects against some dodgy html/javascript, stripslashes just undoes that?

  Changed 15 months ago by DJPaul

(that's referring to the suggested filter re-ordering)

follow-up: ↓ 8   Changed 15 months ago by DJPaul

  • keywords kses, has-patch added; kses removed

I've just tested your suggestion buzz_lightyear and it fixes this bug and it doesn't reenable the particular XSS attack vector that I found before! Wish I understood how w/r/t my previous concerns but i'm not complaining :)

in reply to: ↑ 7   Changed 15 months ago by buzz_lightyear

  • cc buzz_lightyear@… added

Replying to DJPaul:

I've just tested your suggestion buzz_lightyear and it fixes this bug and it doesn't reenable the particular XSS attack vector that I found before! Wish I understood how w/r/t my previous concerns but i'm not complaining :)

Hi DJ,
so are you now confirming, that the fix is working also for you? :)

thanx ;)

follow-up: ↓ 10   Changed 15 months ago by buzz_lightyear

hi,
this seems to be fixed in r1501 and it works for me now.
kses removed and filters were changed to:

<?php

/* Apply WordPress defined filters */
add_filter( 'bp_get_activity_content', 'wptexturize' );

add_filter( 'bp_get_activity_content', 'convert_smilies' );

add_filter( 'bp_get_activity_content', 'convert_chars' );

add_filter( 'bp_get_activity_content', 'wpautop' );

add_filter( 'bp_get_activity_content', 'stripslashes_deep' );

add_filter( 'bp_get_activity_content', 'make_clickable' );

?>

@DJPaul, can you please confirm and close as fixed?
I'd close it myself, but would like to see someone else confirming the fix too.

thanx

in reply to: ↑ 9   Changed 15 months ago by buzz_lightyear

Replying to buzz_lightyear:

it was actually fixed in  http://trac.buddypress.org/changeset/1492

  Changed 15 months ago by DJPaul

I'll test it by the weekend and post here (busy until then)

  Changed 15 months ago by apeatling

  • status changed from new to closed
  • resolution set to fixed

This is fixed as of [1504], it was due to the ordering of filters.

  Changed 14 months ago by anonymous

  • milestone Activity Streams 1.1 deleted

Milestone Activity Streams 1.1 deleted

Note: See TracTickets for help on using tickets.