[wp-trac] Re: [WordPress Trac] #4748: Unprivileged users can perform some actions on pages they aren't allowed to access

[WordPress Trac]

#4748: Unprivileged users can perform some actions on pages they aren't allowed to
access
----------------------+-----------------------------------------------------
 Reporter:  xknown    |        Owner:  anonymous
     Type:  defect    |       Status:  new      
 Priority:  normal    |    Milestone:  2.2.3    
Component:  Security  |      Version:  2.2.2    
 Severity:  normal    |   Resolution:           
 Keywords:            |  
----------------------+-----------------------------------------------------
Comment (by 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
         a. To write down a definition what must be in `$PHP_SELF`.
         a. To create a function doing the job and returning the correct
 value by definition
         a. 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[[BR]]
         m.2) The request by the browser[[BR]]
         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?[[BR]]
         q.2) Which parts of a Request are part of `$PHP_SELF`?[[BR]]
         q.3) Should `$PHP_SELF` be considered as safe?[[BR]]
         q.4) Where is `$PHP_SELF` used within the application
 sourcecode.[[BR]]
         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"
  1. Logged in with that User
  1. Navigated to `/wp-admin/themes.php/index.php`
  1. I read the Message: "You do not have sufficient permissions to access
 this page."
  1. Navigated to `/wp-
 admin/themes.php/index.php?action=activate&template=classic&stylesheet=classic`
  1. 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
  1. See lines 22-51 inside wp-settings.php regarding
 $_SERVER["REQUEST_URI"].

-- 
Ticket URL: <http://trac.wordpress.org/ticket/4748#comment:1>
WordPress Trac <http://trac.wordpress.org/>
WordPress blogging software