Geeklog-Core / geeklog

Geeklog - The Secure CMS.
https://www.geeklog.net
24 stars 19 forks source link

Saving of Staticpage in Editor can result in problems if contain PHP code. Maybe other issues where eval is used with Templates? #1071

Closed eSilverStrike closed 2 years ago

eSilverStrike commented 4 years ago

Ran into this issue in the Autotags plugin which also affects Geeklog itself where ever it uses the PHP eval function.

https://github.com/Geeklog-Plugins/autotags/issues/13

For example in the Staticpage plugin if an autotag contains PHP code and if PHP 7 or higher we evaluate the code using PHP eval function to determine if any errors in the code. If there is we inform the user. See: https://github.com/Geeklog-Core/geeklog/issues/1038

One side affect I notice is that when the eval happens if the php code in the eval string contains the same variable names as the function it is running in, the contents of those variables in the function will be updated (this includes global variables the funtion has access to).

This obviously could result in some major errors.

The chances of this happening is not huge but what can we do about this? Can we run eval within it's own container to protect the variables in the function from being manipulated?

I do like having the code evaluated before save and this is only used by admins so should we just issue warnings in the docs then to use UNIQUE variable names for php in the staticpage or you could result in unintended consequences?

We did add a config option to disable this in Staticpages on save eval check incase they add code like a redirect which would then actual not save the page so we do already have these type of warnings for eval anyways...

Does this issue affect templates as well since they are evaluated as PHP. I don't think we need to worry about it but should just check it out just to be safe and add in any warnings if need be.

eSilverStrike commented 2 years ago

Staticpages uses COM_handleEval for evaluating PHP in pages (that are not templates) so it should be relatively safe from this issue (except for the variables used within its own function).

Staticpages also can use the template caching library so like Themes it also allows the use of PHP. In this case COM_handleEval is not used as that is an outside function as the code we need to evaluate contains references to the template class ($this->...)

Therefore I have added a warning to the Wiki about this possible issue and updated the Static Page Plugin docs included with Geeklog.

http://wiki.geeklog.net/Caching_Template_Library#PHP_Code_in_a_Template_Files