[3.3] Regression: Twig functions defined extensions, added in content no longer work.

[bobdenotter]

Details

• Relevant Bolt Version: [ 3.3 ]

Reproduction

Step 1: Install passwordprotect

step 2: edit the contenttype of Pages, for example to have allowtwig: true:

        body:
            type: html
            height: 300px
            allowtwig: true

Step 3: Add {{ passwordform() }} to a page:

Step 4: Expect to see the form in the frontend, but see this instead:

System log says:

@carson, can you look into this?

[CarsonF]

Where is passwordform function defined?

[rossriley]

https://github.com/bolt/PasswordProtect/blob/master/src/PasswordProtectExtension.php#L50-L64

I've just run into this one too on another extension.

[CarsonF]

This is expected behavior.

passwordform needs to be added to the $app['twig.sandbox.policy.functions'] list either by you or the extension.

[bobdenotter]

Does this need to be done manually? If so, this is a (small) Breaking Change, imho, and it also needs to be documented properly.

[CarsonF]

There is logic implemented to handle BC with old ways of specifying "safe". Such as safe = true on function options. Or the method isSafe defined and returning true.

The extension does neither of these.

Additionally is_safe_callback is incorrectly used. It takes a callable not a boolean. And it does nothing when is_safe is specified. Also is_safe is specified but the function returns Twig_Markup which is redundant. Come on people.

[bobdenotter]

I don't think that's the entirety of the problem.. After I fix the safe's, what breaks the rendering then, is this:

Rendering a record Twig snippet failed: Function "form_widget" is not allowed.

How would I add that function? Something like this doesn't work:

    protected function registerTwigFunctions()
    {
        $app = $this->getContainer();

        $app['twig.sandbox.policy']->addAllowedTag('form_widget');

        return [
            'passwordprotect' => [
                [$app['passwordprotect.twig'],'passwordProtect'],
                ['is_safe' => ['html'], 'safe' => true]
            ],
            'passwordform' => [
                [$app['passwordprotect.twig'], 'passwordForm'],
                ['is_safe' => ['html'], 'safe' => true]
            ]
        ];
    }

[rossriley]

what about addAllowedFunction('form_widget')

[bobdenotter]

Thanks, @rossriley. That indeed helps.. I've made it work now, using this:

    protected function registerTwigFunctions()
    {
        $app = $this->getContainer();

        $app['twig.sandbox.policy']->addAllowedFunction('form_widget');
        $app['twig.sandbox.policy']->addAllowedProperty('app', 'session');
        $app['twig.sandbox.policy']->addAllowedMethod('session', 'get');
        $app['twig.sandbox.policy']->addAllowedProperty('formview', 'vars');

        return [
            'passwordprotect' => [
                [$app['passwordprotect.twig'],'passwordProtect'],
                ['is_safe' => ['html'], 'safe' => true]
            ],
            'passwordform' => [
                [$app['passwordprotect.twig'], 'passwordForm'],
                ['is_safe' => ['html'], 'safe' => true]
            ]
        ];
    }

Is this what we want?

[rossriley]

I'm not sure that's ideal to be honest, it inverts the responsibility putting it on the extension author.

In reality the extension author might not explicitly want to mark the functions as safe, but the site owner, depending on their threat model might be fine with anything being used in Twig.

It would be nicer if we could whitelist everything by default and allow a siteowner to restrict as needed, i'm guessing for most of us when we use allowtwig: true we're happy for site admins to have access to any extensions we've installed too.

[bobdenotter]

I think if the end-user (site implementor in this case) install an extension like passwordprotect, they will have to accept it.

We could see if we could add an option for the control freaks to list somewhere which extension allows what? But, let's not invest too much time in that, until people actually request that.

One thing that might help is more verbose notices in the system log. For example, this:

Rendering a record Twig snippet failed: Calling "get" method on a "Symfony\Component\HttpFoundation\Session\Session" object is not allowed.

Does not make it clear to most people you'll need to add this in an extension:

$app['twig.sandbox.policy']->addAllowedMethod('session', 'get');

