Beta phase evaluation
Issue category | Bug: Valid Twig syntax (eg: conditional with greater/less than) will throw an exception. |
---|---|
Issue priority | Normal: only affect one area of functionality |
Prioritized changes | Yes: Bug fix, security enhancement, and follow up from a major change (#2342287: Allow Twig in Views token replacement). |
Disruption | None. Does not change existing API or site builder workflow. |
(Issue summary updated as of comment #68)
Problem/Motivation
Now that we can use Twig in Views rewrites, we need to respect Twig syntax while filtering for possible XSS vulnerabilities. User-generated content (tokens) that may be used in Views rewrites is auto-escaped by Twig. Thus we only need to validate the rewrite template entered by the site builder. XSS exposure is mitigated by the fact that "Administer Views" permissions are needed to rewrite Views' output.
Additional information
This change is essential to allow #2342287: Allow Twig in Views token replacement to be used to it's full potential. Otherwise, code such as this (which is exactly what result rewriting is all about!) results in a Twig parser error because the ">
" character was converted to ">
"
{% if field_example|length < 50 %}
{{ field_example }}
{% else %}
This field is too long to display
{% endif %}
Proposed resolution
Add a #post_render
function to the inline-template that calls Xss::filterAdmin
to remove XSS vulnerabilities after token replacement. This will improve on D7 security -- previously rewritten strings were filtered for XSS vulnerabilities before, not after, token replacement allowing tokens to be combined into potential security holes.
Remaining tasks
- Moot (using different approach with #post_render):
Settle concerns raised in #8 and #11 regarding code bloat and confusion over which filter to use when. - Moot (using different approach with #post_render):
Determine if removing theXss::filterAdmin()
is an option or a security regression. - #21
Add -test-only patch to demonstrate the issue. Add patch to fix the issue.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#99 | 2466931-90-99.interdiff.txt | 2.13 KB | mikeker |
#99 | 2466931-99-xss-twig-postrender.patch | 8.79 KB | mikeker |
#90 | 2466931-88-90.interdiff.txt | 569 bytes | mikeker |
#90 | 2466931-90-xss-twig-postrender.patch | 8.45 KB | mikeker |
#88 | 2466931-84-88.interdiff.txt | 1.97 KB | mikeker |
Comments
Comment #1
mikeker CreditAttribution: mikeker commentedComment #2
mikeker CreditAttribution: mikeker commentedGrrr... table formatting...
Comment #3
mikeker CreditAttribution: mikeker commentedBased on feedback from @YesCT, I'm re-categorizing this as a task since it is a "minor API changes such as adding new hooks."
Note: adding this to core would let something like #2055597: Allow multi-value field rendering to pass through Views rewrite only once move to contrib (I believe, haven't verified that). But without this in core, there's no chance of #2055597: Allow multi-value field rendering to pass through Views rewrite only once working and #2342287: Allow Twig in Views token replacement has essentially been shot in the foot.
Comment #4
mikeker CreditAttribution: mikeker commentedUpdated issue summary.
Comment #5
mikeker CreditAttribution: mikeker commentedComment #6
mikeker CreditAttribution: mikeker commentedCorrected HTML entities.
Comment #7
mikeker CreditAttribution: mikeker as a volunteer commentedAttached patch adds a static function
Xss::filterAdminTwig
and unit tests.Comment #8
joelpittetI'd try not to name it after "Twig" or for the purpose of Twig if possible because all of the filters are to exclude unsafe markup from Twig.
There must a better solution that a new filter mechanism and maybe it's more permissive twig markup in views.
Comment #9
mikeker CreditAttribution: mikeker as a volunteer commentedSorry, previous patch had some commented out code.
@joelpittet: I'll ping you on IRC next week to discuss. Thanks for the feedback.
Comment #10
star-szrComment #11
mikeker CreditAttribution: mikeker as a volunteer commentedTalked with @joelpittet on IRC. In summary: his concern is about unnecessary bloat and the confusion of having two Xss::filterAdmin type functions. Since editing this field requires "administer views" permissions, he suggested removing the XSS filtering on this field as a possible solution.
I have some concerns about that as it could be seen as a security regression vs D7. I'm going to try and rustle up some more opinions on this...
Comment #12
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated issue summary with a simplified example. Added remaining tasks.
Comment #13
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWhere would that filter be needed? Can the conversions be shown as well, please?
Comment #14
mikeker CreditAttribution: mikeker as a volunteer commented@Fabianx: Here's the IRC conversation, trimmed of the occasional confusion and unrelated banter:
Comment #15
mikeker CreditAttribution: mikeker as a volunteer commented@Fabianx: The long answer is above. The short answer is that we (in D7) ran field rewrites through
filter_xss_admin
. I assumed we would want to do the same in D8. But D8 now allows Twig syntax in rewrites andXss::filterAdmin
removes things like standalone < and > which is valid in a Twig syntax.I would like to get confirmation that removing filterAdmin will not be considered a security regression before I try pushing for that.
Thanks!
Comment #16
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedSo I also have a short and a long answer.
In short:
+1 to using twig templates for tokens
+1 to allowing more complex logic
-1 for the chosen implementation of doing a pre-parse for {{ }} (I will comment a post-commit review on the original issue for follow-up)
-1 to not enabling sandbox mode (no don't want to allow functions by default)
+1 to allow '>' in twig templates
+1 to keep XSS security
-1 to the new filter solution - that feels wrong
Proposed better solution:
Use a #post_render function for the inline template, that filters the XSS of the 'executed' / 'rendered' twig template.
I am not sure that will work, but it feels the most secure. (Lets write some tests for it)
What I would like to see (not all in this issue, but in general) is:
a) A test-only patch using a '<' in the twig template in a view filter field, so that we see where core fails with the code right now
b) A successful patch that makes the test-only patch pass - using the new filter (so it can be evaluated)
c) Another successful patch that uses XSS::filterAdmin() on the "result" of the twig template, to prevent early rendering this should be done via a #post_render callback
d) Some more tests to ensure this works in many cases.
Independently lets open issues for:
a) sandbox mode -- maybe allow url / path / link as functions, but restrict everything else
b) better token parsing (or rather just checking for % and @ as prefix and ignoring everything else)
I hope that helps.
Comment #17
mikeker CreditAttribution: mikeker as a volunteer commented@Fabianx: Thank you for the detailed feedback! Just a couple quick comments.
I like this idea. One concern is that the inline template marks the result as safe. If there were a "
...malicous code..." in the template, could it be marked as safe before then getting filtered? Couldn't that allow the same mallicious code through elsewhere in the render pipeline?
Is #2396607: Allow Views token matching for validation (and remove dead code) what you're talking about? It doesn't actually change the parsing but it does put it all in one place.
Comment #18
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#17:
Wow, excellent thought. Yes, this would be a rare case, where we would undo a mark-safe if there was a difference detected. In which case we should probably do both your new filter and the #post_render.
--
The better token parsing was more my concern of doing a live strtr() on user input twig template, that feels wrong. It is covered here:
https://www.drupal.org/node/2492839 (my points 1 and 3 are actually invalid after discussion with dawehner) ...
Comment #19
cilefen CreditAttribution: cilefen commentedMight we want to validate the twig syntax too?
Comment #20
star-szrYep :)
Comment #21
mikeker CreditAttribution: mikeker as a volunteer commentedFrom #16:
a) Done: 2466931-21-xss-twig-filter-tests-only.patch
b) Done: 2466931-21-xss-twig-filter.patch (includes the test from a, see interdiff)
c) Done: 2466931-21-xss-twig-filter-post_render.patch
d) To do...
I love the
#post_render
option suggested by @Fabianx! Much cleaner and it fixes a long-standing (D7 and probably D6) potential XSS hole with field rewrites where malicious code is split in half and then recombined from two tokens (think first and last name fields rewritten into a full name).My concern raised in #17 is not an issue.
SafeMarkup::set
is called after the#post_render
functions are called. SeeRenderer::doRender
.Unless there are objections to the
#post_render
approach, I'll start writing some tests for this.Comment #24
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#21: Great patches! Much clearer now what is the scope for the usage.
Unfortunately the concerns in #17 are still warranted as the output of twig_render_template itself is marked as safe currently, which is probably not needed as its only used ever within render arrays or we could ask you to mark things safe yourself ...
Should probably re-visit that ::set call after #2273925: Ensure #markup is XSS escaped in Renderer::doRender() is in / edit: could do directly, no need to wait as we deal only with #theme, not #markup here.
Could you open an issue for using the sandbox mode for those inline templates, please? Thanks!
Comment #25
mikeker CreditAttribution: mikeker as a volunteer commentedThis patch fixes the test that shows as broken in the post_render version of #21. I'll look at items raised in #24 shortly.
Comment #26
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#25: Could you explain why this fixes it? /me curious ...
Comment #29
mikeker CreditAttribution: mikeker as a volunteer commentedActually, this one is better.
re: #28
This is a unit test so we hvae to mock the input and output of
render()
. In this case, we added a parameter to the$build
param in the code and thus need to reflect that change in the mocked object.... At least that's what my admittedly limited understanding of PHPUnit leads me to believe.Comment #30
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI think the #post_render approach looks pretty good - test wise.
Could we add a test explicitly checking for '
<script>
' not being present in the template after #post_render?Could we try removing SafeMarkup::set() from twig_render_template() and see what breaks?
As far as I can see that call does nothing except burn memory, because in case there is a prefix or suffix we trust what comes from theme anyway, so not needed to mark safe.
Comment #31
mikeker CreditAttribution: mikeker as a volunteer commentedYes. See 2466931-31-xss-twig-postrender.patch and regular interdiff.
Yes as well: see 2466931-31-no-SafeMarkup-set.patch with the interdiff being between the two patches. But I think it's going to break a lot of things. At least, it should -- in my five second evaluation, anything output by
links.html.twig
is double-escaped. Perhaps other template files as well? I haven't looked into why.Finally, I don't think
twig_render_template
is a factor here as it's not called (as far as I can see) when rendering an inline template. Please let me know if I'm missing it! Though if it is just burning memory, we should still investigate! :)(Edit: properly escaped HTML...)
Comment #35
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#31: Oh, you are totally right, I forgot that inline templates use a different way.
We should be good then :).
Disregard the SafeMarkup::set removal then ...
Thanks for the patch for the #post_render.
We should be ready here soon. (need: final --tests patch and final normal patch for reviews)
Architecture is sane now! :)
Comment #37
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated issue summary and title based on the new (#post_render) approach.
Comment #38
mikeker CreditAttribution: mikeker as a volunteer commentedAlrighty.... I think this should give us what we need to evaluate this. Attached is a -tests-only patch that demonstrates the problem (and adds some additional verification to ensure script tags are being removed correctly) and a patch with the fix.
Comment #44
mikeker CreditAttribution: mikeker as a volunteer commentedNot sure what's happening, but this test is passing on my localhost...
Comment #45
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedToo bad we need to call Xss:filterAdmin twice here on average, but I don't see how we can change that as we need to call it to see if its empty - I think ...
Nice!
--
Overall looks great to me, not sure we can solve #1, but besides that RTBC.
So setting to that state, to get committer feedback.
Comment #46
mikeker CreditAttribution: mikeker as a volunteer commentedThe current code only checks if $token is empty. I added the $filtered_text variable assuming I would need it later. That turned out not to be the case. In hindsight, especially considering this can be called several time for each row in a given view result set, I don't think we should do this...
Another patch coming shortly.
Comment #47
mikeker CreditAttribution: mikeker as a volunteer commentedComment #48
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedNope, still need to filter here in the return case ...
Comment #49
mikeker CreditAttribution: mikeker as a volunteer commentedYes, you're right.
Comment #51
mikeker CreditAttribution: mikeker as a volunteer commentedComment #53
mikeker CreditAttribution: mikeker as a volunteer commentedThat was interesting...
randomName()
returns, well, random characters which sometimes include those that would be escaped byXss::filterAdmin()
. Which causes the test to fail... sometimes.My apologies, testbot, for cursing you when you were just doing what you were told! :)
Comment #55
mikeker CreditAttribution: mikeker as a volunteer commentedI was close in #53... The underlying problem is that if the string does not have any Views token delimiters (
{{
,!
, or%
) it is not run throughXss::filterAdmin
. But it is if it does. That's not good./me: starts writing more tests...
Comment #56
mikeker CreditAttribution: mikeker as a volunteer commentedIt seems the output from
tokenizeValue
is filtered later in the render pipeline, so this is only an issue for tests. I've updatedStylePluginBase::tokenizeValue
to returnXss::filterAdmin
'ed text regardless of whether there are Views token delimiters or not.Comment #57
joelpittetThis may be good to have @dawehner or @timplunkett have a peak at too.
Couple of questions:
Do we need to do this twice?
Maybe worth keeping the simple test case in there too?
I'm general +1 on the new plan though:)
Comment #58
mikeker CreditAttribution: mikeker as a volunteer commentedThanks for the review @joelpittet. Agreed about pinging @dawehner -- hadn't thought about @timplunkett, thanks for the suggestion.
Yes. It is needed to ensure the output
viewsTokenReplace
is always sanitized. Previously, if there are no$tokens
, I was returning$text
unfiltered. See #48.I suppose splitting the conditional into two parts (one for
$text
and one for$token
) might be more performant... (Xss::filterAdmin
doesn't short circuit on an empty string). Updated patch attached along with an update to the function's docblock.The
name
s in the test are the names of the Beatles so this sorta keeps the simple test while adding Twig syntax. :)Comment #60
mikeker CreditAttribution: mikeker as a volunteer commentedGrrr... Figured there was some test somewhere that would get tripped up by
empty
. Usingstrlen
instead.Comment #61
mikeker CreditAttribution: mikeker as a volunteer commentedComment #62
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, from my side, lets get views maintainer review on it.
Comment #63
dawehner+1 to document more explicit what is going on
Seems to be a little bit of an overoptimization, is there a reason for that? Do we often have an empty text there? At least in the places I have checked, there is always some sort of
if()
wrapping this before. On the other hand the if() here is basically for freeDo we really need an empty #post_render entry here?
Comment #64
mikeker CreditAttribution: mikeker as a volunteer commentedre: #63:
Item 2:
Yeah, it may be. In my experience you only see this if you opt to rewrite the field to an empty string. But I wasn't sure about other situations.
The change was in response to #57 and my concern was that
Xss::filterAdmin
runs through it's full complement of string replacements on an empty string. I figured better safe than sorry.Item 3:
Yes. Since we're mocking the Renderer object, we need to match the incoming parameter list which now includes a
#post_render
key in the build array.Comment #65
dawehnerGot it, thank you!
Comment #66
tim.plunkettComment #67
dawehnerSo its still RTBC from my POV
Comment #68
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThat is a bug and we need a beta evaluation, thanks to the views maintainers for chiming in.
Comment #69
mikeker CreditAttribution: mikeker as a volunteer commented@Fabianx, beta eval already exists in the issue summary. I've updated it to treat this as a bug instead of a task as suggested by #68. Also updated the issue summary to show the current status.
Comment #70
alexpottCan we call SafeMarkup::checkAdminXss instead here?
Comment #71
mikeker CreditAttribution: mikeker as a volunteer commentedThanks for the review @alexpott. From #70:
It is, but autoescape only works on the variables passed into a Twig template, not the template itself. In this case, the template (the field rewrite) is the potentially unsafe code (mitigated by the need for the "admin Views" permission). Also, since an XSS hack could be split up into multiple parts and reassembled via a rewrite, it's best to filter for XSS hacks as late as possible.
We could but I don't see the advantage. Renderer::doRender will mark the string as safe following the #post_render callback. Is the concern about potentially double-escaping the string?
Comment #72
alexpott@mikeker good answers. The reason I was asking for
SafeMarkup::checkAdminXss()
is that atmXss::filterAdmin()
also marks stuff as safe (becauseFilter::Xss()
does) but it won't always do this - see #2506195: Remove SafeMarkup::set() from Xss::filter() but we need to here. Sorry I should have included more detail on why I was asking for that.Comment #73
mikeker CreditAttribution: mikeker as a volunteer commented@alexpott, thanks for the clarification. I'll reroll shortly...
Comment #74
mikeker CreditAttribution: mikeker as a volunteer commentedComment #75
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWe also in the future won't need to mark this safe as that will happen after #post_render automatically.
So we can leave this one.
I first thought it was a bug that the patch did not fail, but apparently its not marked as safe by the inline entity markup #pre_render'ing.
Comment #76
mikeker CreditAttribution: mikeker as a volunteer commentedUpdated as per #75.
Comment #77
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #79
mikeker CreditAttribution: mikeker as a volunteer commentedSeems like a random test failure -- it passes on my localhost.
Retesting....
Comment #81
mikeker CreditAttribution: mikeker as a volunteer commentedAs per #77.
Comment #82
alexpottIs this actually necessary?
Use Xss::filterAdmin() no need to use the SafeMarkup version.
Comment #83
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#82.1: Yes, it is, because it was present before. Else no admin filtering is done at all.
Comment #84
mikeker CreditAttribution: mikeker as a volunteer commentedDone.
Comment #85
mikeker CreditAttribution: mikeker as a volunteer commentedNot sure what happened to my uploads. Here they are again.
Comment #86
mikeker CreditAttribution: mikeker as a volunteer commentedTo expand on what @Fabianx says in #83, "no admin filtering is done at all if there are no token replacements in the rewrite string." Granted it requires admin views permissions to edit a Views field rewrite. Also if
tokenizeValue
always returns safe markup, it makes it easier to determine if there is safe markup elsewhere in the Views render pipeline.Comment #87
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWhile we replace Xss::filterAdmin with checkAdminXss, this is okay, because before the $value was marked safe by the renderer implicitly.
However given the code in HEAD did not fail with the Remove SafeMarkup::set form Renderer::doRender(), it would likely be okay to use SafeString here as well.
Or even don't do anything.
One would need to check if there is new double escaping, if this returned just plain.
So maybe we should use:
Xss::filterAdmin here everywhere and see if it breaks?
Comment #88
mikeker CreditAttribution: mikeker as a volunteer commentedAs requested, this patch is essentially #84 but with
s/SafeMarkup::checkAdminXss/Xss::filterAdmin/g
. Let's see what the testbot says.Comment #90
mikeker CreditAttribution: mikeker as a volunteer commentedUgh... That's what I get when I try to do things right before bed! :)
Comment #94
mikeker CreditAttribution: mikeker as a volunteer commentedNot sure why this test is failing for the testbot. It passes on Simplytest.me and on my localhost... Retrying one more time.
Comment #99
mikeker CreditAttribution: mikeker as a volunteer commentedNew tests were failing because of the new render context. But I still don't know why they didn't fail using the Simpletest UI... As mentioned in IRC, running tests using run-test.sh vs Batch API is "just pure WTF."
Anyhow, this should fix things.
@Fabianx: This should have everything going through
Xss::filterAdmin()
instead ofSafeMarkup::checkAdminXss()
. I haven't found any double-escaping in my (limited) manual testing and there doesn't seem to be anything showing up in the automated tests.Are we good here?
Comment #100
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLooks great!
Sorry for the back and forth and thanks for following up!
Comment #101
alexpottThis is nice work and nice tests. Committed cc17945 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
contemplate in core ftw!
Comment #104
FreeAndEasy CreditAttribution: FreeAndEasy commentedComment #105
FreeAndEasy CreditAttribution: FreeAndEasy commented(reopening, see here: https://www.drupal.org/node/2542764)
HTML in the rewritten output is escaped when it shouldn't ("The text to display for this field. You may include HTML or Twig. You may enter data from this view as per the "Replacement patterns" below.").
Comment #106
cilefen CreditAttribution: cilefen commented@FreeAndEasy I think "Closed (fixed)" issues are not to be reopened. It is better to go with your follow-up issue.