WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#11833 closed defect (bug) (invalid)

bizarre behavior in the comment form sanitization

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Security Keywords:
Focuses: Cc:

Description

Found this while trying to break the comment form's multiple link regex (#11830):

<a
href = http://foo.com

<a
href = http://foo.com

>test</a>

when I look into my post's code, I get:

<p><a href = <a  href="http://foo.com" rel="nofollow">http://foo.com</a></p>
<p><a href = <a  href="http://foo.com" rel="nofollow">http://foo.com</a></p>
<p>>test</p>

on the plus side, the nofollow regex works. but those extra < and > should have been html encoded.

Change History (17)

comment:1 @miqrogroove6 years ago

Is this reproducible with the default theme? Is it version 3.0 only?

comment:2 @Denis-de-Bernardy6 years ago

this is when logged in, btw. as a visitor, I ultimately get:

<p>&lt;a<br />
href = <a  href="http://foo.com" rel="nofollow">http://foo.com</a></p>
<p><a  href="http://foo.com" rel="nofollow">test</a></p>
<p>test4</p>

better, but it does suggest there might be bugs in kses.

@miqrogroove: using 3.0 with my theme and a bunch of plugins active, but nothing that filters anything comments-related.

comment:3 @miqrogroove6 years ago

I'm not sure kses is applied at all when you have unfiltered_html caps? The output goes through many many other filters.

comment:4 @Denis-de-Bernardy6 years ago

see also this wonderful use case:

http://core.trac.wordpress.org/ticket/11830#comment:4

<
a	
<a	
	href	=	
	http://foo:bar@foo.com#?#

<a
	href	=	http://foo.com?\"'

>test</a>

test6

test5

the stripslashed and filtered result as a visitor:

<
a	
<a	
	href	=	
	http://foo:bar@foo.com#?#

<a>test</a>

test6

test5

<p><<br />
a<br />
<a<br />
	href	=<br />
	<a  href="http://foo:bar@foo.com#?#" rel="nofollow">http://foo:bar@foo.com#?#</a></p>
<p><a>test</a></p>
<p>test6</p>
<p>test5</p>

comment:5 @Denis-de-Bernardy6 years ago

err, I'm sorry, that actually is without filters, and then with filters.

comment:6 @miqrogroove6 years ago

I really can't get this to happen on 2.9.1 with default theme and no plugins. I get:

<p>&lt;<br />
a<br />
&lt;a<br />
	href	=<br />
	<a href="http://foo:bar@foo.com#?#" rel="nofollow">http://foo:bar@foo.com#?#</a></p>
<p><a>test</a></p>
<p>test6</p>
<p>test5</p>

Needs to be narrowed down to a version or a customization at least.

comment:7 @Denis-de-Bernardy6 years ago

yaya, that's in the comment itself when it gets displayed. my dump is looking at it before we count the links, in check_comment(), which is the stage where we're supposed to filter out comment spam.

adding some null chars in there yields some interesting side effects, btw.

comment:8 @miqrogroove6 years ago

<a>test</a> really shouldn't bother anyone, which is why the filter is for href.

comment:9 @miqrogroove6 years ago

Hey Denis, it looks like one of us managed to break the comment feed on my test site. I'll find out what happened and post some details for this ticket :) If the bug isn't confined to input issues then I'll open a third ticket specifically for the feed system.

comment:10 @miqrogroove6 years ago

Looks like it was me -_-

The XSS Cheat Sheet from ha.ckers.org caused the feed validator to throw "undefined entity".

On to debugging...

comment:11 @miqrogroove6 years ago

I submitted the feed issue to the security list so it can be discussed privately.

comment:12 @hakre5 years ago

Related: #12284

comment:13 @nacin5 years ago

I've closed #11830. Is modifying the regex as described there (we have at least three options) fine for this ticket?

comment:14 @miqrogroove5 years ago

I still don't see a bug here. What's the question exactly?

comment:15 @nacin5 years ago

  • Milestone changed from 2.9.3 to 3.0
<a	
	href	=	
	http://foo:bar@foo.com#?#

<a
	href	=	http://foo.com?\"'

>test</a>

Is technically counted as one link.

We can change the regex in one of three ways (documented in #11830), and all would fix that. I otherwise see no bug.

comment:16 @miqrogroove5 years ago

Oh, I thought this was a kses ticket.

Is technically counted as one link.

WordPress only outputs one link, based on my testing.

comment:17 @nacin5 years ago

  • Milestone 3.0 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Further testing, I can't come up with anything here.

Note: See TracTickets for help on using tickets.