Unprivileged users can perform some actions on pages they aren't allowed to access

[xknown]

You control the access to administration pages on menu.php based on the value of $pagenow, however this variable can be manipulated via PATH_INFO.

if ( preg_match('#([^/]+\.php)$#', $PHP_SELF, $self_matches) ) {
	$pagenow = $self_matches[1];
} elseif ( strpos($PHP_SELF, '?') !== false ) {
	$pagenow = explode('/', $PHP_SELF);
	$pagenow = trim($pagenow[(sizeof($pagenow)-1)]);
	$pagenow = explode('?', $pagenow);
	$pagenow = $pagenow[0];
} else {
	$pagenow = 'index.php';
}

Proof of Concept:

1. Log in to WP (a subscriber user is okay)
2. Access to http://vulnerable/wp/wp-admin/themes.php/index.php
3. Switch the current theme (you need to replace /wp-admin/themes.php/themes.php?action=... by /wp-admin/themes.php/index.php?action=...)

As you can see, this bug lets unprivileged users to switch current theme, de/activate plugins, etc.

[hakre]

I made an analysis of CVS source regarding the reported problem and created a fix:

Analysis:

• noted code above is located in wp-includes/vars.php line line 3 to 14 inside the global php main() function.
• vars.php is included or required in the following position(s):

1.) wp-settings.php line 216

• $PHP_SELF is initialized on line 62ff:

  // Fix empty PHP_SELF
  $PHP_SELF = $_SERVER['PHP_SELF'];
  if ( empty($PHP_SELF) )
  	$_SERVER['PHP_SELF'] = $PHP_SELF = preg_replace("/(\?.*)?$/",'',$_SERVER["REQUEST_URI"]);
  

• as stated above the value of $PHP_SELF can be manipulated by the user. Since both, the value of $_SERVER['PHP_SELF'] and $_SERVER["REQUEST_URI"] are based on user input, this is true (REF:1). Additionally you should be aware of other code modifying these values (REF:2).
• Because the sourcecode is not documented you can not say at this point which content has to be put into $PHP_SELF sothat it can be properly sanitized.
• My suggestion is
  1. To write down a definition what must be in $PHP_SELF.
  2. To create a function doing the job and returning the correct value by definition
  3. Until a) and b) has been done to create a fallback in vars.php for the admin
• What you can say for shure is that $pagenow only contains the filename of the .php-file not containing the path to it. E.G. not wordpress/index.php just index.php.

Possible Fallback Strategies:

Some possile Fallback strategies which came into my mind to hotfix this Issue. Most of them need to be discussed anyway:

• Force the useragent to use at least index.php within the Request when inside wp-admin. (Does this contain a hole? Querystrings or similar should at least NOT attached within the redirect here.)
• Make clear that at least the directory wp-admin is followed by one .php file-name only.
• Since the UA is forced into a .php request and we know that there are no further subdirs this can be sanitzed. If it can not be said which file, an empty string should be returned.
• Since there is a specified set of files used within the admin, a whitelist can be created to compare against.
• $pagenow is used as a place where to find the result. So this is a nice place to inject the hotfix to without touching the rest of the app.

Solving…

Because this is a hotfix only, it targets especially the describben problem above only. Any Usage of $PHP_SELF and $_SERVER['PHP_SELF'] is still dangerous and the places within the source should be clearly analyzed AFTER it has been said for once what MUST BE and IS in $PHP_SELF / $_SERVER['PHP_SELF'] (in the meaning of "inside wordpress" not "PHP in general"):

m.1) The name of the script relative to document root
m.2) The request by the browser
m.3) Specify else

Questions that could be good to be answered in this clarification process:

q.1) Does $PHP_SELF contains stuff of the queryinfo part or not?
q.2) Which parts of a Request are part of $PHP_SELF?
q.3) Should $PHP_SELF be considered as safe?
q.4) Where is $PHP_SELF used within the application sourcecode.
q.5) Is it a good practise to name vars the same like the tainted and known to be insecure variables like $_SERVER['PHP_SELF']? Doesn't this irritate developers and doesn't it make things complicated and more error-prone?

First Hotfix Implementation:

I made this quite short and did not redirect. This could produce sideeffects and create critical holes sothat no-.php-file is transposed into index.php internally only. Everything unmatching is leading into a wp_die('Request Error. Please contact the Administrator.'). Logical problems will result in an Errorcode which is documented inside the according comments. If you encounter such codes while testing, visit trac and discuss. Do not short-ciruit here. Take care.

WARNING: This does only hotfix the describben Issue and will hide the proof of concept. It can not circumvent the existing design errors in wordpress lacking of missing documentation.

Fix Testing Report:

1. Created a User of Role "Subcriber"
2. Logged in with that User
3. Navigated to /wp-admin/themes.php/index.php
4. I read the Message: "You do not have sufficient permissions to access this page."
5. Navigated to /wp-admin/themes.php/index.php?action=activate&template=classic&stylesheet=classic
6. I read the Message: "You do not have sufficient permissions to access this page."

All tests performed successfully. Proof of Concept does not work any longer. Please hack deeper now.

Additional Information:

$pagenow is used on multiple places within the source and by both admin and non-admin code as far as I can see. It would make sense to open a further ticket to remove unneeded $pagenow declarations because some functions are importing $pagenow from the global scope but do not make use of the var then. This can be removed from source to make a further analysis more easy.

References:

1. ​http://blog.phpdoc.info/archives/13-XSS-Woes.html
2. See lines 22-51 inside wp-settings.php regarding $_SERVERREQUEST_URI?.

[hakre]

Security Hotfix for #4748 / admin privilege escalation

[hakre]

UPLOADED THE WRONG FILE! IGNORE.

[xknown]

A more safer approach is to check access rights on the affected files (can_user_can), because the current implementation and the patch you propose could be bypassed (i.e. try this on a Windows box /wp-admin/themes.Php/index.php).

[markjaquith]

First try

[markjaquith]

(In [6029]) Better $pagenow determination. fixes #4748

[ryan]

(In [6061]) Better determination. fixes #4748 for 2.2

[snakefoot]

This security fix should be applied to the 2.0 branch.

[westi]

Backport of $pagenow fix to 2.0

[westi]

I have attached a patch to backport the fix to 2.0 - untested.

[markjaquith]

(In [6066]) Better $pagenow determination. fixes #4748 for 2.0