#43187 closed enhancement (fixed)
Add pre-save filter to make target="_blank" always secure
Reported by: | notnownikki | Owned by: | azaozz |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
To protect against phishing attacks made possible by links having target="_blank"
without the appropriate rel
attribute[1], we should have a content filter that adds rel="noopener noreferrer"
where needed.
[1] https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/
Attachments (14)
Change History (66)
#1
@
7 years ago
- Milestone Awaiting Review deleted
- Resolution set to duplicate
- Status changed from new to closed
#2
@
7 years ago
- Resolution duplicate deleted
- Status changed from closed to reopened
I've taken a look at #37941, and I think what I want to do here is a bit different, and I wasn't specific enough in the ticket!
I'd like to introduce a content filter that catches this where the user has edited the HTML and added target="_blank"
#3
@
7 years ago
- Milestone set to Future Release
- Summary changed from Add rel="noopener noreferrer" to links with target="_blank" to Add content filter to make target="_blank" always secure
Maybe it's good to have separate tickets for each area that needs to be fixed? We are currently adding the rel
attribute if you use the visual editor, but not if you edit the HTML. A content filter would be good to have.
#4
@
7 years ago
- Keywords has-patch needs-unit-tests added
Some initial thoughts:
- This content filter will need some tests to ensure all scenarios are covered, e.g. existing rel attribute, or one of the values already there.
- Out of curiosity I was testing whether
target="blank"
works, and surprisingly it does (at least in Safari). Even usingtarget="black"
or anything in it still works. I'm assuming this has the same security issues, so I would just add therel
attribute as soon as there is atarget
attribute present. - I don't think we can use
shortcode_parse_atts
. Do you think we can just use regex here for therel
attribute as well? - Are there any other input fields where the user can add links with a target attribute? Thinking about title, comments etc.
#5
@
7 years ago
Hmm, I used shortcode_parse_atts
because that's how wp_rel_nofollow_callback
does things, so I assumed that was safe to use. I can replace with a regex though, if needed.
Yes, target
can be a named window, creating it if it doesn't exist, so if target
is set, this should happen. Will fix.
And will add unit tests! Wanted to get initial feedback that I was going in the right direction with this, so thanks for taking a look :)
#6
@
7 years ago
Yes, target can be a named window, creating it if it doesn't exist, so if target is set, this should happen. Will fix.
Ah, that clarifies the behaviour, thanks!
#7
@
7 years ago
About wp_rel_nofollow_callback
, that's interesting. I personally don't feel that comfortable using short code specific functions here, unless we rename it to xml_parse_atts
or something.
#9
@
7 years ago
True, and only having to check that the target
attribute is present makes it simpler. Great! Will fix.
#10
@
7 years ago
I've added https://core.trac.wordpress.org/attachment/ticket/43187/43187.1.diff which has unit tests for the filter function, better target handling (it just needs target to exist in the link) and better support (and test coverage!) for manually entered rel
values.
#11
@
7 years ago
This won't match: <a target="_blank" href="#">
. How about <a[^>]*\starget\s*=[^>]*>
or something similar? Should be faster too?
#14
@
7 years ago
This is looking good to me, I just think we should filter more than just the content. From first sight, it looks like I can still add the target attribute to titles and comments, maybe more?
#15
@
7 years ago
New diff attached.
This covers titles, comments, and content to make sure existing content is safe, and adds the filter to pre_kses
so all new content is safe too.
#16
@
7 years ago
@azaozz reminded me that pre_kses
only works for users who are not allowed to use unfiltered HTML, so this won't work for admins and editors. :( I think the best way is to still use a save filter such as kses (not display), and to circumvent the unfiltered_html
cap check. In other words, we'll have to add the filter everywhere kses is added. Sorry for not realising that earlier.
#17
@
7 years ago
Ah, I guess the results I've been seeing are from the display filter then! I'll disable them and see what I can work out.
I think it still makes sense to do the display filters though, because I'd like to be sure that existing content is made safe too. Does that seem ok?
#18
@
7 years ago
- Keywords needs-unit-tests removed
Another thing you could do is move the current_user_can( 'unfiltered_html' )
check inside the filters, which also eliminates the need for
add_action( 'init', 'kses_init' ); add_action( 'set_current_user', 'kses_init' );
Which is kind of strange IMHO.
#19
@
7 years ago
I think it still makes sense to do the display filters though, because I'd like to be sure that existing content is made safe too. Does that seem ok?
I initially thought that sounded good, but it seems we are also only stripping scripts etc. on save too (not display) and adding another regex on display will impact performance again (there are quite a few already). I don't know what's best though. I'm curious what others think.
#20
@
7 years ago
Yeah... ok, I'll remove the display filtering from this diff for now.
Looks like I can set up the filters in kses_init
instead, and deal with the admin/editor roles much more easily:
function kses_init() { kses_remove_filters(); if ( ! current_user_can( 'unfiltered_html' ) ) { kses_init_filters(); } // Phishing filters (for ALL users) add_filter .... }
#21
@
7 years ago
Heh, well, maybe not as straightforward!
Looks like I can add the filter that way, but it's never activated... Anyway, i'll debug more and update when I have it working instead of updating with everything I find :)
#22
@
7 years ago
Added a new diff, reworked it into a pre-save filter, rather than a content filter.
As part of kses_init
it adds a call to kses_init_phishing_filters
which are always added even if the user can supply unfiltered html.
The filter has been modified to deal with slashes, but they're optional so this can be used on escaped or unescaped content. Units tests were added for this.
I've tested by including links with targets and without targets in titles, posts, and comments.
#23
@
7 years ago
- Summary changed from Add content filter to make target="_blank" always secure to Add pre-save filter to make target="_blank" always secure
In 43187.7.diff : Changed the filter function name to wp_targeted_link_rel
as that's what it's dealing with, and the options to rel
are now filterable.
#24
@
7 years ago
@notnownikki thanks for the patch, looks good.
Thinking that we should implement this security hardening in as many browsers as possible. Firefox now supports rel="noopener"
, but Edge still doesn't: https://caniuse.com/#feat=rel-noopener. So we should probably add noreferrer
there too.
Good explanation of how this works and what it does: https://mathiasbynens.github.io/rel-noopener/.
#27
@
7 years ago
@iseulde my eyes had been playing tricks on me too, and I was too occupied with filters to think about it...
43187.9.diff fixes this. We already add nofollow
to links in comments, so this should be the behaviour we want now.
#28
@
7 years ago
- Milestone changed from Future Release to 5.0
In 43187.10.diff:
- Added a parameter (the matched link HTML) and some docs for the filter.
- One more test where
target="foo"
. - Moved adding the filters to default-filters.php as we want them on for everybody all the time. Seems there is no point in removing and re-adding them together with the kses filters.
@notnownikki, @iseulde any thoughts or anything else? Thinking this is good to go in.
#30
@
7 years ago
Don't we have a lot more fields where the user can insert HTML, such as taxonomy descriptions, bio etc.?
Here's some more in core that seems to have kses filters: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/default-filters.php#L15-L83
#31
@
7 years ago
Looks like we should be adding where any of wp_filter_kses
, wp_kses_post
, or wp_kses_data
are applied, as they'll let through a
tags. The last two blocks headed Save URL
and Display URL
will filter out tags anyway, but we should apply to the rest. I'll get on it.
#32
follow-up:
↓ 34
@
7 years ago
New diff added, the filter is applied in all these cases (all of them pre save, no display filters)
'pre_user_email', 'title_save_pre', 'pre_user_first_name', 'pre_term_description', 'pre_link_name', 'pre_link_target', 'pre_user_nickname', 'pre_comment_author_email', 'pre_link_notes', 'pre_user_last_name', 'pre_term_name', 'pre_user_display_name', 'pre_comment_content', 'pre_link_rel', 'content_save_pre', 'pre_user_description', 'content_filtered_save_pre', 'excerpt_save_pre', 'pre_comment_author_name', 'pre_link_description'
This ticket was mentioned in Slack in #core by notnownikki. View the logs.
7 years ago
#34
in reply to:
↑ 32
@
7 years ago
Replying to notnownikki:
New diff added, the filter is applied in all these cases...
Many of these are filtered through either 'sanitize_text_field'
, 'sanitize_email'
or 'wp_strip_all_tags'
that remove all HTML tags. Seems we only need to check for link targets on 'pre_term_description', 'pre_link_description', 'pre_link_notes', 'pre_user_description'
.
Updated patch coming up.
#35
@
7 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from reopened to closed
In 42770:
#37
@
6 years ago
- Keywords needs-patch added; has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
[42770] should be merged into 5.0. The @since
tags are missing and need adding in.
#42
@
6 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for adding the @since
tags to trunk.
#45
follow-up:
↓ 46
@
6 years ago
Hi @notnownikki @peterwilsoncc , filtering content_save_pre
with wp_targeted_link_rel
breaks the publishing of the customize changeset, because it corrupts the JSON of the customized data. It makes it impossible to save customizations.
I've opened a ticket about it : https://core.trac.wordpress.org/ticket/45292. I think that the problem should be addressed before releasing WP 5.0, because it can potentially impact many websites.
Are you already aware of the issue and is there any plan to fix it?
Thanks
#46
in reply to:
↑ 45
@
6 years ago
Replying to nikeo:
I've opened a ticket about it : https://core.trac.wordpress.org/ticket/45292. I think that the problem should be addressed before releasing WP 5.0, because it can potentially impact many websites.
Are you already aware of the issue and is there any plan to fix it?
I'd missed #45292 but am following it now and will continue the discussion there.
#48
@
6 years ago
- Keywords revert added; fixed-5.0 removed
Let's revert this for now. I like the idea, but we'll need to address the bugs reported, first.
#50
@
6 years ago
- Keywords needs-refresh added; commit revert removed
- Milestone changed from 5.0 to 5.1
Follow-up tickets will be address during the 5.1 cycle.
Hey there,
Thanks for your report! We're already tracking this in #37941.