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

[azaozz]

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

[azaozz]

Moving to 4.0 for investigation and eventual patch.

[SergeyBiryukov]

Related: #28506

[SergeyBiryukov]

Caused by the second line in wp_kses_no_null()​:

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

Introduced in [649].

[miqrogroove]

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?

[miqrogroove]

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?

[miqrogroove]

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.

[miqrogroove]

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]

Make the filter even more greedy.

[miqrogroove]

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

[miqrogroove]

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

[miqrogroove]

Add unit tests.

[miqrogroove]

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.

[DrewAPicture]

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

[miqrogroove]

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.

[azaozz]

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

[iseulde]

has-patch 4.2-early so moving to 4.2.

[DrewAPicture]

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

[miqrogroove]

It needs commit love.

[DrewAPicture]

Replying to miqrogroove:

It needs commit love.

Let's make that happen :-)

[nacin]

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

[mdawaffe]

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.

[wonderboymusic]

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

[miqrogroove]

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?

[miqrogroove]

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

[mdawaffe]

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

[wonderboymusic]

In 32860:

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

Adds unit tests.

Props miqrogroove.
Fixes #28699.