Code execution via Twig templates (including inline)

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.

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: [meta] Theme System Modernization Initiative.

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

@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.

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 Ah, true. I didn't read through #2818121: Create alternative to Twig include function to improve Drupal integration 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.

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.

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.

@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.

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.

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 I'm guessing that by default, empty theme regions are represented by empty arrays.

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.

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.

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).

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

@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 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 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 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.

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 Agreed, adding the "Needs tests" tag as well as we should have unit tests that verify that ReadOnlyRenderArrays can't be modified in Twig.

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.

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.

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!).

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

+++ 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.

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

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.

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.

In general, I'm okay with a more temporary fix (with a @todo) here that allows us to solve the critical issue at hand as long as it's not disruptive.

Here's a straight re-roll on top of 8.5.x.

Added unit test coverage, removing "Needs tests" flag! 😊

I'll be happy to write the change record after profiling is done - who normally does that for core?

Copy + paste got me again.

In #62, I implemented a no-op for the offsetUnset method, which broke the Twig "without" filter. I've removed this as users expect to use this filter so we can't rightly break it. Tests should look better.

Created draft change record at https://www.drupal.org/node/2932417

Reviewed this in person with samuel.mortenson at PNW Drupal Summit just now. He demonstrated the vulnerability to me by touching a file with a views edit. This could be a pretty significant chained vulnerability if there is an open admin permissions vulnerability. Some comments on names and comments below.

