Beta phase evaluation
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
Decide whether to use a Sandbox policy or "ViewModel" or both strategies.Use Sandbox policyTry and see how much work and/or pain these will have on DXWhile adding $settings to a settings.php is not ideal, it does allow these whitelists to be customized.- Follow-up issue: Allow Sandbox policies to be added as services
Ensure that the chosen strategy is swappable via a Sandbox Policy Service or whitelist property extension/trait or otherNow configurable via$settings
.Find sane defaults for whitelistDone, though the sooner we get this in, the sooner we can figure out other methods/prefixes that should be added to the default lists./blacklistor helper methods to add to entities and other objects passed to templates.
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?
Comment | File | Size | Author |
---|---|---|---|
#126 | increment.txt | 4.28 KB | pwolanin |
#126 | 2513266-126.patch | 16.29 KB | pwolanin |
#121 | 2513266-106-121.interdiff.txt | 2.33 KB | mikeker |
#121 | 2513266-121.patch | 16.38 KB | mikeker |
#106 | 2513266-100-106.interdiff.txt | 4.61 KB | mikeker |
Comments
Comment #1
LewisNyman CreditAttribution: LewisNyman at Wunder commentedIt'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.
Comment #2
stevectorThere 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.
Comment #3
star-szrAfter 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.
Comment #4
joelpittetComment #5
star-szrRough proof of concept. Could use a test to show that it works, here's a screenshot from a manual test:
Comment #6
lauriiiWorking on the tests
Comment #7
lauriiiComment #9
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented+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.
Comment #10
stevectorDoes 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.
Comment #11
star-szrI 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 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.
Comment #12
mikeker CreditAttribution: mikeker as a volunteer commentedThat 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()
andhasFoo()
functions.Comment #13
star-szrYup. @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.
Comment #14
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #15
mikeker CreditAttribution: mikeker as a volunteer commentedI've updated #7 to be a whitelist instead of a blacklist. Currently whitelisted are:
The last three are using a more expensive
preg_match
instead ofstrpos
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 tonode.bundle
now need to benode.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.
Comment #17
mikeker CreditAttribution: mikeker as a volunteer commentedWorking on fixing the test failures.
Comment #18
mikeker CreditAttribution: mikeker as a volunteer commentedThis seems to be at the root of a lot of them...
Comment #19
star-szrThanks @mikeker!
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.
Comment #20
mikeker CreditAttribution: mikeker as a volunteer commented@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?Comment #21
mikeker CreditAttribution: mikeker as a volunteer commentedComment #22
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedHmmm,
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.
Comment #23
star-szrWell 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 ;)
Comment #24
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #25
star-szrI'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 :(
Comment #26
mikeker CreditAttribution: mikeker as a volunteer commentedUnderstand 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 anEntityReferenceFieldItemList
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 thetype
property while all ofnode.Type
,node.TYPE
,node.gettype
,node.GetType
, andnode.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.
Comment #27
mikeker CreditAttribution: mikeker as a volunteer commentedI 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
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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*.
Comment #29
star-szr@mikeker I would strongly argue against removing the magic variable resolution, it's one of the most compelling features of Twig IMO.
Comment #30
mikeker CreditAttribution: mikeker as a volunteer commented@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 thatsome.action
leads tosome.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.
Comment #31
mikeker CreditAttribution: mikeker as a volunteer commented#28, @Fabianx:
No,
bundle()
is a (poorly named) getter that returns a string, buttype
doesn't show up in the API docs for the Node object, yet resolves to anEntityReferenceFieldItemList
object in the Twig template when accessed vianode.type
.Comment #32
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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.
Comment #33
joelpittet@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.
Comment #34
alexpottI think at this point sandboxing methods on entities is the way to go. It's the least intrusive.
in_array()
Can be one pattern
/^(get|has|is)[A-Z]
...get
can be in the exact match whitelist.I think bundle should be in the whitelist.
Comment #35
catchMakes sense to do the sandboxing to me as well.
Comment #36
joelpittetThank you @alexpott and @catch! Now we have a way forward:)
Comment #37
joelpittetAnd @mikeker whitelisting seems like a good wait to go instead of blacklisting, thank you.
Comment #38
mikeker CreditAttribution: mikeker as a volunteer commentedAddresses items 1 - 3 in #34. No idea how to deal with #4. :)
Also, changed the return values to
return TRUE
to matchcore/vendor/twig/twig/lib/Twig/Sandbox/SecurityPolicy.php
and added tests for the exact-match whitelisted functions.Comment #39
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWe should be able to switch those back now, right?
Lets use 'id' => TRUE, 'label' => TRUE, ...
and then use isset(), which is slightly faster.
I think we should make that configurable.
Comment #40
star-szrFor #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!
Comment #41
mikeker CreditAttribution: mikeker as a volunteer commented@Fabianx, thanks for the review! Some quick replies to #39:
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.Great idea. Thanks! Will add that in the next iteration.
Comment #42
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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. :)
Comment #43
lauriiiI 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.
Comment #44
star-szrI have to ask: What do we actually mean when we say configurable? How would it be configurable? services.yml?
Comment #45
lauriiiDrupal 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
Comment #46
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#44: Yes, just put the twig_whitelist_regexp with 'bundle|...' in services.yml as a container parameter like twig_debug.
Comment #47
lauriiiFabianx: that would be the easiest way but what if a module needs that to be configured?
Comment #48
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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?
Comment #49
lauriiiI 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.
Comment #50
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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. :)
Comment #51
star-szrI like the sounds of that!
Comment #52
lauriiiI 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.
Comment #53
mikeker CreditAttribution: mikeker as a volunteer commentedThe attached patch deals with the issues raised in #39:
isset()
instead ofin_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.
Comment #54
joelpittetLet's see what this looks like from testbot.
Comment #56
mikeker CreditAttribution: mikeker as a volunteer commented@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
becomescomment.getowner()
) instead of the resolved (comment.owner
becomescomment.getOwner()
sincecomment.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?
Comment #57
mikeker CreditAttribution: mikeker as a volunteer commentedI remembered this time! ... sorta...
Comment #58
joelpittet@mikeker nice catch and it's green:) I always for get to do NR, going to have to patch dreditor to prevent my forgetfulness:)
may be simpler to just add a
/i
flag at the end? it would catch 3rd parth classes that for silly reasons useGetName()
instead ofgetName()
conventions.Comment #59
joelpittetThese two lines look over 80 chars in dreditor but they aren't. So no worries.
nit: This whitespace change isn't needed.
ditto 80, don't worry about it. Dreditor issue;)
ditto 80, don't worry about it. Dreditor issue;)
Comment #60
mikeker CreditAttribution: mikeker as a volunteer commentedI 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. :)
Fixed.
Thanks for the review @joelpittet. And let me know when you patch Dreditor to remind you to set an issue to NR!
Comment #61
larowlanLooks great, especially tests, couple of questions and observations - thanks!
Can we give this a bit longer description that indicates the role it serves for those not familiar with what a sandbox policy is?
What happens here with other objects? Not sure there are any but e.g. Attribute comes to mind for example?
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?
nit: > 80
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
Any reason we can't make this a data provider and then we can use the @expectedException, @expectedExceptionMessage annotations?
nit:Needs a docblock
Comment #62
star-szr@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).
Comment #63
mikeker CreditAttribution: mikeker as a volunteer commented@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.
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...
Comment #64
mikeker CreditAttribution: mikeker as a volunteer commentedMissed a commit when making the previous patch...
Comment #66
joelpittetRe
iStopEverythingFromWorking()
, LOL and good point:)Comment #67
mikeker CreditAttribution: mikeker as a volunteer commentedAddressing @larowlan's remaining points from #61:
Since the node object implements
__get
,node.field_image
returns aDrupal\file\Plugin\Field\FieldType\FileFieldItemList
object. Objects without a __get() method would have to implement eitherfield_foo()
orgetField_foo()
to work as expected.In your example, you would have to write
node.field_image.entity.uri[0].value
to getpublic://field/image/gen9C56.tmp.jpeg
out. Not sure if that's what is supposed to come out of aUriItem
object or not...@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?Thanks, again, for the awesome review!
(Edit: fixed dorked up HTML...)
Comment #68
mikeker CreditAttribution: mikeker as a volunteer commentedRealized I didn't address this:
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?
Comment #69
joelpittetComment #70
joelpittet@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'Comment #71
mikeker CreditAttribution: mikeker as a volunteer commentedIn the process of converting this to a service collection.
Comment #72
pwolanin CreditAttribution: pwolanin at Acquia commentedIs there any indication of the performance impact of Sandbox Mode?
I'm not sure what a service collection means in terms of this patch?
Comment #73
webchickThis sounds potentially horrifying.
Comment #74
catchSo 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
directinstant 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.
Comment #75
larowlanworking on test, will send back to mikeker when done
Comment #76
plachGiven #74, is this actually critical?
Comment #77
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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...
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.
Comment #78
webchick#74 is a good argument for major.
Comment #79
pwolanin CreditAttribution: pwolanin at Acquia commentedSo 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?
Comment #80
larowlanHere'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
Comment #81
joelpittet@pwolanin re #79 this is likely because it doesn't know what it's acting on until runtime.
Example:
Comment #82
dawehnerI'm curious whether we want to use strpos 3 times instead for better performance
80 chars
Did you considered to use a new kernel test base?
This should be split into multiple methods
80 chars
Comment #83
mikeker CreditAttribution: mikeker as a volunteer commentedSorry, I've been offline for a couple days... So, to reply to a bunch of questions in the last week of so:
#72:
No profiling has been done yet -- this may be my excuse to learn xhprof. :)
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 inTwigSandboxPolicy.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:
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?
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?
Those are an oddity with Dreditor when the line is exactly 80 chars long. See #59.
Comment #84
pwolanin CreditAttribution: pwolanin at Acquia commentedUsing a service is going to make this all that much slower than a single sandbox?
Comment #85
lauriiiThese 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.
Comment #86
star-szrI 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
Comment #87
lauriiiOh sorry for missing that. Then we are still missing the profiling :)
Comment #88
mikeker CreditAttribution: mikeker as a volunteer commented(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:
1. Profiling results for the default home page view:
2. Profiling results for an article page with comments enabled and an image for both the author and article:
Comment #89
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #90
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAs 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.
Comment #91
catchComment #92
mikeker CreditAttribution: mikeker as a volunteer commentedIn 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 betwig_sandbox_additional_whitelisted_methods
instead which would array_merge with our default whitelist?Comment #93
mikeker CreditAttribution: mikeker as a volunteer commentedDid 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 showsisset()
to be about 3x faster thanis_null()
.)Then did some more profiling with this latest patch using 5 users, 10 tags, 50 nodes and the default frontpage view:
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!
Comment #94
joelpittetNeeds a slight re-roll, but applies with
patch -p1
. I'll post in a few after profiling.Comment #95
joelpittetWish I could upload but I lost the data files on the plane. Though the results are very similar.
Comment #96
joelpittetVery similar results 2-3% wall time regression. 50 nodes on the homepage.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fc0948e739d&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55fc0948e739d&...
Comment #99
joelpittetReal patch now:) Thanks for the heads up @Cottser
3way auto merge
Comment #100
mikeker CreditAttribution: mikeker as a volunteer commentedAnd another reroll after #2513266: Twig templates can call delete() on entities and other objects landed...
Comment #101
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated issue summary. Added beta eval.
Comment #103
star-szrI'm not going to bother hitting re-test on the old bot now, it's pretty backed up. But setting back to needs review.
Comment #104
star-szrOverall this is looking great and I think it's very very close. Awesome work @mikeker and @joelpittet and everyone :)
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:
Do these get docblocks now? inheritdoc?
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
Comment #105
mikeker CreditAttribution: mikeker as a volunteer commentedWhile 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.
Comment #106
mikeker CreditAttribution: mikeker as a volunteer commentedI'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 offoo 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.
Comment #107
mikeker CreditAttribution: mikeker as a volunteer commentedProfiled #106 using the same setup as #93 (5 users, 10 tags, 50 nodes, default frontpage view):
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.
Comment #108
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - 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.
Comment #110
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedTest fail looks unrelated
Comment #112
pwolanin CreditAttribution: pwolanin at Acquia commentedAn injected service seems like it would be adding extra complexity of dubious value, unless it's also allowing us to remove the settings support?
Comment #113
mikeker CreditAttribution: mikeker as a volunteer commented@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!
Comment #114
RainbowArrayLooks like an unrelated fail.
Comment #118
RainbowArrayOh dang, this didn't get in. Tagging this for triage since this was a potential concern about content getting deleted.
Comment #119
mikeker CreditAttribution: mikeker as a volunteer commentedNot 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.
Comment #120
pwolanin CreditAttribution: pwolanin at Acquia commentedCould 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?
Comment #121
mikeker CreditAttribution: mikeker as a volunteer commentedre: #120:
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.
Yes we are -- thank you for pointing that out! Fixed.
Comment #122
joelpittetJust noticed this is called 'attributes'. Shouldn't this be called 'properties' or 'public member variables'? Nitpik.
Comment #123
pwolanin CreditAttribution: pwolanin at Acquia commentedShould fix the docs. Are properties ever unsafe to access? Seems like this comment might be incorrect in general?
Comment #124
pwolanin CreditAttribution: pwolanin at Acquia commented@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) {}
Comment #125
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedYep, CNW for the comment, that is indeed misleading.
Comment #126
pwolanin CreditAttribution: pwolanin as a volunteer commentedSmall doc and code fixes.
Comment #127
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC then :)
Comment #128
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated issue summary -- the info about changing the whitelist in
$settings
was out of date.Comment #129
mikeker CreditAttribution: mikeker as a volunteer commentedAdded a draft CR.
Comment #130
mikeker CreditAttribution: mikeker as a volunteer commentedAdded followup issue: #2595805: [Followup] Implement the Twig Sandbox Policy as a service collection
Comment #131
xjmDiscussed with the D8 committers and we agreed with making this an rc target.
Comment #132
alexpottI 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!
Added missing constructor on commit.
Comment #134
star-szrSmall 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
Comment #137
pounardThis 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.
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.
Comment #138
joelpittet@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.
Comment #139
quietone CreditAttribution: quietone at PreviousNext commentedPublished the CR