Sandbox SecurityPolicy bypass for rendering properties on ArrayAccess objects
uphlewis opened this issue · 3 comments
Hi, I know this isn't a new issue, and i know it has already been discussed before #106
But i really think that the approach ought to be re-thought for ArrayAccess objects.
I understand the argument that ArrayAccess objects should be treated as arrays #106 (comment), yes that makes total sense when rendering a template- however we know that these objects aren't really arrays...
The author of the ArrayAccess class could build it in such a way that it becomes a security risk when used in a Twig template, because it could/does allow access to the object's properties in contravention with the Sandbox SecurityPolicy.
One big example are Laravel's Eloquent Models.
As it currently stands, if an Eloquent Model object is passed as context to a template, it is trivial to traverse its relations to access a completely different Model object, which the app developer may have never intended.
Even worse, if using Laravel 5.2 (not tested in other versions, but may still be true today) by calling {{model.save}} in a template, the $model->save() method is actually called under the hood when you access $model['save'] (which saves the model's attributes in the database). Who knows what else the template author could get up to by exploiting similar techniques.
I know, it isnt Twig's problem that Laravel chooses/chose to build this much (IMO dangerous) magic into Eloquent Models, but i think it is Twig's responsibility to ensure that its users don't fall into this dangerous trap when using Sandboxing.
Whether they implement ArrayAccess or not, objects are objects, and since Twig can't know what an object has the potential to do with its implementation of that interface, those objects shouldn't be naively treated as arrays, and allowed to bypass the security sandboxes are supposed to provide.
Currently its very easy to slip into a false sense of security when using sandbox mode with a strict security policy.
I don't have the answer to what should be the best approach here, but the current logic seems very risky to me.
In my own implementation, i'm supplementing sandboxed templates by converting any ArrayAccess objects to actual arrays in the context params. As far as i can tell that's the only option currently.
So, you are saying that accessing $model['save'] triggers the save() method on $model? If that's the case, there is nothing we can do. What you describe, converting the objects to an array, seems like the "best" option here.