Originally reported by samuel.mortenson on February 20, 2017 to security.drupal.org #161890

Clear by the Drupal Security team for public resolution as a critical bug.

Problem/Motivation

Summary: Users with access to Twig files or inline Twig templates can abuse the Drupal's support for render arrays in Twig to execute arbitrary code.

===

Views has an arbitrary code execution vulnerability.

Replication steps:

  1. Enable Views
  2. As a user with the "Administer views" permission, create a new View of User Fields (any view with result rows will work)
  3. Add a "Custom text" field
  4. Enter the text "{{ {"#lazy_builder": ["shell_exec", ["touch /tmp/hellofromviews"]]} }}"
  5. Click "Update preview"
  6. See that the file "/tmp/hellofromviews" exists on your machine

This is possible as the "render_var" function is available to Twig entered through Views. As a result, users can enter arbitrary render arrays, which allows for code execution through callbacks like #lazy_builder, #process, #pre_render, #post_render, #access_callback, and potentially other keys. For this example, #lazy_builder was used as it had the most flexible callback method. It's also worth noting that "shell_exec" is used in the example as "eval" is not a function, and as a result can not be called as a part of call_user_func().

Proposed resolution

We have to basically prevent render arrays from being written directly in any template or assigned to a variable in twig and then rendered.

To accomplish this we wrap each render array in an object (extending ArrayObject that implements \Drupal\Core\Render\RenderableInterface) and override methods that would mutate the render array (like setting the value at an index).

This allows us to also make the logic in \Drupal\Core\Template\TwigExtension more consistent, since these variables are now objects that implement RenderableInterface and are treated like any other object.

=======

