Don't import arbitrary globals from the query string

Discussions and requests related to new CMSimple features, plugins, templates etc. and how to develop.
Please don't ask for support at this forums!
Post Reply
cmb
Posts: 14225
Joined: Tue Jun 21, 2011 11:04 am
Location: Bingen, RLP, DE
Contact:

Don't import arbitrary globals from the query string

Post by cmb » Wed Feb 22, 2017 1:46 pm

Hi everybody!

I just had a closer look at Prefix special page names with "xh" which is somehow related to Reliable setting of $s by the way. Anyhow, that brought me again to the issue that arbitrary global variables will be created from the query string. IMO, this is a no-go, because (a) it can easily lead to security issues and (b) we don't even know which global variables (which are bad enough per se) are there. Note that this import is a considerably weakend (because the value of the variable can't be set from the URL; instead it's always 'true') variant of register_globals, which has been removed as of PHP 5.4.0 for security reasons.

Therefore I propose to get rid of this import for CMSimple_XH 1.7. Instead, variables used by the core (such as $sysinfo) should be added to the whitelist, and their usage should be fixed (if necessary at all).

That still leaves a potential BC break regarding extensions (especially plugins) which may use this "feature". The most common issue is the plugin administration which is normally triggered by setting a variable with the name of the plugin (e.g. /?&pagemanager). However, as of XH 1.6.3 there is XH_wantsPluginAdministration which is already used by some plugins. For these plugins we could change the way how the plugin administration is triggered in a backward compatible way. Other plugins would need to be updated. In my opinion that is acceptable, considering that XH_wantsPluginAdministration() is available since over two years and XH 1.7 is still months away (at least).

A less common, but nonetheless existing, issue are other globals used by extensions in this way. This can be easily fixed though, by simply checking for isset($_GET['my_var']) instead of checking for isset($my_var) && $my_var === 'true'. So, in my opinion, this BC break is also acceptable.

Thoughts?
Christoph M. Becker – Plugins for CMSimple_XH

cmb
Posts: 14225
Joined: Tue Jun 21, 2011 11:04 am
Location: Bingen, RLP, DE
Contact:

Re: Don't import arbitrary globals from the query string

Post by cmb » Wed Feb 22, 2017 2:47 pm

I've filed this also as issue on Github and made a respective pull request.
Christoph M. Becker – Plugins for CMSimple_XH

Post Reply