New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential XSS issue with add_query_arg() and remove_query_arg() #39

Closed
quassy opened this Issue Apr 24, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@quassy
Contributor

quassy commented Apr 24, 2015

As you may know quite recently there was news that WordPress plugins could suffer from a XSS vulnerability if they use add_query_arg() and remove_query_arg() without properly sanitizing the data. When the optional third parameter of these functions is omitted, $_SERVER['REQUEST_URI'] is used unescaped, more info here.

Checking your source it seems the functions are used in the following lines of feedwordpress.php:

I am not actually sure if FeedWordPress is vulnerable but I think it should be looked at and esc_url() or esc_raw_url() be added.

@talgalili

This comment has been minimized.

Show comment
Hide comment
@talgalili

talgalili Apr 24, 2015

Hi Quassy,
Do you know if WP 4.1.3 is immune to this issue?

Hi Quassy,
Do you know if WP 4.1.3 is immune to this issue?

@quassy

This comment has been minimized.

Show comment
Hide comment
@quassy

quassy Apr 24, 2015

Contributor

I don't think so. As I understand it the two functions were never meant to escape their inputs / outputs. It was a bug in the documentation which led plugin developers to falsely assume it does and resulted in implementing this security hole unknowningly.

Background: Due to a now-fixed ambiguity in the documentation for the add_query_arg() and remove_query_arg() functions, many plugins were using them incorrectly, allowing for potential XSS attack vectors in their code.

Release notes / blog posts of 4.2 and 4.1.3 don't mention that the issue was fixed, while 4.1.2 mentions it and calls on developers to fix their plugins.

Contributor

quassy commented Apr 24, 2015

I don't think so. As I understand it the two functions were never meant to escape their inputs / outputs. It was a bug in the documentation which led plugin developers to falsely assume it does and resulted in implementing this security hole unknowningly.

Background: Due to a now-fixed ambiguity in the documentation for the add_query_arg() and remove_query_arg() functions, many plugins were using them incorrectly, allowing for potential XSS attack vectors in their code.

Release notes / blog posts of 4.2 and 4.1.3 don't mention that the issue was fixed, while 4.1.2 mentions it and calls on developers to fix their plugins.

@talgalili

This comment has been minimized.

Show comment
Hide comment
@talgalili

talgalili Apr 24, 2015

VERY risky stuff.
Since radgeek does not update the plugin often, any chance you'd know how to fix this?

VERY risky stuff.
Since radgeek does not update the plugin often, any chance you'd know how to fix this?

@quassy

This comment has been minimized.

Show comment
Hide comment
@quassy

quassy Apr 24, 2015

Contributor

I am by no means a WordPress or plugin developer, so take this with two spoons of salt and pepper!... I don't actually know what's happening with $sendback at line 1287. I have the feeling the variable is only declared to never be used again, so what's the point? Still I would change the line to

$sendback = esc_url( remove_query_arg( array('trashed', 'untrashed', 'deleted', 'zapped', 'unzapped', 'ids'), $sendback ) );

1327:

wp_redirect( esc_url_raw( add_query_arg( array('zapped' => 1, 'ids' => $post_id), $sendback ) ) );

1339:

wp_redirect( esc_url_raw( add_query_arg( array('unzapped' => 1, 'ids' => $post_id), $sendback ) ) );

So basically adding ´esc_url()´ every time the URL might be printed somewhere and adding ´esc_url_raw()´ every time the URL is used for a header.

Contributor

quassy commented Apr 24, 2015

I am by no means a WordPress or plugin developer, so take this with two spoons of salt and pepper!... I don't actually know what's happening with $sendback at line 1287. I have the feeling the variable is only declared to never be used again, so what's the point? Still I would change the line to

$sendback = esc_url( remove_query_arg( array('trashed', 'untrashed', 'deleted', 'zapped', 'unzapped', 'ids'), $sendback ) );

1327:

wp_redirect( esc_url_raw( add_query_arg( array('zapped' => 1, 'ids' => $post_id), $sendback ) ) );

1339:

wp_redirect( esc_url_raw( add_query_arg( array('unzapped' => 1, 'ids' => $post_id), $sendback ) ) );

So basically adding ´esc_url()´ every time the URL might be printed somewhere and adding ´esc_url_raw()´ every time the URL is used for a header.

@talgalili

This comment has been minimized.

Show comment
Hide comment
@talgalili

talgalili Apr 24, 2015

Well, let's hope someone who knows more then both of us will give this a look :)

Thank you very much for the report! 👍

Well, let's hope someone who knows more then both of us will give this a look :)

Thank you very much for the report! 👍

@radgeek radgeek closed this in 8ad4dee Apr 26, 2015

radgeek added a commit that referenced this issue Apr 26, 2015

spinx added a commit to business-of-fashion/feedwordpress that referenced this issue May 15, 2015

This should fix #39
Quote: 

> Background: Due to a now-fixed ambiguity in the documentation for the add_query_arg() and remove_query_arg() functions, many plugins were using them incorrectly, allowing for potential XSS attack vectors in their code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment