WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

Last modified 3 months ago

#43187 closed enhancement (fixed)

Add pre-save filter to make target="_blank" always secure

Reported by: notnownikki Owned by: azaozz
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
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 (12)

43187.diff (3.7 KB) - added by notnownikki 5 months ago.
filter the_content, add ref="nofollow noopener" where target is _blank
43187.2.diff (2.4 KB) - added by jainnidhi 5 months ago.
43187.1.diff (7.3 KB) - added by notnownikki 5 months ago.
patch with tests, better handling of target and existing rel values
43187.3.diff (7.6 KB) - added by notnownikki 5 months ago.
43187.4.diff (9.5 KB) - added by notnownikki 5 months ago.
43187.5.diff (9.2 KB) - added by notnownikki 5 months ago.
43187.6.diff (9.1 KB) - added by notnownikki 5 months ago.
43187.7.diff (9.1 KB) - added by notnownikki 5 months ago.
43187.8.diff (9.3 KB) - added by notnownikki 5 months ago.
43187.9.diff (9.1 KB) - added by notnownikki 5 months ago.
43187.10.diff (9.3 KB) - added by azaozz 5 months ago.
43187.11.diff (9.5 KB) - added by notnownikki 5 months ago.

Download all attachments as: .zip

Change History (48)

#1 @swissspidy
5 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Hey there,

Thanks for your report! We're already tracking this in #37941.

#2 @notnownikki
5 months 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"

Last edited 5 months ago by notnownikki (previous) (diff)

#3 @iseulde
5 months 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.

@notnownikki
5 months ago

filter the_content, add ref="nofollow noopener" where target is _blank

#4 @iseulde
5 months 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 using target="black" or anything in it still works. I'm assuming this has the same security issues, so I would just add the rel attribute as soon as there is a target attribute present.
  • I don't think we can use shortcode_parse_atts. Do you think we can just use regex here for the rel 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 @notnownikki
5 months 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 @iseulde
5 months 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 @iseulde
5 months 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.

#8 @iseulde
5 months ago

It seems a simple regex that catches the rel attribute would be faster anyway. :)

#9 @notnownikki
5 months ago

True, and only having to check that the target attribute is present makes it simpler. Great! Will fix.

@jainnidhi
5 months ago

@notnownikki
5 months ago

patch with tests, better handling of target and existing rel values

#10 @notnownikki
5 months 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 @iseulde
5 months ago

This won't match: <a target="_blank" href="#">. How about <a[^>]*\starget\s*=[^>]*> or something similar? Should be faster too?

#12 @notnownikki
5 months ago

Ugh, how did I miss that??! Yes, will fix.

@notnownikki
5 months ago

#13 @notnownikki
5 months ago

Fixed, and test added for the case where target is the first attribute.

#14 @iseulde
5 months 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?

@notnownikki
5 months ago

#15 @notnownikki
5 months 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 @iseulde
5 months 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 @notnownikki
5 months 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 @iseulde
5 months 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 @iseulde
5 months 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 @notnownikki
5 months 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 @notnownikki
5 months 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 :)

@notnownikki
5 months ago

#22 @notnownikki
5 months 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.

@notnownikki
5 months ago

@notnownikki
5 months ago

#23 @notnownikki
5 months 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 @azaozz
5 months 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 explanations of how this works and what it does: https://mathiasbynens.github.io/rel-noopener/. More info at Google's Lighthouse (webpage quality checker): https://developers.google.com/web/tools/lighthouse/audits/noopener.

Last edited 5 months ago by azaozz (previous) (diff)

@notnownikki
5 months ago

#25 @notnownikki
5 months ago

noreferrer added in 43187.7.diff 43187.8.diff

Last edited 5 months ago by notnownikki (previous) (diff)

#26 @iseulde
5 months ago

Ah, I misread nofollow as noreferrer. Why are we adding nofollow?

@notnownikki
5 months ago

#27 @notnownikki
5 months 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.

@azaozz
5 months ago

#28 @azaozz
5 months 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.

#29 @notnownikki
5 months ago

43187.10 makes a lot of sense. All seems good to me!

#30 @iseulde
5 months 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 @notnownikki
5 months 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: @notnownikki
5 months 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.


4 months ago

#34 in reply to: ↑ 32 @azaozz
4 months 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 @azaozz
4 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from reopened to closed

In 42770:

Add pre-save content filter to make target=_blank always secure.

Props notnownikki, iseulde, azaozz
Fixes #43187

#36 @iseulde
3 months ago

Thanks for taking care of this @azaozz!

Note: See TracTickets for help on using tickets.