A descriptive message in system log might go a long way, there.

[CarsonF]

@bobdenotter @rossriley First off, STOP DOING THIS:

$app['twig.sandbox.policy']->addAllowedMethod('session', 'get');

How many times do I have to say that invokes the service, and prevents further modification to the service.

---

It would be nicer if we could whitelist everything by default and allow a siteowner to restrict as needed, i'm guessing for most of us when we use allowtwig: true we're happy for site admins to have access to any extensions we've installed too.

This is a blacklist, and anyone you talk to about security says not do this. Always whitelist.

---

In reality the extension author might not explicitly want to mark the functions as safe, but the site owner, depending on their threat model might be fine with anything being used in Twig.

I agree with this.

I haven't implemented anything yet, because I wasn't sure how to make it easier for users.

Other than adding to each app parameter directly:

$app['twig.sandbox.policy.tags'] = $app->share($app->extend('twig.sandbox.policy.tags', function ($tags) {
    $tags[] = 'foo';

    return $tags;
}));
$app['twig.sandbox.policy.functions']...
$app['twig.sandbox.policy.filters']...
$app['twig.sandbox.policy.methods']...
$app['twig.sandbox.policy.properties']...

[bobdenotter]

What good is the ->addAllowedMethod() method, if we're not allowed to use it?

(not trying to be snarky, i really don't understand)

[CarsonF]

$app['twig.sandbox.policy'] = $app->share($app->extend('twig.sandbox.policy', function ($policy) {
    $policy->addAllowedMethod('');

    return $policy;
}));

I know it's verbose, everyone agrees. That's why they changed it in Silex 2.0.
Once we get there (Bolt v4.0) we can do this:

$app->extend('twig.sandbox.policy', function ($policy) {
    $policy->addAllowedMethod('');

    return $policy;
});

[CarsonF]

Think of it like this: The way you did it says "I need this service now", even though you aren't asking anything of it.

The example I gave says "Hey when someone asks for this service, change it in this way before you give it to them".

This is lazy, so if the service is never asked for that code doesn't need to run. And it allows others to add their modifications, right before it's needed.

This may seem subtle for the Security Policy. It has a larger impact though when the service has dependencies and that dependency has dependencies.

Twig for example:

$app['twig']->addFunction();

Twig now asks for Config and Config asks for Filesystem. And now I don't get to swap out "files" filesystem for an S3 version, because that came after this statement.

This is exactly why we've had so many loading problems the past two years. I think they've all been resolved now though, ResourceManager was the last one.

I hope that clarifies things. Not sure if I explained it very well.

[bobdenotter]

It does make sense if you explain it like that. The verbosity makes it look fugly and verbose though. Like you said.

I'll change them for my extension. However, this does not negate the fact that this is a breaking change between 3.2 and 3.3. At the very least we should document this properly, so that we can point people bumping into this problem to some proper docs. :-)

[CarsonF]

The verbosity makes it look fugly and verbose though. Like you said.

Which is part of why we went with the register* syntax for extension methods.

However, this does not negate the fact that this is a breaking change between 3.2 and 3.3. At the very least we should document this properly, so that we can point people bumping into this problem to some proper docs. :-)

Throwing an exception vs returning null was a known change when I was coding the sandbox. I looked into what it would take to swallow the exception and I was in Twig's compilation land. I talked it over with @GawainLynch and we quickly decided against that and the break would be ok. But it seems we dropped the ball since then communicating that to everyone else. 😔

We can definitely update the exception message to give a suggestion of what to do. Which is the question.

Maybe something like:

# config.yml

twig_sandbox_security_policy: # nicer name plz?
    # Twig functions
    functions:
        - passwordform
    # Twig filters    
    filters:
    # Twig tags
    tags:
    # PHP methods
    methods:
        - [Symfony\Component\HttpFoundation\Session\SessionInterface, get]
        # and maybe shortcut for multiple methods of same class
        - Symfony\Component\HttpFoundation\Session\SessionInterface:
            - get
            - has
    # PHP properties
    properties:

I know part of the problem is a lack of documentation and that's on me.

