Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug.
Issue priority Major because closes potential data-loss vulnerabilities. Not critical because this requires admin-level permissions (access to Twig templates) to exploit this. See #2492839: Views replacement token bc layer allows for Twig template injection via arguments which prevents this from being exploited via the UI.
Prioritized changes Security improvement
Disruption Potential disruption for contrib modules that provide objects to Twig templates.

Problem/Motivation

We are passing fully functional entity objects to Twig templates instead of read-only ViewModels. This allows templates to do bad things, such as node.delete() which defeats a major benefit of using Twig over the PHPTemplate engine.

Proposed resolution

Twig Sandbox policy with whitelisted methods and properties. The list of methods and properties will be configurable as we will undoubtedly miss something a contrib (or even core) module needs access to.

The following methods are whitelisted:

  • id()
  • label()
  • bundle()
  • get()
  • __toString()

Also, methods that start with the following strings are whitelisted:

  • get
  • has
  • is

Finally, any method on the Attribute class is whitelisted as that class is designed to be changed from Twig templates (e.g.: <div {{ attributes.addClass("myClassName") }}

These options are configurable in settings.php (or settings.local.php) as such:

$settings['twig_sandbox_whitelisted_methods'] = [
  'foo',
];
$settings['twig_sandbox_whitelisted_prefixes'] = [
  'bar',
];
$settings['twig_sandbox_whitelisted_classes] = [
  'BazClass',
];

The above code will allow only methods named foo or methods that begin with bar (such as barGetSomeValue), overriding the existing method and prefix whitelists. Any method call on the BazClass class will be allowed.

The current implementation allows access to any public properties on any class. This can be easily changed in future implementations.

Remaining tasks

  1. Decide whether to use a Sandbox policy or "ViewModel" or both strategies.Use Sandbox policy
  2. Try and see how much work and/or pain these will have on DX While adding $settings to a settings.php is not ideal, it does allow these whitelists to be customized.
  3. Follow-up issue: Allow Sandbox policies to be added as services
  4. Ensure that the chosen strategy is swappable via a Sandbox Policy Service or whitelist property extension/trait or other Now configurable via $settings.
  5. Find sane defaults for whitelist/blacklist or helper methods to add to entities and other objects passed to templates. Done, though the sooner we get this in, the sooner we can figure out other methods/prefixes that should be added to the default lists.

User interface changes

n/a

API changes

None. Though contrib modules that provide objects for use in Twig templates will need to either adjust their API to use the default whitelisted methods and prefixes or advise site builders using their module to add the appropriate methods and/or prefixes to $settings as indicated above.

Data model changes

None.

Original report

I tried running {{ node.delete }} within node.html.twig and the node deleted. Bug or feature? I was talking with Larry Garfield and Lewis Nyman at Drupal Camp Twin Cities about the idea of only passing "ViewModels" or otherwise stripped down data into our templates rather than full entities with all methods available. We agreed that the presence of a delete method in Twig cuts against one of the original Twig selling points. The discussion around Twig originally (and still) often highlights that queries and other "bad" methods and functions simply can't be called from a template file. Someone shouldn't be able to destroy data on their site by editing a template.

Which entity methods (if any) should be available from a Twig template?

CommentFileSizeAuthor
#126 increment.txt4.28 KBpwolanin
#126 2513266-126.patch16.29 KBpwolanin
#121 2513266-106-121.interdiff.txt2.33 KBmikeker
#121 2513266-121.patch16.38 KBmikeker
#106 2513266-100-106.interdiff.txt4.61 KBmikeker
#106 2513266-106-twig-content-entities-whitelist.patch16.38 KBmikeker
#100 2513266-100-twig-content-entities-whitelist.patch15.71 KBmikeker
#99 twig_templates_can_call-2513266-98-reroll.patch15.67 KBjoelpittet
#96 twig_templates_can_call-2513266-96--reroll.patch0 bytesjoelpittet
#93 2513266-92-93.interdiff.txt809 bytesmikeker
#93 2513266-93-twig-content-entities-whitelist.patch15.67 KBmikeker
#92 2513266-83-92.interdiff.txt2.58 KBmikeker
#92 2513266-92-twig-content-entities-whitelist.patch15.67 KBmikeker
#83 2513266-80-83.interdiff.txt2.26 KBmikeker
#83 2513266-83-twig-content-entities-whitelist.patch15 KBmikeker
#80 twig-delete-2513266.test.patch14.7 KBlarowlan
#80 interdiff.txt4.3 KBlarowlan
#67 2513266-64-67.interdiff.txt6.61 KBmikeker
#67 2513266-67-twig-content-entities-whitelist.patch10.4 KBmikeker
#64 2513266-60-64.interdiff.txt1.81 KBmikeker
#64 2513266=64-twig-content-entities-whitelist.patch10.42 KBmikeker
#63 2513266-60-63.interdiff.txt1.78 KBmikeker
#63 2513266-63-twig-content-entities-whitelist.patch10.39 KBmikeker
#60 2513266-56-60.interdiiff.txt360 bytesmikeker
#60 2513266-60-twig-content-entities-whitelist.patch10.26 KBmikeker
#56 2513266-53-56.interdiff.txt571 bytesmikeker
#56 2513266-58-twig-content-entities-whitelist.patch10.39 KBmikeker
#53 2513266-38-53.interdiff.txt2.43 KBmikeker
#53 2513266-53-twig-content-entities-whitelist.patch10.39 KBmikeker
#38 2513266-18-38.interdiff.txt4.68 KBmikeker
#38 2513266-38-twig-content-entities-whitelist.patch11.81 KBmikeker
#18 2513266-15-18.interdiff.txt1.18 KBmikeker
#18 2513266-18-twig-content-entities-whitelist.patch11.27 KBmikeker
#15 2513266-7-15.interdiff.txt9.25 KBmikeker
#15 2513266-15-twig-content-entities-whitelist.patch10.1 KBmikeker
#7 twig_files_can_call_the-2513266-7.patch3.91 KBlauriii
#7 twig_files_can_call_the-2513266-test-only-7.patch1.96 KBlauriii
#5 2513266-it-works.png198.85 KBstar-szr
#5 twig_files_can_call_the-2513266-5.patch1.85 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

Issue tags: +frontend, +Twig

It's tempting to bump this to a critical, one of the reasons for moving to Twig is that we wouldn't be handing themers a loaded gun.

stevector’s picture

Title: Twig files can call the delete method on nodes » Twig files can call the delete method on some content entities

There is a difference in the patterns used within content entity templates that seems related; though I don't fully grok it yet.

For nodes and taxonomy terms {{ node.delete }} and {{ term.delete }} actually delete the entities.

For user.html.twig and block.html.twig I have not yet been able to delete the relevant content entity. In both cases the content entity is not available as a base variable. The content entities seem to be within the elements variable but the methods aren't callable.

star-szr’s picture

After some discussion on the Twig call going to play around a bit here to see what might make sense, or at least get some discussion going.

joelpittet’s picture

Issue summary: View changes
star-szr’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.85 KB
198.85 KB

Rough proof of concept. Could use a test to show that it works, here's a screenshot from a manual test:

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Needs review » Needs work
Issue tags: +Needs tests

Working on the tests

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.96 KB
3.91 KB

The last submitted patch, 7: twig_files_can_call_the-2513266-test-only-7.patch, failed testing.

Fabianx’s picture

+1 to using ViewModels - I have been saying this since ages, that fully blown entities don't belong in the templates, but only simple immutable value objects.

Sandbox Mode might have severe performance implications as I don't know how performant the default twig implementation is.

If I remember correctly, they will mainly exchange the getVariable() function with another one (which btw. would probably make using the compiled twig extension impossible), but we need to check.

getVariable() checks for objects as the last instance, so probably for 99% of the code not dealing with magic properties or magic methods, things should stay at the same performance.

Edit:

I think we should white list only get*() methods by default for core as I think our policy is that getters start with get.

stevector’s picture

Does anyone know why the delete method doesn't work with user or block? Perhaps the lowest impact change is to make node match whatever is happening there.

star-szr’s picture

I tend to agree that ViewModels are the ideal here, but wanted to start playing with options and that seemed like a larger change.

@Fabianx I believe the sandbox handling is inside the magical getAttribute (I think what you are referring to as getVariable), and the C extension handles this as well. We should definitely profile but I don't see this adding a ton of function calls at least for what we would be using it for:

        if ($this->env->hasExtension('sandbox')) {
            $this->env->getExtension('sandbox')->checkMethodAllowed($object, $method);
        }

If we are whitelisting only get*() we won't be able to do things like:

{{ node.id }}
{{ node.bundle }}

…and similar. So that needs some more thought if we keep going down the sandbox road.

mikeker’s picture

I think we should white list only get*() methods by default for core as I think our policy is that getters start with get.

That may be the policy, but it doesn't always happen in practice. Eg: EntityInterface::id as pointed out by @Cottser. Perhaps we could special case a few additions to the whitelist?

It's also fair to whitelist any idempotent function, not just get*. Eg: we should whitelist isFoo() and hasFoo() functions.

star-szr’s picture

Yup. @joelpittet and myself discussed this and came to the same conclusion as far as the whitelist. The blacklist approach is not intended as the final implementation but as a proof of concept.

We also discussed the idea of being able to extend the whitelist.

Fabianx’s picture

#11: Yup, that is correct:

https://api.drupal.org/api/drupal/core!vendor!twig!twig!lib!Twig!Templat...

My concerns are not as much then as it indeed will work with the extension as well.

We have 4-8 additional function calls per magic object access, but those are pretty slow already, so nothing to be really concerned about.

Lets get some profiling done anyway.

I think whitelisting:

id
get*
has*
is*

and allowing all properties is fine for a default policy (Lets see what tests break).

Lets just ensure its part of the config in the default.services.yml so that people are able to change it easily.

mikeker’s picture

I've updated #7 to be a whitelist instead of a blacklist. Currently whitelisted are:

id
label
get[A-Z\(]
has[A-Z]
is[A-Z]

The last three are using a more expensive preg_match instead of strpos to better target the functions we're whitelisting. As mentioned earlier, this needs profiling and that may be one place to save a few cycles if needed. Since we're whitelisting, I had to update some of the templates as calls to node.bundle now need to be node.getType() instead. I imagine this will break in some other places as well -- let's see what the testbot says.

I've also added a test to verify allowed functions.

Status: Needs review » Needs work

The last submitted patch, 15: 2513266-15-twig-content-entities-whitelist.patch, failed testing.

mikeker’s picture

Assigned: Unassigned » mikeker

Working on fixing the test failures.

mikeker’s picture

Status: Needs work » Needs review
FileSize
11.27 KB
1.18 KB

This seems to be at the root of a lot of them...

star-szr’s picture

Thanks @mikeker!

+++ b/core/themes/classy/templates/content/node.html.twig
@@ -68,7 +66,7 @@
-    'node--type-' ~ node.bundle|clean_class,
+    'node--type-' ~ node.getType()|clean_class,

+++ b/core/themes/classy/templates/content/taxonomy-term.html.twig
@@ -26,7 +26,7 @@
-    'vocabulary-' ~ term.bundle|clean_class,
+    'vocabulary-' ~ term.getType()|clean_class,

For what it's worth, these can just be node.type and term.type, Twig knows about getters.

http://drupaltwig.github.io/ThemeSystemLA2015/#/7

If we are somehow breaking this functionality then we may need to dig deeper - the most recent interdiff looks suspicious to me.

mikeker’s picture

@Cottser: Thanks for the review!

I think we need to dig deeper... At least I do, as this is where my inexperience with Twig starts to show. In core/themes/bartik/templates/node.html.twig:62, if I add:

<pre>The type is {{ node.type }}</pre>

I get:

Exception: Object of type "Drupal\Core\Field\EntityReferenceFieldItemList" cannot be printed. in Drupal\Core\Template\TwigExtension->escapeFilter() (line 421 of core\lib\Drupal\Core\Template\TwigExtension.php).

If instead I add:

<pre>The type is {{ node.type.getString }}</pre>

or

<pre>The type is {{ node.getType }}</pre>

The node renders correctly. I'm not sure where in the order show here that things go sideways.

Slightly related sidebar: while it's possible to use node.type to reference anything from an array key to an object method (didn't realize it automagically looked for getType and isType -- thanks for pointing that out!) do we have/want a convention that says that methods should look like methods (with parens) and properties should look like properties?

mikeker’s picture

Assigned: mikeker » Unassigned
Fabianx’s picture

Hmmm,

That is unfortunate that the sandbox looks at the literal and not the resolved value.

I think we should file and upstream bug for that, in which case the twig extension would also be updated.

For a limited time we could ship with our own fixed getAttribute(), but I want to discuss this upstream.

star-szr’s picture

Well it sounds like bundle was giving us some benefits, is there also a type property in that case? If there is a property it will try that first.

In other words can we please test {{ node.type }} with HEAD?

Edit: Or of course we could consider whitelisting bundle ;)

