Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#43187 closed enhancement (fixed)

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

Reported by: notnownikki's profile notnownikki Owned by: azaozz's profile 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)

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

Download all attachments as: .zip

Change History (66)

#1 @swissspidy
6 years 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
6 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"

Last edited 6 years ago by notnownikki (previous) (diff)

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

@notnownikki
6 years ago

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

#4 @iseulde
6 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 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
6 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 @iseulde
6 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 @iseulde
6 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.

#8 @iseulde
6 years ago

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

#9 @notnownikki
6 years ago

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

@jainnidhi
6 years ago

@notnownikki
6 years ago

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

#10 @notnownikki
6 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 @iseulde
6 years ago

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

#12 @notnownikki
6 years ago

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

@notnownikki
6 years ago

#13 @notnownikki
6 years ago

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

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

@notnownikki
6 years ago

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

@notnownikki
6 years ago

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

@notnownikki
6 years ago

@notnownikki
6 years ago

#23 @notnownikki
6 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 @azaozz
6 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 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 6 years ago by azaozz (previous) (diff)

@notnownikki
6 years ago

#25 @notnownikki
6 years ago

noreferrer added in 43187.7.diff 43187.8.diff

Last edited 6 years ago by notnownikki (previous) (diff)

#26 @iseulde
6 years ago

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

@notnownikki
6 years ago

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

@azaozz
6 years ago

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

#29 @notnownikki
6 years ago

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

#30 @iseulde
6 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 @notnownikki
6 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.

@notnownikki
6 years ago

#32 follow-up: @notnownikki
6 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.


6 years ago

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

Thanks for taking care of this @azaozz!

#37 @johnbillion
5 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.

@notnownikki
5 years ago

#38 @notnownikki
5 years ago

.12 should have the @since tags

#39 @pento
5 years ago

  • Keywords has-patch has-unit-tests commit added; needs-patch removed

👍🏻

@peterwilsoncc
5 years ago

#40 @peterwilsoncc
5 years ago

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

#41 @peterwilsoncc
5 years ago

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

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.

#42 @peterwilsoncc
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for adding the @since tags to trunk.

#43 @peterwilsoncc
5 years ago

In 43733:

Formatting: Add new tests missed in [43732].

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

#44 @pento
5 years ago

  • Keywords fixed-5.0 added

#45 follow-up: @nikeo
5 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

Last edited 5 years ago by nikeo (previous) (diff)

#46 in reply to: ↑ 45 @peterwilsoncc
5 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.

#47 @ocean90
5 years ago

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.

#48 @pento
5 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.

#49 @peterwilsoncc
5 years ago

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.

#50 @peterwilsoncc
5 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.

#51 @pento
5 years ago

  • Keywords needs-refresh removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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

#52 @SergeyBiryukov
5 years ago

In 45163:

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

See #43187, #46543.

Note: See TracTickets for help on using tickets.