Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#45067 closed enhancement (fixed)

Add CSS URL sanitization to kses.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: pento's profile pento
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests fixed-5.0
Focuses: Cc:

Description

Kses allows author and contributor level users to add images via HTML, passing the sources through some error checking and sanitization.

Neither authors or contributors can add background images via CSS as any style attributes containing brackets (and several other characters) are stripped out.

Basic CSS santization for URLs needs to be added to allow unprivileged users to use the cover image block in the new editor.

Related to Gutenberg issue 2539.

Attachments (6)

45067.diff (4.5 KB) - added by peterwilsoncc 6 years ago.
45067.2.diff (4.6 KB) - added by azaozz 6 years ago.
45067.3.diff (8.4 KB) - added by peterwilsoncc 6 years ago.
45067.4.diff (7.2 KB) - added by peterwilsoncc 6 years ago.
45067.5.diff (6.7 KB) - added by peterwilsoncc 6 years ago.
45067.6.diff (7.5 KB) - added by azaozz 6 years ago.

Download all attachments as: .zip

Change History (24)

@peterwilsoncc
6 years ago

#1 @peterwilsoncc
6 years ago

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

In 45067.diff:

#2 @azaozz
6 years ago

Looking at the patch, it does:

$css_test_string = str_replace( $url_match, '', $css_test_string );

where $url_match is the actual URL. That leaves background-image: url() in place which still triggers removal of the whole selector afterwards. Updating.

@azaozz
6 years ago

#3 @azaozz
6 years ago

45067.2.diff is based on 45067.diff and:

  • Removes the whole url(*) part from the CSS property.
  • Makes the first regex a bit shorter/faster, and moves the trimming of the actual URL to the second regex.
  • A bit of cleanup and some white space fixes (this is a very old code...).

#4 @peterwilsoncc
6 years ago

Thanks for the update @azaozz.

45067.3.diff is based on the second patch but includes a few unit tests.

#5 @peterwilsoncc
6 years ago

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

#6 @peterwilsoncc
6 years ago

  • Keywords commit added

45067.4.diff removes the white space changes my IDE helpfully made outside of the modified functions.

The current patch includes Oz's code and my unit tests. I reckon it's good to go.

#7 @pento
6 years ago

  • Keywords commit removed

Let's not change the behaviour of wp_kses_bad_protocol(). If an empty array is passed in the $allowed_protocols parameter, it should continue to block everything, rather than defaulting to whatever wp_allowed_protocols() returns.

#8 @peterwilsoncc
6 years ago

Changes to wp_kses_bad_protocol() removed in 45067.5.diff.

#9 @pento
6 years ago

  • Keywords commit added

👍🏻

#10 follow-up: @peterwilsoncc
6 years ago

  • Keywords commit removed

Hitting a problem in the shortcode validation tests as a result of this:

1) Tests_Shortcode::test_escaping with data set #8 ('<div style="background:url([[...y]])">', '<div style="background:url([[...y]])">')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<div style="background:url([[gallery]])">'
+'<div style="background:url([gallery])">'


tests/phpunit/tests/shortcode.php:457

[[gallery]] is a valid relative URL, I'll step over the code to see where the outer brackets are stripped.

#11 follow-up: @peterwilsoncc
6 years ago

It looks like it is getting stripped in do_shortcodes_in_html_tags() but I unable to figure out where, @azaozz do you have any knowledge around this?

wp> do_shortcodes_in_html_tags( '<p><a href="[[gallery]]" style="background:url([[gallery]])">Pens and pencils</a></p>', false, [ 2 => 'gallery' ] );
=> string(97) "<p><a href="&#91;gallery&#93;" style="background:url(&#91;gallery&#93;)">Pens and pencils</a></p>"

#12 in reply to: ↑ 10 @azaozz
6 years ago

Replying to peterwilsoncc:

[[gallery]] is a valid relative URL

Hmmm, looking at https://tools.ietf.org/html/rfc3986#section-2.2 [ and ] seem "reserved" as "general delimiters", together with :, /, ?, @, and #. So for [[gallery]] to be a valid relative URL the [ and ] will have to be ulrencoded, or if they are used as delimiters, they will have to be after ? char?

It looks like it is getting stripped in do_shortcodes_in_html_tags()...

Ugh, shortcodes again!! :(

Yeah, we do a lot of sanitization for all kinds of weird uses of shortcodes, will try to figure out what's going on there.

This actually brings a valid question: do we want to support shortcodes in style attributes that have url(*) for users without unfiltered_html? Currently they are not supported (as we remove the whole CSS rule) and seems we should keep it that way.

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

#13 in reply to: ↑ 11 @azaozz
6 years ago

Replying to peterwilsoncc:

It looks like it is getting stripped in do_shortcodes_in_html_tags()

Right, think I figured it out. It is in do_shortcodes_in_html_tags() at https://core.trac.wordpress.org/browser/branches/5.0/src/wp-includes/shortcodes.php#L415.

The first thing do_shortcode_tag() (from preg_replace_callback( "/$pattern/", 'do_shortcode_tag', $attr, -1, $count );) does is to check if the shortcode was "escaped" by using [[ and ]]. If it is, it strips the extra [ and ] and returns the remaining shortcode without executing the callback for it. Then the shortcode output is additionally filtered with wp_kses_one_attr( $new_attr, $elname );.

The failing test is for escaped shortcode [[gallery]], so the expected behavior is to get [gallery] back. That's what happens, however until now the whole background:url([gallery]) was stripped by safecss_filter_attr(), i.e. $new_attr was an empty string, and because of

if ( '' !== trim( $new_attr ) ) {
    // The shortcode is safe to use now.
    $attr = $new_attr;
}

the $attr remained unchanged (it's safe as it has already passed through kses when saving) and no shortcode is "executed".

After the patch here we keep background:url(*) CSS values, so $new_attr is not empty any more. So $attr gets set to $new_attr in the above code, and shows as test error when in fact is the proper output.

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

@azaozz
6 years ago

#14 @azaozz
6 years ago

In 45067.6.diff:

  • Fixed the shortcodes test, replaced the selector with an invalid one (assuming the original test was for a value that won't pass through wp_kses_one_attr();).
  • Added couple of tests for malformed values.
  • Added another conditional to look for malformed values and prevent notices.
Last edited 6 years ago by azaozz (previous) (diff)

#15 @peterwilsoncc
6 years ago

Thanks for following that up, boss.

I've run the full test suite locally and it's passing with the new patch.

As it's deep in the weeds of both KSES and shortcodes, I'll leave it in @pento's hands to review (again) and add the commit keyword (again, hopefully) before taking any further action.

#16 @pento
6 years ago

In 43781:

KSES: Allow url() to be used in inline CSS.

The cover image block uses the url() function in its inline CSS, to show the cover image. KSES didn't allow this, causing the block to not save correctly for Author and Contributor users. As KSES does already check each attribute name against an allowed list, we're able to add an extra check for certain attributes to be able to use the url() function, too.

Props peterwilsoncc, azaozz, pento, dd32.
See #45067.

#17 @pento
6 years ago

  • Keywords fixed-5.0 added

#18 @pento
6 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 44136:

KSES: Allow url() to be used in inline CSS.

The cover image block uses the url() function in its inline CSS, to show the cover image. KSES didn't allow this, causing the block to not save correctly for Author and Contributor users. As KSES does already check each attribute name against an allowed list, we're able to add an extra check for certain attributes to be able to use the url() function, too.

Merges [43781] from the 5.0 branch to core.

Props peterwilsoncc, azaozz, pento, dd32.
Fixes #45067.

Note: See TracTickets for help on using tickets.