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

[notnownikki]

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/

[swissspidy]

Hey there,

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

[notnownikki]

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"

[iseulde]

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]

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

[iseulde]

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.

[notnownikki]

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 :)

[iseulde]

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!

[iseulde]

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.

[iseulde]

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

[notnownikki]

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

[notnownikki]

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

[notnownikki]

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.

[iseulde]

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

[notnownikki]

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

[notnownikki]

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

[iseulde]

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]

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.

[iseulde]

@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.

[notnownikki]

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?

[iseulde]

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.

[iseulde]

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.

[notnownikki]

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 .... 

}

[notnownikki]

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]

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]

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.

[azaozz]

@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.

[notnownikki]

noreferrer added in 43187.7.diff

[iseulde]

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

[notnownikki]

@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]

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.

[notnownikki]

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

[iseulde]

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

[notnownikki]

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.

[notnownikki]

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'

[azaozz]

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.

[azaozz]

In 42770:

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

Props notnownikki, iseulde, azaozz
Fixes #43187

[iseulde]

Thanks for taking care of this @azaozz!

[johnbillion]

[42770] should be merged into 5.0. The @since tags are missing and need adding in.

[notnownikki]

.12 should have the @since tags

[pento]

👍🏻

[peterwilsoncc]

42770.diff​ is 43187.12.diff modified to reapply cleanly after [43731].

[peterwilsoncc]

In 43732:

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

Props notnownikki, iseulde, azaozz.
Merges [42770] to the 5.0 branch.
Fixes #43187.

[peterwilsoncc]

Reopening for adding the @since tags to trunk.

[peterwilsoncc]

In 43733:

Formatting: Add new tests missed in [43732].

Completes merging [42770] to the 5.0 branch.
See #43187.

[nikeo]

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

[peterwilsoncc]

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.

[ocean90]

Related: #45352 (wp_targeted_link_rel filter can result in rel attribute with no value)

I think #45352 and #45292 should get fixed or this should get reverted from the 5.0 branch.

[pento]

Let's revert this for now. I like the idea, but we'll need to address the bugs reported, first.

[peterwilsoncc]

In 43930:

Formatting: Revert pre-save filter adding rel="noopener".

Removes filters adding rel="noopener" to links targeting _blank.

Previous implementation could introduce blank rel attributes and could corrupt JSON data when saving via the customizer.

See #43187.
Reverts [43732] and [43733] from the 5.0 branch.

[peterwilsoncc]

Follow-up tickets will be address during the 5.1 cycle.

[pento]

Closing this as fixed, the follow up tickets need to be addressed, though.

[SergeyBiryukov]

In 45163:

Docs: Add missing @since tags for wp_targeted_link_rel() and wp_targeted_link_rel_callback().

See #43187, #46543.