Fabianx’s picture

#23: I am pretty sure we should do #22.

The sandbox check should be before accessing the method, but after it has been resolved, which method it will use.

star-szr’s picture

I'm not saying we shouldn't do that, but the exception didn't seem like that was the issue is all. I haven't actually tested it myself :(

mikeker’s picture

Understand that I'm saying this with just enough Twig knowledge to get me in trouble... :)

I think the Twig Magic (also here) around resolving variables using the dot-syntax may be causing us more harm than good.

Take node.type for example. In HEAD, this matches with $node->type which is an EntityReferenceFieldItemList and (correctly) throws an exception when printed. However, node.Type renders to "article" because it does not match $node->type, nor $node->type() or $node->Type() because they don't exist, but does match $node->getType().

Similarly, node.bundle does not match a $node property so $node->bundle() is called and returns a string.

In my playing around with it, it seems that properties are matched case-sensitively and methods case-insensitively. Thus node.type matches the type property while all of node.Type, node.TYPE, node.gettype, node.GetType, and node.GETTYPE match $node->getType().

If I'm not missing something (which is altogether possible), this seems like a big headache in our future... @Fabianx, I'm interested to hear what you find upstream.

mikeker’s picture

I suppose the case-sensitive/insensitive bit makes sense: PHP variables are case-sensitive and function names/methods are case-insensitive.

Regardless, I don't think most know this (I could not have answered with certainty until I just looked it up) and I believe it will be a source of confusion. Though I'm not sure there's anything that can be done about it unless we can turn off Twig's magic variable resolution.

See also: https://github.com/twigphp/Twig/issues/361

Fabianx’s picture

#25: Ah, so bundle() is not a getter ... confusing ... I guess we could ensure that every id(), bundle(), type() function also has an alias get*.

star-szr’s picture

@mikeker I would strongly argue against removing the magic variable resolution, it's one of the most compelling features of Twig IMO.

mikeker’s picture

@Cottser: I'm not arguing against magic variable resolution since I don't really know Twig. But I do get nervous around "magic" things in languages. Some can be super cool (_sleep() and __wakeup()) and some can be scary (register_globals). Regardless, they add additional difficulty to debugging any downstream code because it's less obvious that some.action leads to some.getAction().

I believe we can solve this with better documentation -- both in the .html.twig file's docblock and in the Drupal API documentation (wouldn't it be cool to see the return type on pages such as this?)

Sorry, I realize that a lot of this is WAY outside the scope of this issue. Kinda ranting here... Apologies.

