WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#7386 closed defect (bug) (fixed)

clean_url() shouldn't touch dollar, asterisk or single quote characters

Reported by: sambauers Owned by:
Milestone: 2.7 Priority: low
Severity: minor Version: 2.6
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

According to http://www.ietf.org/rfc/rfc1738.txt

the special characters "$-_.+!*'()," ... may be used unencoded within a URL.

Attachments (2)

clean-url-dont-escape-dollar.patch (526 bytes) - added by sambauers 6 years ago.
7386.002.diff (1.1 KB) - added by markjaquith 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 follow-up: markjaquith6 years ago

  • Milestone changed from 2.6.1 to 2.7

Leaving single quotes unescaped would be an XSS security vulnerability. I've no objection to the other characters being allowed. Punting this to 2.7

comment:2 in reply to: ↑ 1 santosj6 years ago

Replying to markjaquith:

Leaving single quotes unescaped would be an XSS security vulnerability. I've no objection to the other characters being allowed. Punting this to 2.7

Sanitizing shouldn't be done in URLs, it should be done, when the page is printed. I don't see how it would matter. If it needs it, then the url must always be contained within double quotes. That should negate the XSS vulnerability.

comment:3 follow-up: markjaquith6 years ago

Sanitizing shouldn't be done in URLs, it should be done, when the page is printed.

Most uses of clean_url() are sanitizing URLs for display.

If it needs it, then the url must always be contained within double quotes. That should negate the XSS vulnerability.

That's a silly thing to require. A href attribute contained within single quotes is valid (X)HTML.

comment:4 in reply to: ↑ 3 jacobsantos6 years ago

Replying to markjaquith:

That's a silly thing to require. A href attribute contained within single quotes is valid (X)HTML.

Yes, but invalid if the url also contains a single quote. Since single quotes are valid in URLs and therefore reasonable that they would be displayed, then it should be assumed that any given URL can have it and therefore that the href should always use double quotes to prevent invalid (X)HTML and XSS attacks.

comment:5 markjaquith6 years ago

Yes, but invalid if the url also contains a single quote.

So we can HTML-entity-encode single quotes if we're displaying the URL, leave it alone if we're not. Requiring double quotes is simply not an option. I'll make a patch.

markjaquith6 years ago

comment:6 markjaquith6 years ago

7386.002.diff leaves single-quotes alone for non-display contexts, encodes it for display context. Allows single quote to be passed in wp_redirect() contexts (i.e. using sanitize_url()), but doesn't allow for XSS in the default (display) context.

comment:7 follow-up: chrise6 years ago

  • Component changed from General to Administration

Links cannot contain high-ascii/UTF-8 characters like 'åäö' (å, ä and ö), had to find an IDN equivalent to be able to link to a specifik site:

Example: "http://www.senatåg.se/" as "Web Address" and "Sena Tåg" as "Name" -- comes out as "http://www.senatg.se/" which is not the desired url.

I'm sorry if this is the wrong place to post this but I thought this ticket would be the a proper place and in context.

comment:8 in reply to: ↑ 7 nbachiyski6 years ago

Replying to chrise:

Links cannot contain high-ascii/UTF-8 characters like 'åäö' (å, ä and ö)

clean_url rejects high-ascii characters, but it happily accept UTF-8 ones:

>>> clean_url('http://баба.com/Ïn thë Gërmän längüägë thërë ärë önly föür Ümläüts: ä, ü, ö, Ä, Ü, Ö')
http://баба.com/ÏnthëGërmänlängüägëthërëärëönlyföürÜmläüts:ä,ü,ö,Ä,Ü,Ö

comment:9 matt6 years ago

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

(In [9179]) clean_url improvements, Hat tip: markjaquith. Reviewed by nbachiyskip. Fixes #7386.

Note: See TracTickets for help on using tickets.