#4748 closed defect (bug) (fixed)
Unprivileged users can perform some actions on pages they aren't allowed to access
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 2.0.12 | Priority: | normal |
Severity: | normal | Version: | 2.2.2 |
Component: | Security | Keywords: | has-patch has-fix security privilege-escalation |
Focuses: | Cc: |
Description
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:
- Log in to WP (a subscriber user is okay)
- Access to
http://vulnerable/wp/wp-admin/themes.php/index.php
- 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.
Attachments (4)
Change History (15)
#3
@
17 years ago
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
).
#7
in reply to:
↑ description
@
17 years ago
This security fix should be applied to the 2.0 branch.
#9
@
17 years ago
- Milestone changed from 2.3 to 2.0.12
- Resolution fixed deleted
- Status changed from closed to reopened
This ticket was mentioned in Slack in #cli by schlessera. View the logs.
6 years ago
Note: See
TracTickets for help on using
tickets.
I made an analysis of CVS source regarding the reported problem and created a fix:
Analysis:
$PHP_SELF
is initialized on line 62ff:$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).$PHP_SELF
sothat it can be properly sanitized.$PHP_SELF
.$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:
$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"):Questions that could be good to be answered in this clarification process:
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:
/wp-admin/themes.php/index.php
/wp-admin/themes.php/index.php?action=activate&template=classic&stylesheet=classic
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: