WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 9 months ago

Last modified 9 months ago

#21974 closed defect (bug) (fixed)

esc_url() doesn't allow protocol-relative URLs with colons

Reported by: SergeyBiryukov Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch commit
Focuses: Cc:

Description

This doesn't work:

wp_enqueue_style( 'twentytwelve-fonts', "//fonts.googleapis.com/css?family=Open+Sans:400italic,700italic,400,700", array(), null );

The colon is the culprit. wp_kses_bad_protocol() reduces the URL to:

400italic,700italic,400,700

So esc_url() returns an empty string:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/formatting.php#L2572

Attachments (4)

21974.patch (518 bytes) - added by SergeyBiryukov 19 months ago.
21974.unit-test.patch (943 bytes) - added by SergeyBiryukov 19 months ago.
21974.2.patch (537 bytes) - added by SergeyBiryukov 12 months ago.
21974.3.patch (753 bytes) - added by SergeyBiryukov 12 months ago.

Download all attachments as: .zip

Change History (18)

SergeyBiryukov19 months ago

comment:1 SergeyBiryukov19 months ago

21974.patch only calls wp_kses_bad_protocol() if the URL doesn't start with a slash. There's a similar detection earlier:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/formatting.php#L2559

Perhaps wp_kses_bad_protocol() should be patched instead?

comment:2 helenyhou18 months ago

  • Keywords has-patch added

comment:3 follow-ups: nacin18 months ago

  • Component changed from General to Formatting
  • Milestone changed from 3.5 to Future Release

Technically, a colon is a "reserved" character which means outside of its official use, it must be encoded. This reminds me of #16859.

comment:5 SergeyBiryukov12 months ago

#23853 was marked as a duplicate.

SergeyBiryukov12 months ago

SergeyBiryukov12 months ago

comment:6 in reply to: ↑ 3 SergeyBiryukov12 months ago

  • Milestone changed from Future Release to 3.6

Replying to nacin:

Technically, a colon is a "reserved" character which means outside of its official use, it must be encoded.

An encoded colon (: or :) doesn't work either.

wp_kses_bad_protocol_once() splits by any of those values:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/kses.php#L1053

And esc_url() still returns an empty string.

I guess we shouldn't call wp_kses_bad_protocol() at all for a relative URL. Refreshed the patch.

21974.2.patch just fixes the issue.

21974.3.patch also skips the strtolower() check, which is redundant in this case.

Version 0, edited 12 months ago by SergeyBiryukov (next)

comment:7 chengas12312 months ago

#23853 was marked as a duplicate.

comment:8 in reply to: ↑ 3 CoenJacobs11 months ago

Replying to nacin:

Technically, a colon is a "reserved" character which means outside of its official use, it must be encoded. This reminds me of #16859.

It does the same with URLs containing the colon as a split between URL and port.

comment:9 follow-up: markjaquith10 months ago

Weird quirk: the bug does not trigger when the path is "/"

Triggers the issue: //example.com/foo?foo=abc:def

Does not: //example.com/?foo=abc:def

See: [1296/tests]

comment:10 markjaquith10 months ago

This needs a security review.

comment:11 nacin9 months ago

  • Keywords commit added

21974.3.patch looks good to me. Asking duck_ and mdawaffe to examine closer.

comment:12 nacin9 months ago

Per duck_, this is good. A few other security libraries don't do *anything* when the first character is /, #, or ? — which reminded us both of #22951, the esc_url() performance ticket.

comment:13 nacin9 months ago

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

In 24642:

Skip protocol checking in esc_url() when we are dealing with a relative URL. Prevents munging of colons in paths and query strings, when present in a protocol-relative URL.

props SergeyBiryukov.
fixes #21974.

comment:14 in reply to: ↑ 9 mdawaffe9 months ago

Replying to markjaquith:

Weird quirk: the bug does not trigger when the path is "/"

Yeah - there's a strange exception is kses for this case.

Note: See TracTickets for help on using tickets.