WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 7 weeks ago

#28699 new defect (bug)

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

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

Description

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

Attachments (3)

miqro-28699.patch (460 bytes) - added by miqrogroove 8 months ago.
Make the filter even more greedy.
miqro-28699.2.patch (1.2 KB) - added by miqrogroove 8 months ago.
... but don't always run the filter.
miqro-28699.3.patch (3.5 KB) - added by miqrogroove 8 months ago.
Add unit tests.

Download all attachments as: .zip

Change History (20)

comment:1 @azaozz8 months 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.

comment:3 @SergeyBiryukov8 months 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].

comment:4 @miqrogroove8 months 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?

comment:5 @ircbot8 months ago

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

comment:6 @miqrogroove8 months 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?

comment:7 @miqrogroove8 months 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.

comment:8 @miqrogroove8 months 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.

@miqrogroove8 months ago

Make the filter even more greedy.

@miqrogroove8 months ago

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

comment:9 @miqrogroove8 months ago

  • Keywords kses added

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

@miqrogroove8 months ago

Add unit tests.

comment:10 @miqrogroove8 months 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.

comment:11 @ircbot8 months ago

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

comment:12 @ircbot7 months ago

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

comment:13 @DrewAPicture7 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.

comment:14 @SergeyBiryukov6 months ago

  • Milestone changed from Future Release to 4.1

comment:15 @miqrogroove4 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.

comment:16 @azaozz4 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.

comment:17 @iseulde7 weeks ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

Note: See TracTickets for help on using tickets.