mikeker’s picture

#28, @Fabianx:

Ah, so bundle() is not a getter

No, bundle() is a (poorly named) getter that returns a string, but type doesn't show up in the API docs for the Node object, yet resolves to an EntityReferenceFieldItemList object in the Twig template when accessed via node.type.

Fabianx’s picture

Assigned: Unassigned » alexpott
Issue tags: +Needs framework manager review

I am assigning to alex pott for framework manager review - as this is a pretty radical change.

We also will need to get Entity API folks involved.

joelpittet’s picture

@Fabianx I agree with you on having the sandbox policy work on the resolved methods upstream. That sounds like a good solution all round.

I like the way this issue seems to be going, @mikeker thanks for making a green patch, I thought that would be harder:D You seem to be picking up the magic stuff quicker than you let on;)

I agree with @Cottser to try to keep magic stuffs as it is a pretty cool feature even though I completly see where you are coming with @mikeker on the hard to debug part because like CSS selectors there is specificity/precedence to know when you have a problem. Don't really want to "undo" a feature, specially an upstream one.

Sandboxing seems to be the easier approach to this problem (as long as people can extend it easily as mentioned a few times already). ViewModels would be cool but would take a hellva lot of work and likely a new meta to get off the ground.

Wouldn't mind hearing what @catch has to say here too, will ping them both.

Edit: and YES profiling this would be good once we have a direction to take this.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs framework manager review

I think at this point sandboxing methods on entities is the way to go. It's the least intrusive.

  1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,59 @@
    +      foreach ($whitelist as $exact_match) {
    +        if ($method == $exact_match) {
    +          return;
    +        }
    +      }
    

    in_array()

  2. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,59 @@
    +      $whitelist_patterns = [
    +        '/^get[A-Z\(]/',
    +        '/^has[A-Z]/',
    +        '/^is[A-Z]/',
    +      ];
    +      foreach ($whitelist_patterns as $pattern) {
    +        // If the method name starts with a whitelisted prefix, allow it.
    +        if (preg_match($pattern, $method)) {
    +          return;
    +        }
    +      }
    

    Can be one pattern /^(get|has|is)[A-Z] ... get can be in the exact match whitelist.

  3. +++ b/core/themes/classy/templates/content/node.html.twig
    @@ -68,7 +66,7 @@
    -    'node--type-' ~ node.bundle|clean_class,
    +    'node--type-' ~ node.getType()|clean_class,
    
    +++ b/core/themes/classy/templates/content/taxonomy-term.html.twig
    @@ -26,7 +26,7 @@
    -    'vocabulary-' ~ term.bundle|clean_class,
    +    'vocabulary-' ~ term.getType()|clean_class,
    

    I think bundle should be in the whitelist.

  4. With respect to being hard to debug is there anyway that twig debug could output which method it has magically used?
catch’s picture

Makes sense to do the sandboxing to me as well.

joelpittet’s picture

Issue summary: View changes

Thank you @alexpott and @catch! Now we have a way forward:)

joelpittet’s picture

Assigned: alexpott » Unassigned
Issue summary: View changes

And @mikeker whitelisting seems like a good wait to go instead of blacklisting, thank you.

mikeker’s picture

Addresses items 1 - 3 in #34. No idea how to deal with #4. :)

Also, changed the return values to return TRUE to match core/vendor/twig/twig/lib/Twig/Sandbox/SecurityPolicy.php and added tests for the exact-match whitelisted functions.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/templates/comment.html.twig
@@ -71,7 +71,7 @@
-    comment.owner.anonymous ? 'by-anonymous',
+    comment.getOwner().isAnonymous() ? 'by-anonymous',

+++ b/core/themes/bartik/templates/node.html.twig
@@ -64,7 +62,7 @@
-    'node--type-' ~ node.bundle|clean_class,
+    'node--type-' ~ node.getType()|clean_class,

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -71,7 +71,7 @@
-    comment.owner.anonymous ? 'by-anonymous',
+    comment.getOwner().isAnonymous() ? 'by-anonymous',

We should be able to switch those back now, right?

  1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,52 @@
    +      $whitelist = [
    +        'id',
    +        'label',
    +        'bundle',
    +        'get',
    +      ];
    +      if (in_array($method, $whitelist)) {
    +        return TRUE;
    +      }
    

    Lets use 'id' => TRUE, 'label' => TRUE, ...

    and then use isset(), which is slightly faster.

  2. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,52 @@
    +      if (preg_match('/^(get|has|is)[A-Z]/', $method)) {
    +        return TRUE;
    +      }
    

    I think we should make that configurable.

star-szr’s picture

For #39.2 can we just make the sandbox policy swappable?

And yeah let's roll back some of those template changes other than the docs. And nitpick but some of those docs are over 80 chars.

Awesome work @mikeker thank you!

mikeker’s picture

@Fabianx, thanks for the review! Some quick replies to #39:

We should be able to switch those back now, right?

Yes, we can, but this brings us back to the opaque "magic" discussion: we can be explicit about what we're calling with comment.getOwner().isAnonymous(). Someone can look up each of those calls in the API docs, know what they do, and easily debug a potential issue in one of those routines. If we can't add debug information about the method resolution (edit: as mentioned in #34.4), I would argue that we should be as explicit as possible (ie: use as little magic as we can) to make it clear what Twig functions call what Drupal functions.

Yes, it's more letters to type and yes it makes for changes that are not completely needed at a late point in the product cycle. But I believe it will save us from many WTF-issues in the future.

Because node.bundle is a function listed on the Node object API page we should roll that back. Will do in the next iteration if no one beats me to it.

and then use isset(), which is slightly faster.

Great idea. Thanks! Will add that in the next iteration.

Fabianx’s picture

#40: I would be in favor of making the regexp configurable, but I can live with making that an injected service that can be swapped out.

We should make it a service anyway. :)

lauriii’s picture

I agree with @Fabianx that TwigSandboxPolicy regexp should be configurable. If its only swappable it can be only changed once. I can imagine that being a problem.

star-szr’s picture

I have to ask: What do we actually mean when we say configurable? How would it be configurable? services.yml?

lauriii’s picture

Drupal alter or service tags, or is this kind of functionality provided by Twig? I know its possible to add more stuff into there easily but what if someone needs to allow something

Fabianx’s picture

#44: Yes, just put the twig_whitelist_regexp with 'bundle|...' in services.yml as a container parameter like twig_debug.

lauriii’s picture

Fabianx: that would be the easiest way but what if a module needs that to be configured?

Fabianx’s picture

#47: Yes, unfortunately .yml files can't be merged if they have the same parameter name.

So our best bet is to use a good old fashioned hook or an event?

Or maybe sandbox policies can be chained and we need to do it like request_policy / response_policy?

lauriii’s picture

I think its possible to add multiple policies but AFAIK policy contains only restrictions, and something that has been restricted is not possible to allow again by adding new policy which is the problem. I think it should be still possible to allow them per module basis because some modules might have methods that needs to be accessed even though core wouldn't want to access them. Changing it in core becomes also quite fragile because we might break something in there without giving them any other possibility to fix it than renaming the methods.

Fabianx’s picture

#49

Yes, we need would to do the policy like our other access checks with 3-kind logic:

- So a whitelist implementation would return ALLOWED when allowed and neutral when it does not fit.
- A blacklist policy would specifically deny.

where only when its allowed we allow it overall.

@see https://tag1consulting.com/blog/access-control

I think that would be the cleanest solution and then indeed chaining policies via a twig_sandbox_policy service tag would be solution. :)

star-szr’s picture

I like the sounds of that!

lauriii’s picture

I like that. Fabianx++!

