get_pagenum_link() needs esc_url()

[guigouz]

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

[scribu]

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

[scribu]

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.

[scribu]

Related: #13051

[guigouz]

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 ?

[westi]

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

[scribu]

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.

[nacin]

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.

[emartin24]

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.

[emartin24]

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.

[kawauso]

Patch get_pagenum_link() on return

[kawauso]

Patch get_next_posts_page_link() and get_previous_posts_page_link() on return

[kawauso]

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.

[ryan]

An optional $escape arg that defaults to true?

[nacin]

In [20685]:

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

[markjaquith]

In [21084]:

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