Potential security issue in the Templace class

[dhrrgn]

Was looking through here to see what issues the (not so helpful) commenter (here: http://blog.phpdeveloper.org/?p=504#comment-687279) saw as an issue in the Template class. I did notice one thing that could cause potential security issues.

In Shield\Template on line 205, you extract the properties, then load the template in like so:

<?php

extract($this->_properties);
ob_start();
include_once $templateFile;
return ob_get_clean();

The issue here is 2 fold:

1. You are extracting the properties into the current scope. This COULD cause issues if the developer stupidly overwrites your local variables, however, you can help avoid this by better scoping and less common local variable names.
2. (this is the "big one") You include the template inside the scope of the class. This means that the template has access to the current Template instance. Since it extends from Base, that means they have access to the DiC, which means they have access to everything.

These can both be avoided (the first one not entirely, but can help avoid accidental var overwrites) by moving the template inclusion into it's own scope. Prior to PHP 5.4 I used a simple closure for this, since the closure had no access to $this, but now in 5.4, Closures have access to $this.

In FuelPHP 2.0, we solved this issue (which was actually a side-benefit of the original intention of doing this) by moving the parsing into it's own "driver". This has the main benefit of allowing multiple parsers (e.g. Plain PHP, Twig, etc), but has the benefit of also limiting it's scope immensely.

See here for how we did it: https://github.com/fuelphp/kernel/blob/master/classes/Fuel/Kernel/Parser/Php.php

We still use my original safe-guard closure (which I always call a "cleanRoom"), this is not entirely necessary, but adds an extra layer of "safeness" with no overhead. We also name out local variables inside the clean room so that the chances they get overwritten by accident very very low.

[dhrrgn]

Forgot to mention, this is not a security issue by itself really, as it requires an oversight on the developers end to send unsanitized user input through to the template.

[dhrrgn]

After thinking for another second, you could use the EXTR_SKIP flag in extract() to avoid the accidental overwrites.

[ircmaxell]

With 5.4, you could also just bind the closure to new StdClass. That way, $this is still defined, but useless to the template layer... Quite a simple way of handling that potential issue...

[enygma]

updated this in commit c7ffc06

thanks for the recommendations!