Closed dhrrgn closed 11 years ago
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.
After thinking for another second, you could use the EXTR_SKIP
flag in extract()
to avoid the accidental overwrites.
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...
updated this in commit https://github.com/enygma/shieldframework/commit/c7ffc06779974b60c2671f739934df00cbd8ffc6
thanks for the recommendations!
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:
The issue here is 2 fold:
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.