Make WordPress Core

Opened 15 years ago

Closed 15 years ago

#12394 closed defect (bug) (fixed)

kses removes valid attribute from xhtml elements

Reported by: dougal's profile dougal Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.2
Component: Formatting Keywords: has-patch, tested, kses, xhtml, html
Focuses: Cc:

Description

There is an edge-case which can cause kses to discard the last attribute of an empty XHTML element, if the closing slash is not preceded by a space.

Example:

input = <img src='foo.jpg' bogus='disallowed attr' alt='my image'/>
output = <img src='foo.jpg'/>
expected = <img src='foo.jpg' alt='my image'/>

The problem is that kses assumes that the closing slash on an XHTML element will always be preceded by a space. While the space is recommended for backwards compatibility with HTML4, it is not strictly required.

Attachments (2)

kses.php.diff (1.0 KB) - added by dougal 15 years ago.
Patch to improve XHTML compatibility on empty elements like img
test_post_filtering.php.diff (1.2 KB) - added by dougal 15 years ago.
A couple of kses testcases, including one for this bug.

Download all attachments as: .zip

Change History (9)

@dougal
15 years ago

Patch to improve XHTML compatibility on empty elements like img

#1 @dougal
15 years ago

For reference, here is the XHTML 1.0 spec section discussing empty elements. Note that in the examples, there is no space: http://www.w3.org/TR/xhtml1/#h-4.6

It is only in the HTML Compatibility Guidelines that they recommend adding the space, for compatibility with HTML4: http://www.w3.org/TR/xhtml1/#guidelines

#2 @nacin
15 years ago

I was able to both verify the bug and verify the fix. Looks sane to me.

Just don't test with a title attribute, as that one isn't whitelisted.

<img src="blah"> // works
<img src="blah" > // works
img src="blah" /> // works
<img src="blah"/> // patch fixes this scenario

#3 @ryan
15 years ago

Looks good. We need to add test cases (test_post_filtering.php).

@dougal
15 years ago

A couple of kses testcases, including one for this bug.

#4 @dougal
15 years ago

  • Version set to 2.9.2

Attached a diff for test_post_filtering.php for a couple of minor kses checks, including the one mentioned in this bug.

Really, as mentioned in the wordpress-dev IRC chat the other day, we should probably work on a whole suite of security-related kses checks.

This reminded me that a while back, there was a suggestion of replacing kses with HTML Purifier. However, I see that HTML Purifier is PHP 5 only, so that decision will have to wait until the PHP version requirement for WordPress is updated in the future.

However, this might be a good resource to look at for formulating unit tests: http://htmlpurifier.org/live/smoketests/xssAttacks.php

#5 @westi
15 years ago

Test cases committed thank you.

#6 @westi
15 years ago

Looks good to me as well.

Committing.

#7 @westi
15 years ago

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

(In [13561]) Improve kses handling of attributes in valid XHTML self closed img tags. Fixes #12394 props dougal.

Note: See TracTickets for help on using tickets.