That makes things more complicated for sure than what we have right now. This would be something that we might be able to do in a follow-up, even in 8.1.x. I'm just not sure if this can go in without it being configurable. If we proceed with the Fabianx' solution, we need to update the proposed solution to the issue summary. If not, we need to create a follow-up.

mikeker’s picture

The attached patch deals with the issues raised in #39:

  • The templates have been returned to their former state -- the discussion about whether we should be using more or less specific Twig variables can happen in a followup.
  • Switched the whitelist to use isset() instead of in_array(). Great tip!

@Cottser in #40, I wasn't able to find any docblocks over 80 chars. Did I miss one?

This patch doesn't deal with any of the configuration ideas discussed in #42 - #52. That sorta stuff is above my pay-grade! :) But if someone can explain it, I'm happy to take a stab at it.

joelpittet’s picture

Status: Needs work » Needs review

Let's see what this looks like from testbot.

Status: Needs review » Needs work

The last submitted patch, 53: 2513266-53-twig-content-entities-whitelist.patch, failed testing.

mikeker’s picture

@joelpittet: Thanks for catching that -- I always forget to set issues to NR! Grrrr...

Looks like another example of what @Fabianx pointed out in #22 -- that Twig looks at the literal (comment.owner becomes comment.getowner()) instead of the resolved (comment.owner becomes comment.getOwner() since comment.getowner() doesn't exist).

Not sure if this is by-design or not since PHP methods are case-insensitive, but I've changed the regex to handle both situations.

@Fabianx: Is there an upstream issue we can track with an @TODO in the code?

mikeker’s picture

Status: Needs work » Needs review

I remembered this time! ... sorta...

joelpittet’s picture