1. +++ b/core/lib/Drupal/Core/Render/ProtectedRenderArray.php
   @@ -0,0 +1,79 @@
   +/**
   + * Provides a render array object where indexes are read-only.
   + *
   + * @ingroup utility
   + */
   
   +++ b/core/modules/system/src/Tests/Theme/TwigReadOnlyTest.php
   @@ -0,0 +1,41 @@
   +class TwigReadOnlyTest extends WebTestBase {
   

   Why are we doing this? @todo comment on why

2. +++ b/core/lib/Drupal/Core/Render/RenderArrayIterator.php
   @@ -0,0 +1,49 @@
   +/**
   + * Provides an array iterator where indexes are read-only.
   + *
   + * @ingroup utility
   + */
   

   Why are we doing this? @todo comment on why

3. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -140,7 +141,43 @@ public function getTemplateClass($name, $index = NULL) {
   +  /**
   +   * Wraps render arrays with objects, to prevent modification from Twig.
   +   *
   +   * @param array &$context
   +   *   An array of parameters.
   +   */
   

   Would be nice to have a "... because otherwise Twig can allow any PHP callback to be excuted in certain situations."

4. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -140,7 +141,43 @@ public function getTemplateClass($name, $index = NULL) {
   +  /**
   +   * Wraps render arrays with objects, to prevent modification from Twig.
   +   *
   +   * @param array &$context
   +   *   An array of parameters.
   +   */
   

   s/objects/protected objects

5. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -140,7 +141,43 @@ public function getTemplateClass($name, $index = NULL) {
   +  /**
   +   * Unwraps read only objects into render arrays.
   +   *
   +   * @param array &$context
   +   *   An array of parameters.
   +   */
   

   @todo s/read only/protected

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

   testProtectedTwigVariables()

7. +++ b/core/modules/system/src/Tests/Theme/TwigReadOnlyTest.php
   @@ -0,0 +1,41 @@
   +    // To modify a ReadOnlyRenderArray, Twig will convert it to an array and
   

   s/ReadOnlyRenderArray/ProtectedRenderArray

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
@@ -73,6 +73,12 @@ function twig_theme_test_theme($existing, $type, $theme, $path) {
+  $items['twig_theme_test_read_only'] = [

s/twig_theme_test_read_only/twig_theme_test_protected

This addresses all feedback from #68, and replaces all instances of "read only" with "protected".

@elijah-lynn brought up that this implementation wraps every Twig variable that is an array, even if it's not actually a render array. This means that if you had an array of strings called "classes", and passed that to Twig, then Twig tried to edit that array (add a class), this patch would break that functionality. I haven't seen this before, but it's worth bringing up. Should we have some fancy logic that tries to determine if a given array is a render array before wrapping it?

Code looks good. Small nit:

+++ b/core/modules/system/src/Tests/Theme/TwigProtectedRenderArrayTest.php
@@ -0,0 +1,41 @@
+    // To modify a ReadOnlyRenderArray, Twig will convert it to an array and

Leftover ReadOnly...

> This means that if you had an array of strings called "classes", and passed that to Twig, then Twig tried to edit that array (add a class), this patch would break that functionality. I haven't seen this before, but it's worth bringing up. Should we have some fancy logic that tries to determine if a given array is a render array before wrapping it?

I have seen such use cases in the wild. I'd say this opens an amazing can of works though:

* We can't tell from an array if it is a render array as an empty array is a valid render array as well as a valid non-render array.
* So the point is how this array is used. Finding out this in the general case is equivalent to the unsolvable halting problem.

If this is true, we can either
* make all arrays protected, which is a BC break for the themers
* let variable providers wrap renderables, determining their use, which is also a BC break

(OMG, how often do i hope my reasoning is wrong... ;-)

@axel.rutz Yeah this is a tough one. With the way the current patch works, if we used something like preg_grep('/^#/', array_keys($possible_render_array)), we could see if any key of a given array starts with a hash, then we could assume that it's a render array and wrap it. This could have unintended consequences, but wouldn't be more insecure because we still prevent rendering non-protected arrays.

This addresses #70, thanks for the review.

#72: "contains a #foo key" is neither necessary (see empty array amongst others) nor sufficient (i had such nonrender arrays already) condition to know the array will be rendered.

so with that we'll have both problems:
* themers swearing their arrays are broken
* unrecognized render arrays passed in that will be rendered after an attacker injected their evil code will have exceptions thrown

@axel.rutz Are you saying that you've passed arrays to Twig with keys that included "#" which were _not_ render arrays? By restricting the scope of the wrapping we would not touch any other arrays passed to Twig.

Yes i had arrays that (for non-twig reasons) had random-byte keys so some proportion of it has #foo keys. But that's imho the minor point.

Really problematic is the other point: Render arrays with no #foo keys. I don't see how to handle that.

@axel.rutz Could you provide an example of a render array that contains no keys that start with "#"? Just trying to get a handle on the logic we need to implement.

What about an array that has non-numeric keys (as in a dictionary) gets wrapped. If an array has only numeric keys (as in a list) then it doesn't get wrapped.

This patch protects against what was brought up in #69, that arrays that are not render arrays passed to Twig may need to be edited in Twig. This protection is accomplished by relying on the \Drupal\Core\Render\Element class to determine if a key in an array is recognized as a render array property (this just checks that the first character in the key is "#").

This is-render-array check is recursive, which I didn't want to do, but I had to because the render system in Drupal allows deeply nested arrays that may not contain "#" keys until the very last layer. While rare, we should support these odd arrays. This is documented and tested in the interdiff.

A side-affect of this is that if you pass an array with a key that starts with a "#" to Twig, and that is not a render array, and you try to modify that array in Twig, that will no longer work. I think this is an incredibly rare case, so I'm not very worried about it. Hopefully tests pass. 🤞

The error from #79 is happening because the Shortcut Block tries to render an array containing an empty array (i.e. [[]]), which is not recognized as a render array and therefore not wrapped, which leads to an exception being thrown. If this was simply an empty array we could do an empty() check in TwigExtension.php, but this is a non-empty array. Not sure about the best way to approach this problem.

Valid real-life render arrays are amongst others: [], [[], []], [[[]]]

@axel.rutz

1. Do we have a test set for valid render arrays?
2. Also, does that continue on into endless sets of empty arrays?
3. Is any array that contains an empty array considered a valid render array?

So I think we need Render API or other core maintainers to weigh in on this before proceeding.

To summarize the issue: We need to prevent render arrays from being created or edited in Twig. To accomplish this, all arrays passed to Twig as variables (context) are wrapped in an immutable object. This works fine, but it was brought up that some developers pass non-render-arrays to Twig that they may need to append to using something like the merge filter. An array of classes is an example of this.

To support the mutability of non-render-array arrays passed to Twig, conditional wrapping logic was added in #79. This works in most cases, but has an issue where in some cases a deeply-nested-empty array is passed to Twig and rendered using the {{ variable }} syntax.

Our options at this point:

1. Revert to the logic that was passing tests in #73, and wrap all arrays passed to Twig unconditionally.
   This may require developers to change how they work with arrays in Twig that need to be mutable, but wrapping all arrays removes a lot of complexity and reduces the performance impact of this patch.
2. Push forward on only trying to wrap render arrays in #79, and add logic to check for deeply-nested-empty arrays (i.e. Treat [[[]]] as a render array).
3. Keep the logic in #79 the same, and require theme developers to check if a render array is empty before rendering it in Twig.

I am in favor of option #1, as it removes complexity and is easier to test and document for users. There is going to be an impact for developers regardless of the solution we choose.

Instead of preventing to edit arrays could we instead hash the #pre_render callbacks etc. and don't execute them unless the hash matches?

> Instead of preventing to edit arrays could we instead hash the #pre_render callbacks etc. and don't execute them unless the hash matches?

Interesting idea. This boils down to:
a) Change only array parts that look *dangerous* (i.e. contain callbacks)
b) Add a key that is effectively a code signature

While this does not solve the underlying problem (that we cannot surely know how an array will be used), a) greatly reduces the class of changed arrays, thus the chance of a falsely changed array, and b) may mitigate the effect of such a false change.

EDIT:

Just after sending i realized that you surely meant not to embed the hash in the render array, but to store it outside. Interesting.
So we just store a whitelist.

Then we have to work out: what to hash? only the callbacks? (Then we can as well store them.) Then we need to prove that a callback can not be abused by changing its arguments.
If we're not sure about that we must hash every array and all their sub-arrays (as they can be rendered separately).

I thought we have e.g. an Attributes class to handle mutable attributes?

We do - for the array of classes example an attributes object would be a great alternative.

OK so we say "Dear themers, all arrays are belong to us and will get wrapped. If you want mutable objects, use ArrayAccess." - right?
Some disruption, but rock solid!

OK so we say "Dear themers, all arrays are belong to us and will get wrapped. If you want mutable objects, use ArrayAccess." - right?

Um. No.

[Base] themes like Drupal Bootstrap do heavy render array manipulation. On some real live working sites, this extends into the [Twig] templates themselves because it's often easier to manage than preprocessing. This includes creating things like icons or other various theme-related elements on the fly.

This mentality to "decrease themer's functionality even more" because "we must blanket bomb the entire system" is ludicrous.

This entire issue only exists due to it being yet another byproduct of the much larger issue: we still use antiquated render arrays for nearly everything; this isn't going away anytime soon.

The actual reason this issue exists is because we simply slapped some extension into Twig that renders variables using our Renderer service (which is far from perfect BTW).

From the OP, the primary concern is executing sensitive callbacks with something like the following:

{{ {"#lazy_builder": ["shell_exec", ["touch /tmp/hellofromviews"]]} }}

The reality is: this has always been a reality in themes and can even be done in 7.x with far greater damage (PHPTemplate) lol

That being said, the introduction of Twig inline templates stored in the DB (for things like Views) has introduced a new little nitch that we hadn't yet considered, this being one of them (which we knew would likely happen, it was just a matter of time to find it).

From my perspective though, we'd be far better off introducing a validation of all render array callables (e.g. this really isn't about Twig at all) and filter out restricted/disabled functions in PHP.

Any "valid callback" should be a closure, service, Drupal namespaced method, or a procedural function that isn't blacklisted as mentioned above.

Just wrapping an already severely fragile render array can only spell major consequences (most likely for existing sites + contrib) yet unforeseen.

@markcarver @axel.rutz Just wanted to comment that any alternative solution that is less disruptive to themers is preferable (and welcome). This issue is just about mitigating the security vulnerability, and wrapping render arrays was just one approach. I don't want to give the impression that the current approach is what's necessarily going to land (we have no render api maintainer sign off as of yet).

I'll spend some time next week thinking about and maybe proof-of-concepting #85 and #90.

@markcarver, thank you for your viewpoint! You really sound as if you are quite angry though.

> [Base] themes like Drupal Bootstrap do heavy render array manipulation. On some real live working sites, this extends into the [Twig] templates themselves because it's often easier to manage than preprocessing. This includes creating things like icons or other various theme-related elements on the fly.

Interesting. Do you have example links for this?

Example 1: the classic case is modifying the '#attributes' of a render array to add a class.

{% extends "@block/block.html.twig" %}
{#
/**
 * @file
 * Override the system menu block to match IU Framework expectations for the
 * social menu in belt region.
 */
#}
{% block content %}
  {% if content %}
    {{ content|merge({'#attributes': {'class': ['social']}}) }}
  {% endif %}
{% endblock %}

^ Source

To toss another idea into the mix, would it be safer to replace the calls to call_user_func() with a new drupal_call_user_func() and blacklist vulnerable/exploitable functions before executing, such as:

function drupal_call_user_func($method, $value) {
  // blacklist functions from https://stackoverflow.com/questions/3115559/exploitable-php-functions#3697776
  static $blacklist = ['exec', 'passthru', 'system', 'shell_exec', 'popen', 'proc_open', 'pcntl_exec'];

  if (in_array($method, $blacklist) {
    die('unsafe call');
  } else {
    call_user_func($method, $value);
  }
}

This would have the benefit of protecting other callback vectors that may be vulnerable.

(and the same protection for call_user_func_array() as drupal_call_user_func_array() )

@diamondsea I think that's the alternative we need to look into, to not disturb what themes are doing today with mutable arrays passed to Twig. Maintaining a blacklist isn't easy (we wan't to make the first iteration as complete as possible), but I think we can do it in a new issue.

Give me a blacklist and i'll find an exploit not on it...
(Think object method calls...)

Yeah, I noticed that with the SA-CORE-2018-001 exploit Dries linked to:
https://research.checkpoint.com/uncovering-drupalgeddon-2
though I'm not quite understanding how the code got uploaded in order to execute it somewhere the autoloader (?) could find the class to run it.

I think an alternative approach might be #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE which sets out to limit what can be used as a callback in the render system.

Strong +1 for that!

I've re-rolled #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE and it's ready for reviews.

Discussed with @samuel.mortenson we agreed to postpone this on #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE and bump that to critical.

#2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE was committed so it's time to wake this up.

This issue may still need to be fixed to fix the Twig vulnerability: #2966711: Limit what can be called by a callback in form arrays, not sure if form callbacks can be triggered in Twig.

This will need to be committed to D9 in order to truly protect Twig: #3081025: Remove technical debt and complication from when doTrustedCallback() could either trigger errors or throw exceptions

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.

If a user has access to Twig files he likely has access to php files as well. I would only concern about Twig templates that can be edited through admin interface (i.e. in Views UI). Besides code execution, access unprivileged users to editing Twig templates can lead to information disclosure which is not covered here. See #3083276: Split safe/unsafe functions/filters to their own (sub)modules for details.

What if we just create an alternative Twig service that does not support render arrays at all?

Having read through this issue and looked at #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE what remains for this issue?

Tagging for Bug Smash, hopefully we can get the issue summary update done

Trying to make sense of this issue and understand if there's still any vulnerability here after #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE went in.

Before I say anything else, huge hat tip to @samuel.mortenson for reporting this to the sec team, and for all the effort and thought that went into trying to plug this vulnerability! Also thanks to @pwolanin, @Cottser, the core committers, and others who have provided invaluable insights, examples, counter-examples, and so on.

I just manually tested the steps in the summary on a fresh 9.5.x site, and instead of a '/tmp/hellofromviews' file, I got this exception (as expected):

The website encountered an unexpected error. Please try again later.

Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("Render #lazy_builder callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was shell_exec. See https://www.drupal.org/node/2966725") in "__string_template__34236c56fa60b62b87f4802840b18e89f1cef5c855f51b26b95845f9b832f158" at line 1. in Twig\Template->displayWithErrorHandling() (line 419 of vendor/twig/twig/src/Template.php).

That much works as designed, and solves the originally reported problem. 🎉

In #116 @samuel.mortenson linked off to #3081025: Remove technical debt and complication from when doTrustedCallback() could either trigger errors or throw exceptions. At the time that comment was written, that issue was about changing the behavior of #2966327 to stop triggering deprecations and always throw exceptions. However, that security change went in as part of #3104307: Remove BC layers in various Drupal\Core components (seriously bad scope management there 😬). As of 9.0.0, core/lib/Drupal/Core/Render/Renderer.php always invokes doTrustedCallback() to throw exceptions for insecure render array callbacks. So we're now safe-by-default on that front, and #3081025 morphed into a new scope (which might never be committed, since we probably want to keep the flexibility for either deprecations or exceptions for other uses of doTrustedCallback()).

That leaves only #2966711: Limit what can be called by a callback in form arrays. I'm not clear what's Twig-specific about that issue, or why this issue needs to stay open about the Twigy aspects of that (already marked critical) task. If you can trick Twig into calling form callbacks (yuck), it seems by far the best solution is to use a similar approach at #2966711 as we did for #2966327 and make form callbacks always secure, not just in some Twig-specific universe.

Therefore, I believe the right status here is "Closed (outdated)" or something, and we can focus any remaining energy on #2966711. However, I hate that using that status would not give credit to all the immense effort that went into this issue. 😢 "Transferring credit" seems like a PITA, noisy, and a waste of time. I'd rather just mark this "Closed (fixed)", instead. 😉

Meanwhile:

1. I added a summary of this comment to the issue summary to explain why I think this is no longer a bug.
2. Removing the various "needs" tags that this no longer needs.
3. Setting to 'Needs review' status to get confirmation of my assessment and my proposed resolution of simply marking this issue "fixed" to credit the folks that have worked on this extremely important problem.

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.

Users with access to Twig files are very likely have access to PHP files as well. That shouldn't be considered as a security issue. PHP code in templates has been used in Drupal 7 for years as well as in many other CMS powered by PHP template engine. I think we should only care about Twig templates that can be potentially edited via web UI, i.e. Views substitutions.

I propose we create an alternative Twig service (twig.safe) without any support for render arrays. We could use it in Views substitution and inline templates. Contrib modules like Twig Input Filter could also benefit from it.

Eventually, we could use that Twig environment for token replacements #2719477: Add support for Twig into Token::replace.

I think we probably need an extra follow-up for callbacks like #process for additional hardening that aren't using the trusted callback stuff either.

> I propose we create an alternative Twig service (twig.safe) without any support for render arrays. We could use it in Views substitution and inline templates. Contrib modules like Twig Input Filter could also benefit from it.

Big +1 on that.

Twig was announced as sandboxed, and i remember a talk saying "Hey, you even can give any non-trusted dudista access to your theming folder." I still deem it wrong to allow-by-default creating of render arrays (which has a big information disclosure potential) (instead of only explicitly allowing that), but it's like that now. Let's at least harden views and all the contrib modules using twig.

> I propose we create an alternative Twig service (twig.safe) without any support for render arrays. We could use it in Views substitution and inline templates. Contrib modules like Twig Input Filter could also benefit from it.

Could we get away with creating twig.unsafe, and then just switching over the theme layer to it? Would be a behaviour change for contrib, but would also fix any issues all at once.

> Could we get away with creating twig.unsafe, and then just switching over the theme layer to it? Would be a behaviour change for contrib, but would also fix any issues all at once.

+1 on that, the pattern "safe by default" is well proven and is friendly to the human factor.

I'm pretty certain that would break so many things

Following up on #121 I tested following the steps in the description. When I click update preview I get this in the logs which seems to be correct.

Twig\Error\RuntimeError: An exception has been thrown during the rendering of a template ("Render #lazy_builder callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was shell_exec. See https://www.drupal.org/node/2966725") in "__string_template__68d34f33f67b08a940b5e9cd800677fa" at line 1. in Twig\Template->displayWithErrorHandling() (line 419 of /var/www/html/vendor/twig/twig/src/Template.php).

So +1 for closed (outdated)

Other callbacks that don't use trusted callback interface but I think are safe:

* machine name exists, this isn't called during rendering, so won't get executed from twig, it only gets executed when validation fires which doesn't happen if you embed a form element in a twig file, it would need to be in something a form builder returned
* entity builder callbacks, same reason as above, plus you'd need an entity form for them to trigger

I think the next step here is to try #125 and see what breaks

Moving to NW for the IS update. Think a new title would useful too, but may just be me.

And for the implementation of #125