Sandbox security escalation allowing users to read configuration

I recently discovered that when using the sandbox of Twig, the global variables added by Laravel are still available. By default, the framework adds 'app' as a shared variable in the ViewServiceProvider.

The problem with this is that although by itself app, as an instance of Application is quite harmless, the Container instance which it inherits is not.
Twig's sandbox requires you to add the magic __isset() method to your classes in order to allow the sandbox' security policy to check if the method or property you are calling on your class is allowed. Laravel's Container class does not have this method, meaning that the __get() method can be used freely within the sandbox, regardless of the given security policy.

The Application class contains all instances for the application, including the configuration one. This configuration can be requested by simply calling $application->config, which will use the magic get method to fetch the config repository, which in turn allows you to request any value from the current configuration as it implements ArrayAccess (unfortunately #362 doesn't fix this as it's not a Model).

Tl;dr

You can read the configuration of any Laravel application from within Twig's sandbox by using the global app variable added by the framework itself.

For example:

{% sandbox %}
    {% include(template_from_string(user_template) only %}
{% endsandbox %}

with user_template being the following:

The key is {{ app.config.app.key }}

would produce The key is base64:your_app_key..., while using the config_get() function would result in a Twig SecurityException being thrown as intended.

I don't suppose many users of this package are even using the sandbox or care about this issue but my proposed solution for now is to remove the app variable from the global context by either overriding it locally for your template or from within one of your app's ServiceProviders:

{% sandbox %}
    {% include(template_from_string(user_template) with {app: null } only %}
{% endsandbox %}

Or in a ServiceProvider:

public function boot(): void {
    View::share('app', null);
}

Again, I don't know how big of an issue this really is with this package itself but I thought I'd leave it here anyway so it can either be documented somewhere or discussed.

Well we should probably fix that indeed. Not sure how though.

@MaxKorlaar Thanks for logging this. If you have any thoughts on the below, please comment :)

@barryvdh We'd have to handle it here -

TwigBridge/src/Node/GetAttrNode.php

Line 132 in 2054abd

    
           if (Template::METHOD_CALL !== $type and is_a($object, 'Illuminate\Database\Eloquent\Model')) {

It's been a while since I looked at this but could we not just change is_a($object, 'Illuminate\Database\Eloquent\Model') to $object instanceof ArrayAccess. The is_a code has been here pretty much sicne the beginning. I've just copied it around in my previous PRs.

Not sure why twigphp/Twig#1863 was rejected / closed which did what I'm suggesting. Possibly - twigphp/Twig#106 (comment)

See #398

I've decided to just override base_template_class when I'm using a security policy.

abstract class Template extends \Twig\Template
{
    /**
     * {@inheritdoc}
     */
    public function display(array $context, array $blocks = [])
    {
        \Twig\Template::display($context, $blocks);
    }
}