@mikeker nice catch and it's green:) I always for get to do NR, going to have to patch dreditor to prevent my forgetfulness:)

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -41,7 +41,7 @@ public function checkMethodAllowed($obj, $method) {
-      if (preg_match('/^(get|has|is)[A-Z]/', $method)) {
+      if (preg_match('/^(get|has|is)[A-Za-z]/', $method)) {

may be simpler to just add a /i flag at the end? it would catch 3rd parth classes that for silly reasons use GetName() instead of getName() conventions.

joelpittet’s picture

  1. +++ b/core/modules/node/templates/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    These two lines look over 80 chars in dreditor but they aren't. So no worries.

  2. +++ b/core/modules/node/templates/node.html.twig
    @@ -68,7 +66,6 @@
    -
    

    nit: This whitespace change isn't needed.

  3. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    ditto 80, don't worry about it. Dreditor issue;)

  4. +++ b/core/themes/classy/templates/content/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    ditto 80, don't worry about it. Dreditor issue;)

mikeker’s picture

may be simpler to just add a /i flag at the end? it would catch 3rd parth classes that for silly reasons use GetName() instead of getName() conventions.

I thought about that but preferred the more restricted version. Who knows, perhaps there's a method out there named iStopEverythingFromWorking()? OK, that's a bit far fetched.

Is there a performance benefit to using /i? If not, I'd prefer the subtle "enforcement" of our coding standards. :)

nit: This whitespace change isn't needed.

Fixed.

Thanks for the review @joelpittet. And let me know when you patch Dreditor to remind you to set an issue to NR!

larowlan’s picture

Looks great, especially tests, couple of questions and observations - thanks!

  1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,52 @@
    + * Default sandbox policy for Twig templates.
    

    Can we give this a bit longer description that indicates the role it serves for those not familiar with what a sandbox policy is?

  2. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,52 @@
    +    if ($obj instanceof EntityInterface) {
    

    What happens here with other objects? Not sure there are any but e.g. Attribute comes to mind for example?

  3. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,52 @@
    +      $whitelist = [
    +        'id' => TRUE,
    +        'label' => TRUE,
    +        'bundle' => TRUE,
    +        'get' => TRUE,
    +      ];
    +      if (isset($whitelist[$method])) {
    +        return TRUE;
    +      }
    +
    +      // If the method name starts with a whitelisted prefix, allow it.
    +      if (preg_match('/^(get|has|is)[A-Za-z]/', $method)) {
    

    What about node.field_image.entity.uri or node.field_tags.0.label? will those still work? Should we add tests to verify they do?

  4. +++ b/core/modules/node/templates/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    nit: > 80

  5. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,122 @@
    +    $loader = new \Twig_Loader_String();
    +    $twig = new \Twig_Environment($loader);
    +    $policy = new TwigSandboxPolicy();
    +    $sandbox = new \Twig_Extension_Sandbox($policy, TRUE);
    +    $twig->addExtension($sandbox);
    ...
    +    $loader = new \Twig_Loader_String();
    +    $twig = new \Twig_Environment($loader);
    +    $policy = new TwigSandboxPolicy();
    +    $sandbox = new \Twig_Extension_Sandbox($policy, TRUE);
    +    $twig->addExtension($sandbox);
    

    Can this go in a common setUp method to avoid duplication?, you'll need to assign $twig to a property to access it later of course

  6. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,122 @@
    +    $caught = FALSE;
    +    try {
    +      $twig->render('{{ entity.delete }}', ['entity' => $entity]);
    +    } catch (\Twig_Sandbox_SecurityError $e) {
    +      $caught = TRUE;
    +    }
    +    $this->assertTrue($caught, '\Twig_sandbox_security exception was thrown when entity was tried to be removed.');
    +
    +    $caught = FALSE;
    +    try {
    +      $twig->render('{{ entity.save }}', ['entity' => $entity]);
    +    } catch (\Twig_Sandbox_SecurityError $e) {
    +      $caught = TRUE;
    +    }
    +    $this->assertTrue($caught, '\Twig_sandbox_security exception was thrown when entity was tried to be saved.');
    +
    +    $caught = FALSE;
    +    try {
    +      $twig->render('{{ entity.create }}', ['entity' => $entity]);
    +    } catch (\Twig_Sandbox_SecurityError $e) {
    +      $caught = TRUE;
    +    }
    +    $this->assertTrue($caught, '\Twig_sandbox_security exception was thrown when new entity was tried to be created.');
    

    Any reason we can't make this a data provider and then we can use the @expectedException, @expectedExceptionMessage annotations?

  7. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,122 @@
    +  public function testEntitySafeMethods() {
    

    nit:Needs a docblock

star-szr’s picture

+++ b/core/modules/node/templates/node.html.twig
@@ -4,12 +4,10 @@
- * - node: Full node entity.
- *   - id: The node ID.
- *   - bundle: The type of the node, for example, "page" or "article".
- *   - authorid: The user ID of the node author.
- *   - createdtime: Time the node was published formatted in Unix timestamp.
- *   - changedtime: Time the node was changed formatted in Unix timestamp.
+ * - node: The node entity with limited access to object properties and methods.
+     Only "getter" methods (method names starting with "get", "has", or "is")
+     and a few common attributes such as "id" and "label" are available. Calling
+     other methods (such as node.delete) will result in an exception.

@joelpittet is correct, these aren't over 80 they just look it in Dreditor somehow. However they are missing the "field of stars" (asterisks in column 2).

mikeker’s picture

@larowlan: Thanks for the review in #61. I'm going to address #2 first, because I think the patch will break a bunch of things and that will take a bit of cleanup.

2. What happens here with other objects? Not sure there are any but e.g. Attribute comes to mind for example?

According to our goal of making this a whitelist, Attribute.addClass() should be blocked. However I didn't move the thrown exception outside the if-block when we changed from a blacklist to a whitelist, so it still works...

This patch now restricts all methods similar to how we were previously restricting Entity methods. I've whitelisted all methods on the Attribute object -- Twig-gurus, let me know if that is too generous. I'm sure the testbot will point out other objects/methods that need to be whitelisted...

mikeker’s picture

Missed a commit when making the previous patch...

The last submitted patch, 63: 2513266-63-twig-content-entities-whitelist.patch, failed testing.

joelpittet’s picture

Re iStopEverythingFromWorking(), LOL and good point:)

mikeker’s picture

Addressing @larowlan's remaining points from #61:

  • 1. Done.
  • 2. Done in #64.
  • 3.
    What about node.field_image.entity.uri or node.field_tags.0.label? will those still work?

    Since the node object implements __get, node.field_image returns a Drupal\file\Plugin\Field\FieldType\FileFieldItemList object. Objects without a __get() method would have to implement either field_foo() or getField_foo() to work as expected.

    In your example, you would have to write node.field_image.entity.uri[0].value to get public://field/image/gen9C56.tmp.jpeg out. Not sure if that's what is supposed to come out of a UriItem object or not...

  • 5. Done.
  • 6. Sorta done: I wasn't familiar with the @expectedExceptionMessage annotation. Since the message in this case changes based on the UID of the mock and the method being called, can we create a dynamic annotation? Is there a way to use placeholders in an annotation?
  • 7. Done.

Thanks, again, for the awesome review!

(Edit: fixed dorked up HTML...)

mikeker’s picture

Realized I didn't address this:

What about node.field_image.entity.uri or node.field_tags.0.label? will those still work? Should we add tests to verify they do?

Or, rather, didn't address the "should we add tests" part.

Normally I'd say "of course!" However the tests added here are unit tests and we'd need to mock the bejeezus out of things to cover something like node.field_image.entity.uri.

Perhaps we need some sort of fully-rendered-page test where we could include a bunch of objects and their common methods and verify they are rendered correctly? Suggestions?

joelpittet’s picture

Title: Twig files can call the delete method on some content entities » Twig templates can call delete() on entities and other objects
joelpittet’s picture

@mikeker regarding #68 I'd suggest maybe a SimpleTest for node.field_image.entity.uri Mostly because it would be easier and will cover a bit of the functional aspect of this. Which is to agree with your idea of 'fully-rendered-page test'

mikeker’s picture

Assigned: Unassigned » mikeker

In the process of converting this to a service collection.

pwolanin’s picture

Is there any indication of the performance impact of Sandbox Mode?

I'm not sure what a service collection means in terms of this patch?

webchick’s picture

Priority: Major » Critical

This sounds potentially horrifying.

catch’s picture

So it would be horrifying if this actually happened, the question is whether and when that could happen:

- if it's via the Views UI, then that gives you more direct instant ways to wreak havoc than for example an XSS issue would, you can just immediately start deleting stuff if the entity is available to the views template UI for example - but the Views UI permission is restricted and so should anything else be that lets you write templates in the UI (such as contemplate).

- if it's via a template, then it's no less secure than phptemplate was, I've seen an entire module-worth of stuff happening in node.tpl.php before, although part of the point of Twig was extra security.

So I think this is 'just hardening' as opposed to a vulnerability, but it's very unexpected and could possibly lead to some vulnerabilities being worse than they otherwise might be.

However even if it's 'just hardening', using the sandbox/whitelist could lead to some methods which are valid in a template being blocked (or we might miss some and find out after this lands), so getting it in sooner than later gives us a chance to find those quicker. Also apart from the security implication it feels more minor than patch release in terms of the change.

@pwolanin if there's a performance hit, it'll be when compiling templates, not when rendering them afaik. It'd be interesting to know if this actually affects the compiled templates at all though.

larowlan’s picture

Assigned: mikeker » larowlan

working on test, will send back to mikeker when done

plach’s picture

Given #74, is this actually critical?

Fabianx’s picture

#74: It does affect the compiled templates, even if the code is the same. It is part of Twig_Template::getAttribute(): (or twig_template_get_attribute for the compiled twig extension).

https://api.drupal.org/api/drupal/core!vendor!twig!twig!lib!Twig!Templat...

  if ($this->env->hasExtension('sandbox')) {
    $this->env->getExtension('sandbox')->checkMethodAllowed($object, $method);
  }

is the code, so looking at some additional function calls, which is not too bad.

That is pretty problematic as the default policy depends on throwing an Exception - as discussed above already.

So the best would be if we had exchangeable policies as for the rest of policies in core (e.g. Request / Response / Access policies).

And then we use the usual three way (NEUTRAL / ALLOW / DENY ) scheme and then map the result of that to an Exception() or no Exception().

That would be at least IMHO the perfect way to solve the issue and superior to Twig's own handling of that.

However a configurable whitelist already goes a long way and might be the MVP we need to solve this issue.

webchick’s picture

Priority: Critical » Major

#74 is a good argument for major.

pwolanin’s picture

So the method calls are not resolved at compile time? Are they expected to be? It would seem that should be part of compilation.

In this case, it would seem a simple whitelist is something that shouldn't have to be re-run every time you access a method?

larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
4.3 KB
14.7 KB

Here's a test that verifies you can still use stuff like {{ node.field_term.entity.label }}

sorry @mikeker but couldn't re-assign to you (weren't in list?)

Think this is pretty close now

joelpittet’s picture

@pwolanin re #79 this is likely because it doesn't know what it's acting on until runtime.

Example:

// $ composer require twig/twig
require_once 'vendor/autoload.php';
$twig = new Twig_Environment($loader = new Twig_Loader_String(), $twig_options = ['auto_reload' => TRUE]);

class Water extends ArrayObject {}
class Ocean {
  private $name = '';
  public function __construct($name) {
    $this->name = $name;
  }
  public function getName() {
    return $this->name;
  }
}
class Lake extends Ocean {}
$ocean = new Ocean('Pacific');
$lake = new Lake('Leman');
$water = new Water(['ocean' => $ocean, 'lake' => $lake]);
echo $twig->render("{% set key = 'lake' %}{{ water[key].name }}", ['water' => $water]);

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,62 @@
    +    if (preg_match('/^(get|has|is)[A-Za-z]/', $method)) {
    

    I'm curious whether we want to use strpos 3 times instead for better performance

  2. +++ b/core/modules/node/templates/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    80 chars

  3. +++ b/core/modules/system/src/Tests/Theme/TwigWhiteListTest.php
    @@ -0,0 +1,130 @@
    +class TwigWhiteListTest extends KernelTestBase {
    

    Did you considered to use a new kernel test base?

  4. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,123 @@
    +  public function testEntitySafeMethods() {
    

    This should be split into multiple methods

  5. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    
    +++ b/core/themes/classy/templates/content/node.html.twig
    @@ -4,12 +4,10 @@
    + * - node: The node entity with limited access to object properties and methods.
    ...
    +     and a few common attributes such as "id" and "label" are available. Calling
    

    80 chars

mikeker’s picture

Sorry, I've been offline for a couple days... So, to reply to a bunch of questions in the last week of so:

#72:

Is there any indication of the performance impact of Sandbox Mode?

No profiling has been done yet -- this may be my excuse to learn xhprof. :)

I'm not sure what a service collection means in terms of this patch?

The idea was first raised by @Fabianx in #50. It would allow a module to provide a Twig sandbox policy as a service that would be chained with other policies to allow an allow/deny/neutral result for any call from a Twig template.

In terms of this patch, it means defining the service_collection in core, building a policy manager that loops through the registered policies and provides a yes/no answer for any given method call, and moving the code currently in TwigSandboxPolicy.php into a default service. That's what I was working on earlier, but wasn't able to post a patch before heading out of town... I believe I'm pretty close to finishing this, but I'm having problems getting the service collector to recognize the default policy service. I will ping folks on IRC later today for help.

#82:

1. I'm curious whether we want to use strpos 3 times instead for better performance

I ran a quick benchmark for this case and the strpos-three-times option is about 3x faster than preg_match for the negative (calling a disallowed method) and between 3x and 7x faster for calling an allowed method! I've updated the patch accordingly. Thanks @dawehner!

This could be optimized further by ordering the get/has/is strpos calls so that the most frequently used goes first. I took the current order based on Twig's order of resolving dot notation (see the Implementation section). Any other suggestions?

4. This should be split into multiple methods

Done: one method to test prefixes and one to test whitelisted methods. Or did you want to see one for each prefix and each method?

80 chars

Those are an oddity with Dreditor when the line is exactly 80 chars long. See #59.

pwolanin’s picture

Using a service is going to make this all that much slower than a single sandbox?

lauriii’s picture

+++ b/core/themes/bartik/templates/node.html.twig
@@ -4,12 +4,10 @@
+ * - node: The node entity with limited access to object properties and methods.
...
+     and a few common attributes such as "id" and "label" are available. Calling
+     other methods (such as node.delete) will result in an exception.

+++ b/core/themes/classy/templates/content/node.html.twig
@@ -4,12 +4,10 @@
+ * - node: The node entity with limited access to object properties and methods.
...
+     and a few common attributes such as "id" and "label" are available. Calling
+     other methods (such as node.delete) will result in an exception.

These are still over 80 chars

I think we can leave the policy configuration into a follow-up and try to get this in as it is. We still need to profile the current patch.

star-szr’s picture

I know it really seems like they're over 80 chars but it's some kind of Dreditor bug (sorry!). They are exactly 80 chars.

Edit: https://github.com/unicorn-fail/dreditor/issues/260

lauriii’s picture

Oh sorry for missing that. Then we are still missing the profiling :)

mikeker’s picture

(This is my first time using XHProf so if there is a better way to present these results or something that looks fishy, please let me know!)

Setup:

  • Ubuntu 14.04 running in a VM, using the defaults from this VM and XDebug disabled
  • Clean D8 using the standard install profile plus the devel_generate module
  • Using the stock settings.local.php and services.yml files (rendering caching off, Twig debugging off)
  • Added 5 users, 10 terms and 50 nodes

1. Profiling results for the default home page view:

vagrant@drupalvm:/var/clean$ ./xhprof-kit/benchmark-branches.sh 55ecb2432915b baseline-8.0.x 8.0.x 2513266-83
### BENCHMARKING ...

=== baseline-8.0.x..8.0.x compared (55ecb2432915b..55ecb6fe8f590):

ct  : 162,054|162,054|0|0.0%
wt  : 686,674|684,289|-2,385|-0.3%
mu  : 26,135,176|26,078,280|-56,896|-0.2%
pmu : 26,233,232|26,175,128|-58,104|-0.2%

<a id="xhprof-profiler-output" href="http://vagrant-clean.core.d8/xhprof-kit.php/?source=drupal-perf&url=%2F&run1=55ecb2432915b&run2=55ecb6fe8f590&extra=baseline-8.0.x..8.0.x">Profiler output</a>


=== baseline-8.0.x..2513266-83 compared (55ecb2432915b..55ecb787445e2):

ct  : 162,054|166,381|4,327|2.7%
wt  : 686,674|700,449|13,775|2.0%
mu  : 26,135,176|26,206,920|71,744|0.3%
pmu : 26,233,232|26,369,344|136,112|0.5%

<a id="xhprof-profiler-output" href="http://vagrant-clean.core.d8/xhprof-kit.php/?source=drupal-perf&url=%2F&run1=55ecb2432915b&run2=55ecb787445e2&extra=baseline-8.0.x..2513266-83">Profiler output</a>


Aggregated file written successfully.
Aggregated file written successfully.



### FINAL REPORT

=== baseline-8.0.x..8.0.x compared (55ecb2432915b..55ecb6fe8f590):

ct  : 162,054|162,054|0|0.0%
wt  : 686,674|684,289|-2,385|-0.3%
mu  : 26,135,176|26,078,280|-56,896|-0.2%
pmu : 26,233,232|26,175,128|-58,104|-0.2%

=== baseline-8.0.x..2513266-83 compared (55ecb2432915b..55ecb787445e2):

ct  : 162,054|166,381|4,327|2.7%
wt  : 686,674|700,449|13,775|2.0%
mu  : 26,135,176|26,206,920|71,744|0.3%
pmu : 26,233,232|26,369,344|136,112|0.5%

=== SUM: 2513266-83-summary..8_0_x-summary compared (55ecb7a973705..55ecb7b1047c4):

ct  : 16,643,516|16,210,816|-432,700|-2.6%
wt  : 73,610,969|72,489,277|-1,121,692|-1.5%
mu  : 2,621,304,072|2,608,382,144|-12,921,928|-0.5%
pmu : 2,637,549,064|2,618,132,856|-19,416,208|-0.7%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        |    166,381 |    171,797 |    166,435 |    166,381 |    166,381 |
| 8_0_x             |    162,054 |    167,470 |    162,108 |    162,054 |    162,054 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        |    700,449 |    937,600 |    736,110 |    729,387 |    801,205 |
| 8_0_x             |    684,289 |    930,671 |    724,893 |    715,407 |    809,353 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        | 26,206,760 | 26,809,848 | 26,213,041 | 26,206,920 | 26,206,920 |
| 8_0_x             | 26,078,248 | 26,497,608 | 26,083,821 | 26,078,280 | 26,078,280 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        | 26,369,224 | 26,974,328 | 26,375,491 | 26,369,344 | 26,369,344 |
| 8_0_x             | 26,174,808 | 26,663,504 | 26,181,329 | 26,175,128 | 26,175,128 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

2. Profiling results for an article page with comments enabled and an image for both the author and article:

vagrant@drupalvm:/var/clean$ ./xhprof-kit/benchmark-branches.sh 55ecbbf1235ef baseline-8.0.x 8.0.x 2513266-83
### BENCHMARKING ...

=== baseline-8.0.x..8.0.x compared (55ecbbf1235ef..55ecbc9e67db4):

ct  : 67,091|67,091|0|0.0%
wt  : 296,695|297,592|897|0.3%
mu  : 21,884,216|21,884,216|0|0.0%
pmu : 22,018,216|22,018,216|0|0.0%

<a id="xhprof-profiler-output" href="http://vagrant-clean.core.d8/xhprof-kit.php/?source=drupal-perf&url=%2F&run1=55ecbbf1235ef&run2=55ecbc9e67db4&extra=baseline-8.0.x..8.0.x">Profiler output</a>


=== baseline-8.0.x..2513266-83 compared (55ecbbf1235ef..55ecbcdaa2765):

ct  : 67,091|68,358|1,267|1.9%
wt  : 296,695|302,125|5,430|1.8%
mu  : 21,884,216|22,003,736|119,520|0.5%
pmu : 22,018,216|22,137,480|119,264|0.5%

<a id="xhprof-profiler-output" href="http://vagrant-clean.core.d8/xhprof-kit.php/?source=drupal-perf&url=%2F&run1=55ecbbf1235ef&run2=55ecbcdaa2765&extra=baseline-8.0.x..2513266-83">Profiler output</a>


Aggregated file written successfully.
Aggregated file written successfully.



### FINAL REPORT

=== baseline-8.0.x..8.0.x compared (55ecbbf1235ef..55ecbc9e67db4):

ct  : 67,091|67,091|0|0.0%
wt  : 296,695|297,592|897|0.3%
mu  : 21,884,216|21,884,216|0|0.0%
pmu : 22,018,216|22,018,216|0|0.0%

=== baseline-8.0.x..2513266-83 compared (55ecbbf1235ef..55ecbcdaa2765):

ct  : 67,091|68,358|1,267|1.9%
wt  : 296,695|302,125|5,430|1.8%
mu  : 21,884,216|22,003,736|119,520|0.5%
pmu : 22,018,216|22,137,480|119,264|0.5%

=== SUM: 2513266-83-summary..8_0_x-summary compared (55ecbcebf3fb4..55ecbcf2a8d02):

ct  : 6,840,131|6,713,431|-126,700|-1.9%
wt  : 32,803,615|31,540,786|-1,262,829|-3.8%
mu  : 2,202,139,568|2,189,699,912|-12,439,656|-0.6%
pmu : 2,215,580,168|2,203,165,592|-12,414,576|-0.6%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        |     68,358 |     72,689 |     68,401 |     68,358 |     68,358 |
| 8_0_x             |     67,091 |     71,422 |     67,134 |     67,091 |     67,091 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        |    300,118 |    414,770 |    328,036 |    320,686 |    392,346 |
| 8_0_x             |    296,901 |    392,143 |    315,408 |    313,356 |    330,292 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        | 22,003,688 | 23,041,488 | 22,021,396 | 22,003,736 | 22,003,856 |
| 8_0_x             | 21,884,216 | 22,678,112 | 21,896,999 | 21,884,216 | 21,884,280 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2513266-83        | 22,137,416 | 23,240,992 | 22,155,802 | 22,137,480 | 22,137,592 |
| 8_0_x             | 22,018,104 | 22,878,928 | 22,031,656 | 22,018,216 | 22,018,336 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+
Fabianx’s picture

#74 did not take into account that we have currently a bug in the replacement of %1 %2 etc. tokens in view templates. (#2492839: Views replacement token bc layer allows for Twig template injection via arguments)

Combined this issues could be a big problem (I have not tested this):

- Admin User puts in %1 for a template also having a node object available as {{ node }}
- Attacker visits /view/[name]/{{ node.delete() }}/

as that is perfectly valid code no check_plain() would help against this.

I am unsure of critical, but if someone was able to delete a node via that attack vector one or both would probably be critical indeed.

Edit:

I tested it now and there is no {{ node }} object available in views rewrites, so I _think_ both can remain major bugs.

Fabianx’s picture

As there was no attack vector possible that this issue would help against, only made #2492839: Views replacement token bc layer allows for Twig template injection via arguments critical and not this one.

As you never have any object in views rewrites, this particular issue only pertains to normal templates and hence is a security improvement and not a critical bug, but mostly major.

catch’s picture

mikeker’s picture

In the interest of getting things in before the door shuts, I've added support for setting the whitelisted methods and prefixes in settings.php. Eventually, this should be a service collection with a yes/no/neutral result similar to node access, but I haven't had time this week...

With this, we at least give site builders and module maintainers the ability to add/remove methods and prefixes from the whitelist. Not ideal, but it'll work.

Point of discussion: do we want to allow people to REMOVE things from the default whitelist (this code allows $settings['twig_sandbox_whitelisted_methods'] = [];)? Or should this be twig_sandbox_additional_whitelisted_methods instead which would array_merge with our default whitelist?

mikeker’s picture

Did some profiling, then remembered that isset() is faster than just about anything else. (As I already learned in #39, but managed to forget already! My run-it-a-million-times-and-compare profiling shows isset() to be about 3x faster than is_null().)

Then did some more profiling with this latest patch using 5 users, 10 tags, 50 nodes and the default frontpage view:

=== baseline-8.0.x..8.0.x compared (55f47afddbb77..55f47cbd52618):

ct  : 158,191|158,191|0|0.0%
wt  : 654,929|652,469|-2,460|-0.4%
mu  : 26,343,184|26,343,184|0|0.0%
pmu : 26,533,120|26,533,120|0|0.0%

=== baseline-8.0.x..2513266-83 compared (55f47afddbb77..55f47d0cb9a29):

ct  : 158,191|162,366|4,175|2.6%
wt  : 654,929|670,498|15,569|2.4%
mu  : 26,343,184|26,481,928|138,744|0.5%
pmu : 26,533,120|26,671,856|138,736|0.5%

=== baseline-8.0.x..2513266-92-isset compared (55f47afddbb77..55f47d33ae0bf):

ct  : 158,191|162,368|4,177|2.6%
wt  : 654,929|671,558|16,629|2.5%
mu  : 26,343,184|26,484,864|141,680|0.5%
pmu : 26,533,120|26,674,712|141,592|0.5%

Which tells me that adding the ability to customize the whitelists adds hardly anything in terms of performance. In fact the wall time 95 percentile numbers for this patch were below that of #83 and 8.0.x!

joelpittet’s picture

Issue tags: +Needs reroll

Needs a slight re-roll, but applies with patch -p1. I'll post in a few after profiling.

joelpittet’s picture

Wish I could upload but I lost the data files on the plane. Though the results are very similar.

### FINAL REPORT

=== 8.0.x..8.0.x compared (55fb2d22ec071..55fb2dfdf13b0):

ct  : 468,842|468,842|0|0.0%
wt  : 1,179,434|1,178,031|-1,403|-0.1%
mu  : 31,814,760|31,814,760|0|0.0%
pmu : 32,440,368|32,440,368|0|0.0%

=== 8.0.x..2513266 compared (55fb2d22ec071..55fb2e539b180):

ct  : 468,842|482,883|14,041|3.0%
wt  : 1,179,434|1,206,275|26,841|2.3%
mu  : 31,814,760|31,966,840|152,080|0.5%
pmu : 32,440,368|32,587,384|147,016|0.5%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
joelpittet’s picture

Very similar results 2-3% wall time regression. 50 nodes on the homepage.

=== 8.0.x..8.0.x compared (55fc0948e739d..55fc0a181a814):

ct  : 468,813|468,813|0|0.0%
wt  : 1,068,477|1,056,752|-11,725|-1.1%
mu  : 31,809,088|31,808,968|-120|-0.0%
pmu : 32,442,648|32,442,184|-464|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fc0948e739d&...

=== 8.0.x..2513266 compared (55fc0948e739d..55fc0a915f05b):

ct  : 468,813|482,854|14,041|3.0%
wt  : 1,068,477|1,088,966|20,489|1.9%
mu  : 31,809,088|31,960,568|151,480|0.5%
pmu : 32,442,648|32,589,528|146,880|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fc0948e739d&...

Status: Needs review » Needs work

The last submitted patch, 96: twig_templates_can_call-2513266-96--reroll.patch, failed testing.

The last submitted patch, 96: twig_templates_can_call-2513266-96--reroll.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.67 KB

Real patch now:) Thanks for the heads up @Cottser

3way auto merge

Falling back to patching base and 3-way merge...
Auto-merging core/lib/Drupal/Core/Template/TwigEnvironment.php
mikeker’s picture

mikeker’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary. Added beta eval.

Status: Needs review » Needs work

The last submitted patch, 100: 2513266-100-twig-content-entities-whitelist.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

I'm not going to bother hitting re-test on the old bot now, it's pretty backed up. But setting back to needs review.

star-szr’s picture

Status: Needs review » Needs work

Overall this is looking great and I think it's very very close. Awesome work @mikeker and @joelpittet and everyone :)

  1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
    @@ -0,0 +1,81 @@
    +    // Allow settings.php to override our default whitelisted methods and
    +    // prefixes.
    +    static $whitelisted_methods = NULL;
    +    static $whitelisted_prefixes = NULL;
    

    Maybe these can just be properties on the object? This seemed a bit weird to me to have statics inside the method like this.

    In other words:

    protected $whitelisted_methods = NULL;
    protected $whitelisted_prefixes = NULL;
    
    …
    
    if (!isset($this->whitelisted_methods) || !isset($whitelisted_prefixes)) {
      …
    }
    
  2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,131 @@
    +
    +  protected function setUp() {
    +    parent::setUp();
    

    Do these get docblocks now? inheritdoc?

  3. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
    @@ -0,0 +1,131 @@
    +    $loader = new \Twig_Loader_String();
    

    Use \Drupal\Core\Template\Loader\StringLoader here please, \Twig_Loader_String is going away in Twig 2.x. See #2555243: Upgrade path / plan to Twig 2.x aka 2.0

mikeker’s picture

Assigned: Unassigned » mikeker
+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -0,0 +1,81 @@
+    if ($obj instanceof Attribute) {
+      // Allow any operations on the Attribute object as it is intended to be
+      // changed from a Twig template, for example calling addClass(). This
+      // cannot be overridden by settings.php.
+      return TRUE;
+    }

While we're tinkering, do we need a configurable whitelist for allowed objects?

Thanks for the review @Cottser! I'll incorporate your feedback in the next reroll.

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
16.38 KB
4.61 KB

I've added the feedback from #104. Yeah, I'm not sure why I didn't add those as properties in the first place.

I've also added the ability to whitelist entire objects. The downside of this change is we no longer use instanceof which means we need to whitelist child objects separately from their parents. This could be good or it could be bad. It forces you to be careful and deliberate about what objects you're whitelisting, but it could make for a lot of settings.php additions if there are lots of foo extends Attribute bits out there.

Finally, this is a big enough change that I'll rerun the profiling scenario just to make sure this doesn't add much to the existing regression.

mikeker’s picture

Profiled #106 using the same setup as #93 (5 users, 10 tags, 50 nodes, default frontpage view):

=== baseline-8.0.x..8.0.x compared (5601c62d03120..5601c6d4ed09f):

ct  : 141,452|141,452|0|0.0%
wt  : 637,886|645,609|7,723|1.2%
mu  : 26,528,152|26,528,152|0|0.0%
pmu : 26,706,240|26,706,240|0|0.0%

=== baseline-8.0.x..2513266-106 compared (5601c62d03120..5601c73d235a8):

ct  : 141,452|145,605|4,153|2.9%
wt  : 637,886|662,167|24,281|3.8%
mu  : 26,528,152|26,674,584|146,432|0.6%
pmu : 26,706,240|26,850,880|144,640|0.5%

Which is very similar to what I got in #93 and what @joelpittet saw in #96 -- approximately a 2.5 - 3% wall time regression when all caching is turned off.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - Lets please create a follow-up issue to make the sandbox policy an injected service.

We can at every point in time add an injected service, so not blocking this very important security improvement issue on that detail, but a follow-up would be nice.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 106: 2513266-106-twig-content-entities-whitelist.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Test fail looks unrelated

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 106: 2513266-106-twig-content-entities-whitelist.patch, failed testing.

pwolanin’s picture

An injected service seems like it would be adding extra complexity of dubious value, unless it's also allowing us to remove the settings support?

mikeker’s picture

@pwolanin: Yes, it would let us to remove the settings.php $conf bit... That's a stop-gap for now.

It would also any module respond with an allow/deny/neutral a la the node access system with a single Twig extension that would poll all services tagged as sandbox policy providers... as long as the performance hit isn't too great!

RainbowArray’s picture

Status: Needs work » Reviewed & tested by the community

Looks like an unrelated fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 106: 2513266-106-twig-content-entities-whitelist.patch, failed testing.

RainbowArray’s picture

Issue tags: +rc target triage

Oh dang, this didn't get in. Tagging this for triage since this was a potential concern about content getting deleted.

mikeker’s picture

Status: Needs work » Reviewed & tested by the community

Not sure why the patch in #106 isn't showing the correct testbot results. This patch is passing tests as of 12 Oct. Moving this back to RTBC as per #114.

pwolanin’s picture

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -0,0 +1,106 @@
+    // Convert a basic array into whitelisted_item => TRUE. This allows us to
+    // use isset() which is faster than in_array().
+    foreach ($whitelisted_methods as $method) {
+      $this->whitelisted_methods[$method] = TRUE;
+    }
+    foreach ($whitelisted_objects as $object) {
+      $this->whitelisted_objects[$object] = TRUE;
+    }

Could use array_flip here instead of the foreach loops.

Also, the variable name and setting name for objects doesn't seem accurate. You are actually whitelisting class names not objects?

mikeker’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.38 KB
2.33 KB

re: #120:

Could use array_flip here instead of the foreach loops.

Yes, but we would have to special-case the first element in the array ([0 => 'foo'] flips to ['foo' => FALSE]). Considering this is run once when the sandbox is instantiated (ie: once per request) and the list of whitelisted classes/methods should be minimal, I don't believe the optimization is worth the reduced readability/additional complexity.

Also, the variable name and setting name for objects doesn't seem accurate. You are actually whitelisting class names not objects?

Yes we are -- thank you for pointing that out! Fixed.

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -15,7 +15,7 @@
  * limiting access to potentially unsafe attributes or methods. Since we do not
...
+ * objects can do by whitelisting certain classes, methods, and attributes.

Just noticed this is called 'attributes'. Shouldn't this be called 'properties' or 'public member variables'? Nitpik.

pwolanin’s picture

Should fix the docs. Are properties ever unsafe to access? Seems like this comment might be incorrect in general?

pwolanin’s picture

@mikeker - You don't have to special-case 0. It's still set.

Looks from the code like we are ignoring all property access:

public function checkPropertyAllowed($obj, $property) {}

Fabianx’s picture

Status: Needs review » Needs work

Yep, CNW for the comment, that is indeed misleading.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
16.29 KB
4.28 KB

Small doc and code fixes.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then :)

mikeker’s picture

Issue summary: View changes

Updated issue summary -- the info about changing the whitelist in $settings was out of date.

mikeker’s picture

Added a draft CR.

mikeker’s picture

xjm’s picture

Issue tags: -rc target triage +rc target

Discussed with the D8 committers and we agreed with making this an rc target.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think this is slightly risky as potentially might break people's sites - however it is way more risky to not do this. Committed db091f3 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
index 4313e7f..5423bde 100644
--- a/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -36,6 +36,9 @@ class TwigSandboxPolicy implements \Twig_Sandbox_SecurityPolicyInterface {
    */
   protected $whitelisted_classes = NULL;
 
+  /**
+   * Constructs a new TwigSandboxPolicy object.
+   */
   public function __construct() {
     // Allow settings.php to override our default whitelisted classes, methods,
     // and prefixes.

Added missing constructor on commit.

  • alexpott committed db091f3 on 8.0.x
    Issue #2513266 by mikeker, joelpittet, lauriii, Cottser, pwolanin,...
star-szr’s picture

Small follow-up, after talking to a colleague I'm of the opinion that we may have lost some valuable docs: #2610344: Re-add some documentation about what you can get from the node object in node.html.twig

  • alexpott committed db091f3 on 8.1.x
    Issue #2513266 by mikeker, joelpittet, lauriii, Cottser, pwolanin,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

pounard’s picture

This patch was, in my opinion, not the right way to go. Instead of whitelisting you should blacklist dangerous methods only on entities and potentially controllers.

I am putting a Symfony full stack into Drupal, and a lot of things explode because of this.

It's tempting to bump this to a critical, one of the reasons for moving to Twig is that we wouldn't be handing themers a loaded gun

This is where I do not agree, themers and integrators should be allowed to call methods on object, the whitelist is too restrictive, for example in my use case, I may whitelist "toArray" for the Symfony exception renderer, but I cannot guess what other methods are safe and I cannot whitelist everything the themer has the right to do. Because I cannot guess it, I would opt for a blacklist instead, Drupal knows better what's dangerous for Drupal, but it cannot guess what's safe for the rest of the world.

I mean, themers don't work on a prod (hopefully) and themers are supposed to know what they are doing.

joelpittet’s picture

@pounard, not knowing all the details of your use-case, I'd actually suggest we make this a service we can swap out. What do you think, and if you agree, maybe you can open a follow-up. This is not likely to change as the default in 8.x lifetime, but we can make it swappable.

quietone’s picture

Published the CR