Range of suggested resolutions that were considered:

  • Original suggestion by samuel.mortenson:

    Instead of filtering dangerous render arrays entered this way in Views, I would suggest blocking render_array and \Drupal\Core\Template\TwigNodeVisitor outright, in a way that other modules could emulate if they too wanted to allow end-users to enter Twig through the UI.

    The attached patch adds two new services, twig_strict and twig.strict_extension, which can be used to process Twig with a restricted set of components (functions, filters, etc.). As of right now the only known vulnerability is render_var, I have only restricted that function.

    That service, in turn, can be enabled by users of the "inline_template" element, by passing the option "#strict_mode => TRUE". In the patch I had Views turn this on by default for all Views Plugins that use advanced rendering.

  • A suggestion by pwolanin:

    We are possibly doing this since render arrays are not object that can be cast to string.

    It looks like we have to basically prevent render arrays from being written directly in any template or assigned to a variable in twig and then rendered.

    Can Twig distinguish variables passed in verses created inside the template? We could possibly wrap each render array in an object (like ArrayObject that implements \Drupal\Core\Render\RenderableInterface) but might need to make it immutable inside Twig which could break existing templates or parts of the render flow.

  • Another suggestion by pwolanin that samuel.mortenson proved was not sufficient:

    We could recursively flag all render arrays going into twig as variables by setting a value for a # property that could not be set by someone creating render array in inline twig or even in a twig template with set.

    From Samuel:

    Here's an example of working around this idea in Twig, assuming "safe_render_array" is passed to Twig and already marked as safe.

    {# Add malicious render array key to existing safe array #}
    {% set safe_render_array = safe_render_array|merge({'#lazy_builder': ["shell_exec", ["touch /tmp/hellofromviews"]}) %}
    {# Copy #_safe_to_render key from existing safe array to new array, to mock that it was already marked as safe #}
    {% set nasty_array = {"#lazy_builder": ["shell_exec", ["touch /tmp/hellofromviews"]], '#_safe_to_render': safe_render_array['#_safe_to_render']} %}
    

    A real world example may be more complex, but this should illustrate the problem with relying on an array key.

  • A further suggestion

    webform for 8.x (accidentally?) disables "#lazy_builder" by adding extra properties to all render arrays. Depending on what the valid use case is for that, maybe we could do the same in Twig.

Remaining tasks

Get feedback from people using Twig on how elements of render arrays are accessed and used.

Decide on a secure approach

Implement the fix

Fix failing tests

Add more test coverage

User interface changes

none

API changes

May be some BC-breaking changes to how render arrays may be accessed or modified in templates.

Data model changes

none

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

Issue summary: View changes
samuel.mortenson’s picture

Issue summary: View changes

I'm a fan of @pwolanin's suggestion, "we have to basically prevent render arrays from being written directly in any template or assigned to a variable in twig and then rendered". If we're OK with preventing users from modifying render arrays passed to Twig, I think this is the most secure option.

My original suggestion of adding a new Twig extension that's explicitly "safe" is only desirable as it doesn't change the current behavior of how render arrays work with Twig, but isn't a generic or fool-proof solution.

lauriii’s picture

Yikes! From a perspective of a front-end developer, the original proposal of creating twig_strict service sounds better. The way people theme Drupal includes very commonly modifying and creating render arrays in a template. There is a plan to introduce this convention for Drupal core here #2821399: Theme system improvements roadmap.

samuel.mortenson’s picture

FileSize
4.71 KB

Given that feedback, here's the original twig_strict patch I submitted with the issue.

pwolanin’s picture

@laurii - The suggested alternate render service is not sufficient in my mind since it's opt-in, so and contrib may neglect to use it, and it still leaves this hole open for people who can write Twig template files. I think the goal should be to make it as close as possible to safe for untrusted users to supply Twig templates.

Also - to be clear, we are only talking about preventing you from constructing an arbitrary array in Twig and having it treated as a valid render array by the render system.

Any Twig function or helper that takes params and returns a render array or renderable object should be able to work.

My general thinking at the moment is that we will need to wrap bare render arrays in an object implementing \ArrayAccess interface and \Drupal\Core\Render\RenderableInterface

We would build the class to not allow you to set values *but you could unset to support the |without filter), and if you get a value that is an array we'd also wrap that array in a new object before returning it.

By doing that we could change \Drupal\Core\Template\TwigExtension::renderVar so we only handle render arrays that come from an object implementing \Drupal\Core\Render\RenderableInterface

samuel.mortenson’s picture

@pwolanin Supporting existing users that are already creating render arrays in Twig, and changing existing render array properties, is definitely tough. I agree that a generic solution is safer here, but if we remove this functionality we'll force users to implement preprocess functions if they want to create or modify a render array.

To support existing use cases and still use the wrapping object, I think the following would need to happen:

  1. Wrap all render arrays passed to Twig with an object
  2. Implement __get to return items in the render array, which should also be wrapped in an object if they're render arrays
  3. Implement __set to allow users to set "safe" render array properties (we would need to decide on a blacklist of unsafe properties
  4. Have the class implement the Iterator interface, to support Twig for loops and operations like array_merge (the merge filter)
  5. If an unwrapped render array is passed to the render function, filter, or node traverser, recursively unset unsafe properties

It's complicated, but unless the community can agree to break current Twig functionality, we need to have a solution that maintains safety while not breaking BC.

samuel.mortenson’s picture

Another funny option would be to not wrap the whole array, but before passing variables to Twig recursively wrap all unsafe property values in an object. Then, before rendering render arrays with the filter/function/traverser, you find all unsafe properties in the array, and if their values are objects you set them back to their original value, otherwise you unset them as they must have been set in Twig. It sounds complicated, but compared to wrapping the entire render array, might be simple to implement.

This allows us to have a generic solution as @pwolanin originally suggested, while only targeting the properties that we know are unsafe. In this way existing Twig users are only affected if, for some reason, they're setting callbacks in Twig (which I've never seen in the wild).

I'll work on a POC patch, but in the mean time let me know what you think.

pwolanin’s picture

@samuel.mortenson - if you look at the issue laurii mentioned, it's about helpers that return render elements (arrays), not creating bare render arrays in Twig.

I really don't think that creating your own bare arrays to render can be a supported use?

re: wrapping unsafe elements - we don't necessarily know in advance what those are (or what contrib might add). Still it's worth a try to see if it works.

samuel.mortenson’s picture

@pwolanin Ah, true. I didn't read through #2818121: Create render array generator that can be used in Twig so I wasn't aware that they were suggesting adding a new Twig tag with that kind of syntax. I think we need more community input to see how people are using render arrays in Twig, because it's possible that this is a feature not many people know about/have tried to use.

samuel.mortenson’s picture

Assigned: Unassigned » samuel.mortenson

Assigning this to myself while I work on a POC and test coverage.

My idea from #8 and #7 regarding "safe" render properties would be really complex, and as @pwolanin points out doesn't cover contrib adding new unsafe properties. This is the list I came up with, but there's no way to tell if it's complete:

+  public static $unsafeProperties = [
+    '#access_callback',
+    '#after_build',
+    '#ajax',
+    '#element_validate',
+    '#lazy_builder',
+    '#post_render',
+    '#pre_render',
+    '#process',
+    '#submit',
+    '#theme',
+    '#theme_wrappers',
+    '#upload_validators',
+    '#validate',
+    '#value_callback',
+  ];

Given the complexity of maintaining a property blacklist, I'm working on wrapping all variables passed to Twig in objects, which is going OK so far.

samuel.mortenson’s picture

Assigned: samuel.mortenson » Unassigned
FileSize
10.33 KB

So I worked on the "It looks like we have to basically prevent render arrays from being written directly in any template or assigned to a variable in twig and then rendered." solution, and while the solution isn't' perfect, it does protect against the originally reported vulnerability.

Overview of the POC:

- A wrapping class called "ReadOnlyArrayObject" was created, which extends the standard \ArrayObject and implements no-op versions of any method that could change its contents.

- In Drupal's Twig implementation, I made it so that all context passed into Twig is wrapped in a "ReadOnlyArrayObject" object.

- In TwigExtension, I made ::renderVar only accept ReadOnlyArrayObject objects for rendering, throwing an error if an unwrapped array is passed for rendering.

Problems:

The solution has one major flaw, which is why I'm uploading the patch but not putting the issue into review. Since we're wrapping every render array in an object, "if" statements in Twig will always evaluate to true. This is because casting PHP objects to boolean (aka evaluating them as "truthy") will always be true, and that behavior cannot be overridden. This makes \ArrayObject behave differently than array(), as (bool) array() is false, and (bool) new \ArrayObject([]) is true.

The impact of this is that every Twig template that once has a statement like this:

{# Check to see if page.featured is empty #}
{% if page.featured %}

would now need to write that as

{# Check to see if page.featured is empty #}
{% if not page.featured is empty %}

If you apply the patch and install the standard profile, you should be able to see the problem on the homepage, as the "featured" region is visible but contains no blocks.

Summary:

The impact on frontend developers and existing themes/modules may be too big to accept an all-encompassing object wrapping solution. That said, this was an interesting exercise and might be useful for future solutions to this issue.

At this point, I'm not sure what the best mitigation for this problem is. Per #11, there are a lot of render array keys that could be considered "unsafe", and even if we wanted to filter them in Twig I don't know how we would.

If we want to re-use the POC work I did here, maybe we could combine it with the Twig strict filter and have modules like Views opt-into it?

Anyway, I hope we get something out of the code I wrote. Un-assigning for now.

pwolanin’s picture

@samuel.mortenson thanks - sounds like a great start. I'll try to find time to look at extending that.

lauriii’s picture

@samuel.mortenson thanks for the POC. It looks good but as you said, the change is quite disruptive. This would require multiple changes even in Drupal core.

I discussed earlier today with @pwolanin about the use case where render arrays are being created in Twig and we both agreed that it is not a good convention to create render arrays in templates. I misunderstood that this would prevent us from creating render array generators, but this never was the intention.

I also think that immutability for render arrays would make the whole subsystem more robust. Arrays just are not the easiest tool to use build something like that. I linked another issue which includes exploring around similar things but with slightly different approach.

samuel.mortenson’s picture

Did a little more research on overriding the default PHP behavior of casting objects to boolean, and it appears that SimpleXML objects are the only exception to PHP's default behavior of always casting objects to true (ref http://php.net/manual/en/language.types.boolean.php#language.types.boole...). This RFC goes into some detail as well, and suggests allowing objets to overload their default cast behavior: https://wiki.php.net/rfc/object_cast_to_types.

In http://stackoverflow.com/a/14329778, a Stack Overflow user goes over the problem in good detail and gives an example of how you could extend SimpleXMLElement to have custom "truthy" behavior, which could be used in conjunction with the patch in #12 to get past the major flaw in the current POC.

I'll take a look at how crazy this is, and see if it can be used to improve the current patch.

samuel.mortenson’s picture

FileSize
14.1 KB
3.69 KB

I looked into using SimpleXMLElement instead of ArrayObject but it didn't pan out, I don't think we can get array-like behavior and array-like boolean casting with just the object.

Here's a really crazy solution to the problem I talk about in #12. Basically we use the TwigNodeVisitor to find every instance of a named variable in an if statement, and what that variable in a custom Twig function called "is_bool", which simply casts a variable to boolean unless that variable is a ReadOnlyArrayObject, in which case it determines truthiness based on "count($var) > 0".

Keep in mind that this is still POC code, but I'd like like to get some review to see if this direction makes sense, or if we need to step back and try another solution.

pwolanin’s picture

@samuel.mortenson why is page.featured an empty array?

samuel.mortenson’s picture

@pwolanin I'm guessing that by default, empty theme regions are represented by empty arrays.

catch’s picture

Status: Active » Needs work
samuel.mortenson’s picture

Actually re-reading my patch from #16, we could not do any of the things in the interdiff and instead only wrap non-empty arrays. Should clear up a ton of the complexity, I'll work on a patch.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
10.41 KB
1.79 KB

That didn't take long. I regret working so long on #16 but am glad that we don't have to review that complexity. Let's kick off the testbot to see how much breaks.

Status: Needs review » Needs work

The last submitted patch, 21: render-array-wrapping-2860607-21.patch, failed testing.

samuel.mortenson’s picture

The warning that's causing multiple tests to fail is a result of us not settings proper flags for our ReadOnlyArrayObjects. Here's a new patch which fixes that (hopefully).

Status: Needs review » Needs work

The last submitted patch, 23: render-array-wrapping-2860607-23.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
10.88 KB
940 bytes

Found a bug in render_var where toRenderable() was called but not wrapped in a ReadOnlyArrayObject.

Status: Needs review » Needs work

The last submitted patch, 25: render-array-wrapping-2860607-25.patch, failed testing.

pwolanin’s picture

@samuel.mortenson - I'm a little confused here. Why re-wrap render arrays instead of simple dealing with them inside the if?

samuel.mortenson’s picture

@pwolanin This approach doesn't change the default behavior of toRenderable, as the Twig engine is what specifically needs to make render arrays read-only. The interdiff from #25 prevents a notice from being thrown later on when $arg = $arg->getArrayCopy(); is executed, although you're probably on the right track thinking that we could shuffle the logic in renderVar around to reduce the wrapping calls. I'll try to clean up the logic in my next patch.

pwolanin’s picture

@samuel.mortenson are you in IRC? Maybe we should talk, or let me know if you want me to try rolling a version of the patch.

samuel.mortenson’s picture

@pwolanin I haven't started writing a new patch so feel free to work off of #25. It's in an OK place to be reviewed/refactored to be a bit cleaner, and is close to passing tests, which is good! We're further along than I expected. I'm not on IRC consistently but I'll try to be there later today.

pwolanin’s picture

The docs on this option look like it may not be the right thing to use?

    /**
     * Entries can be accessed as properties (read and write).
     */
    const ARRAY_AS_PROPS = 2;
samuel.mortenson’s picture

@pwolanin Ah, you're right. We would need to override __set() in addition to using that flag. The reason I added that flag in #23 is that Twig is inconsistent with how it handles objects - sometimes it checks if an object implements \ArrayAccess and uses a call like $variable[$key], and other times it just checks if it's an object and runs $variable->$key. We could submit PRs to Twig to change its behavior to be more consistent, but I figured that would be better suited as a follow up.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
12.45 KB
10.37 KB

Here's a revised patch moving the class into the Drupal\Core\Render namespace so it can implement RenderableInterface

Did not try to address any test fails or add tests. It looks like the existing test could be a Kernel test instead of a web test?

pwolanin’s picture

@samuel.mortenson - ok, I tried taking it out in #32 (and there were not flaming errors in the Drupal ui) before I saw your comment so let's see what the tests do.

Do you have specific file/line examples of where Twig tries to access properties?

Update:

testing in a php shell, overriding function __set() does not seem to block the behavior.

Probably we need to implement __get() instead

samuel.mortenson’s picture

@pwolanin You can see the notice that was causing multiple tests to fail by applying the patch in #21 (before the flags), enabling the language module, and visiting /admin/config/regional/language/detection.
You should see this notice:

Notice: Undefined property: Drupal\Component\Utility\ReadOnlyArrayObject::$configurable in Twig_Template->getAttribute() (line 536 of vendor/twig/twig/lib/Twig/Template.php).

Your idea of just implementing __get and not passing flags sounds good to me.

pwolanin’s picture

Here's a patch adding that __get.

pwolanin’s picture

From discussion with @samuel.mortenson, fallback should be an exception since the variable could be something unexpected like a resource, not just an array.

The last submitted patch, 33: 2860607-32.patch, failed testing.

The last submitted patch, 36: 2860607-36.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 37: 2860607-37.patch, failed testing.

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
15.46 KB
2.25 KB

Tweaked template_preprocess_forum_list() and \Drupal\Tests\Core\Template\TwigExtensionTest a bit to support the ReadOnlyRenderArray object, which should fix most or all test failures. template_preprocess_forum_list() was setting render arrays as values in a Taxonomy Term object, which is totally non-standard. Instead of refactoring the entire function, I just wrapped that markup in a ReadOnlyRenderArray object. I've never seen this outside of forum, so I don't think it's going to be a common use case or one we should support.

pwolanin’s picture

Great that you were able to knock out the last fails. Possibly for forum we should use this instead:

\Drupal\Core\Render\Markup::create($forum->description->value)

Though that's marked @internal

We should probably mark the ReadOnlyRenderArry class @internal also, since tit shouldn't be used outside the render system.

samuel.mortenson’s picture

Issue tags: +Needs tests

@pwolanin Agreed, adding the "Needs tests" tag as well as we should have unit tests that verify that ReadOnlyRenderArrays can't be modified in Twig.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs performance review
  1. +++ b/core/lib/Drupal/Core/Render/ReadOnlyRenderArray.php
    @@ -0,0 +1,79 @@
    +  public function offsetSet($index, $newval) {}
    

    I think we should throw exceptions if something has been prevented from happening. This would improve the DX massively since it can only happen by a developer error and they would benefit if they could see that their code is not working as quickly as possible.

  2. Should we allow filters/functions to modify render arrays? Not sure if there are any good use cases out there for that but it might be a valid use case.
  3. Profiling this wouldn't probably at least hurt us.
  4. The biggest concern I have is the fact that this change is very disruptive. If we would commit this change as it is is now, it would likely end up breaking many sites. This might be one of the changes that we can only properly fix in Drupal 9.
pwolanin’s picture

@laurii why would this be disruptive to existing sites? Can you show me an example of a site's Twig template that would break?

I don't think that modifying render arrays in Twig was ever supposed to work, so if you were doing something that evil in Twig I don't think we have a responsibility to keep it working.

A filter or function can modify the render array by getting it as a plain array and then re-wrapping it to return, so I don't think that's a problem.

lauriii’s picture

Few of breaking examples:

Copied this directly from the Drupal Twig slack from a conversation that happened yesterday:

{{ {"#theme": 'my_theme', '#data': 'value'} }}

But also something like:

{{ render_array|without('#prefix') }}

I agree that these examples are a bit evil, but they still would cause a break.

pwolanin’s picture

@laurii - the second one should work fine with this patch (please double check and we should include a test) - it's just a copy and unset.

Perhaps the class naming needs to be be better since you can unset values so it's not truly read-only. Maybe something like "ProtectedRenderArray" ?

Cottser’s picture

Discussed with @xjm @alexpott @effulgentsia @catch and we agreed this is a critical security issue. And tentatively tagging with a security-ish tag (security people, please change to something more appropriate or remove if that makes more sense, thanks!).

Cottser’s picture

Sounds like just plain ol' security might make sense, even though it's lowercase (gasp!).

pwolanin’s picture

From discussion with laurii and Cottser in person, a way to provide an option for people who might be writing render arrays is to pull in the Twig theme function from http://cgit.drupalcode.org/components/tree/src/Template/TwigExtension.php.

Maybe that should go in 8.3 and this change in 8.4 so people have time to prepare?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
15.69 KB
8.6 KB

Quick patch for name change to ProtectedRenderArray and rework of forum code.

NR for bot.

alexpott’s picture

+++ b/core/modules/forum/forum.module
@@ -545,7 +547,9 @@ function template_preprocess_forum_list(&$variables) {
-    $variables['forums'][$id]->description = ['#markup' => $forum->description->value];
+    // Work-around the fact that this is an object instead of a render array
+    // by setting the property to a filtered Markup object so it's not escaped.
+    $variables['forums'][$id]->description = Markup::create(Xss::filter($forum->description->value));

Why not just $variables['forums'][$id]->description = ['#markup' => (string) $forum->description->value];

We don't want to see Markup::create() everywhere.

That said this change is tricky because custom / contrib could have done exactly the same thing as this. I think this fix needs to happen entirely within the render system and shouldn't impact other code.

alexpott’s picture

The stringification in #52 is not going to work. Is the problem being caused by the fact that $variables['forums'][$id] is an object?

samuel.mortenson’s picture

Yes, the problem is that the entity is having render arrays set on it, which is a bad practice that I've never seen anywhere else.

If the change feels awkward we can refactor the forum template and function more.

Edit: #41 has more detail on this.

pwolanin’s picture

Refactoring forum module seemed out of scope, so this was the most feasible fix

+++ b/core/modules/system/src/Tests/Theme/TwigReadOnlyTest.php
@@ -0,0 +1,41 @@
+  public function testReadOnlyTwigVariables() {

probably need to change the name of this?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Cottser’s picture

It sounds like maybe a good next step to just get this moving forward would be to refactor the forum.module theme layer code in a separate issue. Agreed with #41 that it doesn't seem like something we need to support. Created a separate issue to discuss that: #2901773: Refactor template_preprocess_forum_list() to not manipulate NodeInterface objects

I agree with #44 that this could benefit from profiling, adding that specific tag. During the D8 development cycle, all Twig variables were wrapped in a TwigReference object #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions.. Removing those objects didn't seem to cause a huge performance bump but I don't think the scenario that was tested was a super heavy one: #2114563-81: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions.

Another thing we need to be aware of is the TX/DX changes of having render arrays wrapped in terms of debugging and such. I suspect this will need a change record no matter what.

I also tried various tags, filters, etc. from core and Twig itself to try to further exploit this with the patch from #51 applied and wasn't able to come up with anything. Also, I confirmed that the iterable Twig test still works as expected with ProtectedRenderArray objects.