WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 11 months ago

#28699 closed defect (bug) (fixed)

\0 (backslash+zero) gets stripped from post content for users without "unfiltered_html"

Reported by: azaozz Owned by: miqrogroove
Milestone: 4.3 Priority: normal
Severity: normal Version: 1.0
Component: Formatting Keywords: kses has-patch
Focuses: Cc:

Description

Happens when saving posts from both the Visual and Text editors. Seems caused by kses.

Attachments (5)

miqro-28699.patch (460 bytes) - added by miqrogroove 2 years ago.
Make the filter even more greedy.
miqro-28699.2.patch (1.2 KB) - added by miqrogroove 2 years ago.
... but don't always run the filter.
miqro-28699.3.patch (3.5 KB) - added by miqrogroove 2 years ago.
Add unit tests.
miqro-28699.4.patch (3.0 KB) - added by miqrogroove 12 months ago.
miqro-28699.5.patch (3.2 KB) - added by miqrogroove 11 months ago.

Download all attachments as: .zip

Change History (36)

#1 @azaozz
2 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.0

Moving to 4.0 for investigation and eventual patch.

#3 @SergeyBiryukov
2 years ago

  • Component changed from General to Formatting
  • Version set to 1.0

Caused by the second line in wp_kses_no_null():

$string = preg_replace('/(\\\\0)+/', '', $string);

Introduced in [649].

#4 @miqrogroove
2 years ago

So according to that original kses.php file, \0 has always been stripped, even though the function comments say it was intended to strip NUL bytes only.

Can we call this a bug, or can we identify any concerns in HTML, JS, or CSS related to having that phrase appear in the output?

This ticket was mentioned in IRC in #wordpress-dev by mauteri. View the logs.


2 years ago

#6 @miqrogroove
2 years ago

Looks like \0 is special (octal) in JS in at least two contexts.

\0 is also special (hex) in CSS content attributes.

These might also prefix any non-zero integer.

Any security implications there?

#7 @miqrogroove
2 years ago

I think there's a significant concern with the XSS Cheat Sheet example:

<DIV STYLE="background-image:\0075\0072\006C\0028'\006a\0061\0076\0061\0073\0063\0072\0069\0070\0074\003a\0061\006c\0065\0072\0074\0028.1027\0058.1053\0053\0027\0029'\0029">

Because, wp_kses_no_null() is used inside of safecss_filter_attr(). If the latter function has any usage outside of the usual wp_kses_attr() calls, then someone could be depending on the removal of hex codes for security.

#8 @miqrogroove
2 years ago

A secondary concern would be that the kses filter incorrectly removes multiple chars from the middle of user input. This could be exploited to form other unwanted strings, including \0 itself by simply re-encoding as \\00.

@miqrogroove
2 years ago

Make the filter even more greedy.

@miqrogroove
2 years ago

... but don't always run the filter.

#9 @miqrogroove
2 years ago

  • Keywords kses added

Patch attached. I'll try to write the unit tests before the first beta.

@miqrogroove
2 years ago

Add unit tests.

#10 @miqrogroove
2 years ago

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

In miqro-28699.3.patch:

  • Do not strip \0 from user's CDATA.
  • Do not strip \0 from most attributes.
  • Do strip \0 in STYLE attributes.
  • \0 and \\00 are now identical and removed greedily.
  • STYLE and SCRIPT elements unaffected; kses removes them by default.
  • Add unit tests.

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


23 months ago

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


22 months ago

#13 @DrewAPicture
22 months ago

  • Keywords 4.1-early added
  • Milestone changed from 4.0 to Future Release

Too late in the cycle for something like this. Let's try to hit it in 4.1 early. Patch still applies.

#14 @SergeyBiryukov
21 months ago

  • Milestone changed from Future Release to 4.1

#15 @miqrogroove
19 months ago

  • Keywords 4.2-early added; 4.1-early removed

Please punt to Future Release. I need to discuss this more with azaozz, and it looks like we can get it ready for 4.2.

#16 @azaozz
19 months ago

  • Milestone changed from 4.1 to Future Release

Right. We probably can also explore removing it in some contexts for users with unfiltered_html.

#17 @iseulde
17 months ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

#18 @DrewAPicture
15 months ago

@miqrogroove, @azaozz: What's left here? The latest patch still applies and the unit tests pass.

#19 follow-up: @miqrogroove
15 months ago

It needs commit love.

#20 in reply to: ↑ 19 @DrewAPicture
15 months ago

  • Keywords commit added

Replying to miqrogroove:

It needs commit love.

Let's make that happen :-)

This ticket was mentioned in Slack in #core by drew. View the logs.


15 months ago

#22 @nacin
15 months ago

  • Keywords 4.2-early removed
  • Milestone changed from 4.2 to Future Release

@mdawaffe or @duck_, can you evaluate this for 4.3 inclusion?

#23 @mdawaffe
15 months ago

wp_kses_no_null( $string, $strip_slash_zero = true ) is a so-called "boolean trap" and violates our coding standards: https://make.wordpress.org/core/handbook/coding-standards/php/#self-explanatory-flag-values-for-function-arguments.

Other than that, I don't see any problems.

#24 @miqrogroove
12 months ago

  • Milestone changed from Future Release to 4.3

#25 @miqrogroove
12 months ago

  • Owner set to miqrogroove
  • Status changed from new to accepted

#26 @wonderboymusic
12 months ago

  • Keywords commit removed

@miqrogroove - can you offer an alternative to alleviate the boolean trap?

#27 @miqrogroove
12 months ago

I'm not understanding what is needed there. I realize it's just a style change. The logic requires a parameter for true/false when removing \0 versus not removing \0. So what is the alternative?

#28 @miqrogroove
12 months ago

Added a patch that uses $slash_zero = 'remove' instead of $strip_slash_zero = true Is that the better style?

This ticket was mentioned in Slack in #core by miqrogroove. View the logs.


11 months ago

#30 @mdawaffe
11 months ago

Yeah - you found the alternatives. I like attachment:miqro-28699.5.patch​ - when reading a call to the function, it's clear what the parameter does. Any of the following would pass the "no boolean trap" test.

wp_kses_no_null( $string, 'keep_slash_zero' ); // Not very WordPressy
wp_kses_no_null( $string, array( 'slash_zero' => 'keep' ) ); // miqro-28699.5.patch
wp_kses_no_null( $string, array( 'keep_slash_zero' => true ) ); // Seems most WordPressy to me

Either of the last two seem fine to me. Stylistically, I may have a slight preference for attachment:miqro-28699.5.patch, since it allows for the current "keep", "remove", and a possible future "urlencode", etc. (We'll never need/want such an option; I'm just talking about style habits, not the code.)

#31 @wonderboymusic
11 months ago

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

In 32860:

Don't strip \0 (backslash+zero) from post content for users without "unfiltered_html"

Adds unit tests.

Props miqrogroove.
Fixes #28699.

Note: See TracTickets for help on using tickets.