WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 3 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 2 years ago.
21974.unit-test.patch (943 bytes) - added by SergeyBiryukov 2 years ago.
21974.2.patch (537 bytes) - added by SergeyBiryukov 23 months ago.
21974.3.patch (753 bytes) - added by SergeyBiryukov 23 months ago.

Download all attachments as: .zip

Change History (19)

@SergeyBiryukov2 years ago

comment:1 @SergeyBiryukov2 years 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 @helenyhou2 years ago

  • Keywords has-patch added

comment:3 follow-ups: @nacin2 years 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 @SergeyBiryukov23 months ago

#23853 was marked as a duplicate.

@SergeyBiryukov23 months ago

@SergeyBiryukov23 months ago

comment:6 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov23 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 23 months ago by SergeyBiryukov (next)

comment:7 @chengas12323 months ago

#23853 was marked as a duplicate.

comment:8 in reply to: ↑ 3 @CoenJacobs21 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: @markjaquith21 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 @markjaquith21 months ago

This needs a security review.

comment:11 @nacin20 months ago

  • Keywords commit added

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

comment:12 @nacin20 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 @nacin20 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 @mdawaffe20 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.

comment:15 in reply to: ↑ 6 @miqrogroove3 months ago

Replying to SergeyBiryukov:

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.

For future reference, the correct encoding is %3a or %3A which is not equivalent to the colon HTML character entity.

Note: See TracTickets for help on using tickets.