WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#14556 closed enhancement (fixed)

get_pagenum_link() needs esc_url()

Reported by: guigouz Owned by: nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.0.1
Component: Security Keywords: has-patch
Focuses: Cc:

Description

We're using get_pagenum_link() to build a page navigation instead of older/newer posts only. We've found this vulnerability on multiple sites, here's an example

http://robertbasic.com/blog/?%3E%22'%3E%3CScRiPt%3Ealert(428017202033)%3C/ScRiPt%3E

Attachments (2)

14556.diff (349 bytes) - added by kawauso 4 years ago.
Patch get_pagenum_link() on return
14556.later.diff (597 bytes) - added by kawauso 4 years ago.
Patch get_next_posts_page_link() and get_previous_posts_page_link() on return

Download all attachments as: .zip

Change History (20)

comment:1 @scribu5 years ago

  • Component changed from General to Security

comment:2 @scribu5 years ago

  • Keywords reporter-feedback added

I'm not able to reproduce. Please paste the code you're using to generate the page links.

comment:3 @scribu5 years ago

  • Severity changed from critical to normal

You need to use esc_url() before outputing what get_pagenum_link() returns.

Decreasing severity because it's used properly everywhere in core.

Should probably add a warning somewhere in the function's doc.

comment:4 @scribu5 years ago

Related: #13051

comment:5 follow-up: @guigouz5 years ago

The code is here - http://robertbasic.com/blog/wordpress-paging-navigation/
If you're not using mod_rewrite, wouldn't esc_url() mess with navigation ?

comment:6 @westi5 years ago

In future the correct way to report potential security issues is detailed here: http://codex.wordpress.org/Security_FAQ

comment:7 in reply to: ↑ 5 @scribu5 years ago

Replying to guigouz:

If you're not using mod_rewrite, wouldn't esc_url() mess with navigation ?

Nope. esc_url() is for escaping any type of URL.

comment:8 @scribu5 years ago

  • Keywords reporter-feedback removed
  • Summary changed from get_pagenum_link() vulnerable to XSS attacks to get_pagenum_link() needs esc_url()

comment:9 @nacin4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Unless this gets fixed at a lower level, i.e. #13051, any functions that return links will return them unescaped. Only functions that return HTML with a link should have the link escaped.

comment:10 @emartin244 years ago

I ran into this same issue with my pagination plugin, WP-Paginate.

It doesn't seem to be an issue with get_comments_pagenum_link(), but unless I wrap get_pagenum_link() with esc_url(), I am able to create an XSS vulnerability.

I can see how it might be a complicated issue, but I would expect WordPress to sanitize values returned from it's functions, or at the very least provide a huge warning to theme/plugin developers of potential issues with certain functions.

comment:11 @emartin244 years ago

  • Cc eric@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Given that #13051 refers to links that come from the database/options, I don't think this issue is the same.

The problem is that someone can pass a query string that when used in conjunction with get_pagenum_link() can create a XSS vulnerability.

In the get_pagenum_link function in wp-includes/link-template.php, what about changing:

$result = $base . $request . $query_string;

to:

$result = esc_url($base . $request . $query_string);

or:

$result = $base . $request . htmlspecialchars($query_string);

I will send an email to security@…, but wanted to re-open this issue for consideration.

comment:12 @ocean904 years ago

  • Milestone set to Awaiting Review

@kawauso4 years ago

Patch get_pagenum_link() on return

@kawauso4 years ago

Patch get_next_posts_page_link() and get_previous_posts_page_link() on return

comment:13 @kawauso4 years ago

Attached a couple of patches that patch get_pagenum_link() (the initial function) and the functions that use it using esc_url(). IMO it'd be best to patch the former since calls don't necessarily use the latter functions.

comment:14 @kawauso4 years ago

  • Keywords has-patch added

comment:15 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.4
  • Type changed from defect (bug) to enhancement

comment:16 @ryan3 years ago

An optional $escape arg that defaults to true?

comment:17 @nacin3 years ago

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

In [20685]:

Always escape the output of get_pagenum_link(). fixes #14556.

comment:18 @markjaquith3 years ago

In [21084]:

Always escape the output of get_pagenum_link(). fixes #14556 for the 3.3 branch.

Note: See TracTickets for help on using tickets.