[bobdenotter]

Throwing an exception vs returning null

Oh, yes, getting an exception is much better than just having something "stop working". :-)

Maybe something like:

Nah, that sounds like overkill.. I've solved it like this for now:

https://github.com/bolt/PasswordProtect/blob/master/src/PasswordProtectExtension.php#L36-L47

We should document that, and make sure the error message people get is distinctive enough for people to lead to that part of the docs.

[bobdenotter]

Should we also consider making the whitelisting coarser / simpler?

From:

                $policy->addAllowedMethod('formview', 'isrendered');
                $policy->addAllowedProperty('formview', 'parent');
                $policy->addAllowedProperty('formview', 'vars');

To:

                $policy->addAllowed('formview');

[GwendolenLynch]

Should we also consider making the whitelisting coarser / simpler?

So you could add

$policy->addAllowed('access_control');

…and allow the whole service to be use in records giving editors the ability to grant "login" access to someone that load the page they wrote?

[GwendolenLynch]

Speaking of … https://github.com/bolt/PasswordProtect/blob/master/src/PasswordProtectExtension.php#L40

[bobdenotter]

This:

$policy->addAllowedMethod('session', 'get');

Is required, because the extension does:

$this->app['session']->set('passwordprotect', 1);

Don't think that's easy to circumvent.

[GwendolenLynch]

->get() versus ->set()

… but that is still a CVE worthy hole

[GwendolenLynch]

Let alone the whole session data is open the line up

[bobdenotter]

Right. Ok, nevermind making it broader. Let's keep it more verbose.

[GwendolenLynch]

Documentation for the win!

[rossriley]

Sorry, to keep being the backwards compatibility moaner but I really think this is too big a break for a minor version.

We encourage extension developers to add >=3.0, <4.0 to their compatibility and it's not going to be feasible to warn every extension developer, marketplace or application level that 3.3 is going to break their extensions unless they release a new version adding all their Twig functions like Bob has done.

I agree with the concept of having a separate Twig sandbox, I'm not really sure that extension developers should have to care about it at all, I guess in some cases if you were doing something you knew wasn't suitable for general use then you could reasonably say don't let this near the general public... most Twig functions that are added though are just to do general stuff like output a tracking tag, output social links, we often provide things like prices that come from a separate db etc, they might be used in a mix of templates and in free copy too.

So is there anything we can do to patch this in the 3.x series, preferably letting Twig function normally but if not doing something to whitelist all extension functions/filters as they are added?

We can obviously be more specific about how things should work when we are bumping up a version, but I'm a bit concerned that a semver composer update is going to break a lot of functionality that depends on every single extension author auditing their code.

[bobdenotter]

I can go either way on this. I see why it's a worthwhile effort, and I wouldn't mind putting in some effort to get a number of extensions updated. Especially if the warning is descriptive to know what to add.

adding all their Twig functions like Bob has done.
something to whitelist all extension functions/filters as they are added?

It's not that, exactly. In the case of my passwordprotect extension, the two Twig functions it adds aren't the problem. It was breaking on other things, like form_widget, which aren't added by the extension itself.

[CarsonF]

@rossriley that's not even what we did 3.0-3.2. If the function/filter was added with a ['safe' => true] (not to be confused with is_safe which is for escaping), or the extension had an isSafe method that returned true (that goes back to 2.x days) then the function/filter was added to the safe_twig environment. So if they didn't do that they would still get an exception saying the function doesn't exist.

Unless some where was being rendered with the main twig instead of safe_twig and now it's being rendered with the sandbox...

Also, like bob said we can't know all the use cases magically. However, we could go through and add to the whitelist ourselves. There's already a substantial default list here

[ross]

@ross

I assume you're looking for @rossriley 😀

[CarsonF]

@ross AH! Sorry man. That's what I get for replying before coffee 🤦‍♂️

[bobdenotter]

We've decided to not revert the work done wrt SecurityPolicy, and instead mitigate the slight BC break by adding better notices for developers to fix it in their extensions. (see #6504, for a WIP)