WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 23 months ago

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

Download all attachments as: .zip

Change History (19)

@SergeyBiryukov3 years ago

comment:1 @SergeyBiryukov3 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 @helenyhou3 years ago

  • Keywords has-patch added

comment:3 follow-ups: @nacin3 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 @SergeyBiryukov2 years ago

#23853 was marked as a duplicate.

@SergeyBiryukov2 years ago

@SergeyBiryukov2 years ago

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

comment:7 @chengas1232 years ago

#23853 was marked as a duplicate.

comment:8 in reply to: ↑ 3 @CoenJacobs2 years 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: @markjaquith2 years 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 @markjaquith2 years ago

This needs a security review.

comment:11 @nacin23 months ago

  • Keywords commit added

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

comment:12 @nacin23 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 @nacin23 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 @mdawaffe23 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 @miqrogroove6 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.