Valid Twig syntax is incorrectly escaped in Views rewrites

Grrr... table formatting...

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

Updated issue summary.

Corrected HTML entities.

Attached patch adds a static function Xss::filterAdminTwig and unit tests.

Sorry, previous patch had some commented out code.

@joelpittet: I'll ping you on IRC next week to discuss. Thanks for the feedback.

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

Updated issue summary with a simplified example. Added remaining tasks.

@Fabianx: Here's the IRC conversation, trimmed of the occasional confusion and unrelated banter:

(8:46:00 PM) mikeker: joelpittet: Got a minute to talk about https://www.drupal.org/node/2466931?
(8:48:18 PM) mikeker: joelpittet: I wanted to see if you had any naming suggestions. You said you didn't like the idea of filterAdminTwig().
(8:48:20 PM) joelpittet: mikeker: I don't understand what problem it's solving
(8:48:50 PM) mikeker: joelpittet: The problem is that filterAdmin() removes standalone > and < characters.
(8:49:02 PM) mikeker: joelpittet: But thtat's valid in a Twig context.
(8:49:29 PM) mikeker: joelpittet: I suppose I should back up and say that this all part of rewriting field output using Twig.
(8:49:30 PM) joelpittet: mikeker: maybe it shouldn't be filtered, do you know how we are currently dealing with the body field?
(8:50:55 PM) mikeker: joelpittet: The body field is rendered using whatever text formatter is set for that display.
(8:52:14 PM) joelpittet: mikeker: kinda hoping views can be treated the same way
(8:53:07 PM) mikeker: joelpittet: I think i'm not explaining it well....
(8:53:34 PM) mikeker: joelpittet: Totally bogus example:
(8:54:46 PM) mikeker: joelpittet: If you rewrite a text field such that it truncates after 25 characters you could include Twig similar to: {% if field_test|length > 25 %} .... do something...
(8:55:17 PM) mikeker: joelpittet: But Xss::filterAdmin() will encode the > to &gt; and the Twig parser will barf.
(8:58:41 PM) joelpittet: mikeker: my suggestion is we should not use Xss::filterAdmin() on those fields at all.
(8:59:33 PM) joelpittet: mikeker: you're suggestion is on the safer side, but I think we should not be filtering the input from the admin interface and autoescape should be on by default.
(8:59:37 PM) mikeker: joelpittet: Isn't that going to be seen as a security regression? Granted, you need "admin views" permission to put text in a field rewrite. 
(8:59:46 PM) joelpittet: mikeker: hope not:D
(8:59:55 PM) joelpittet: mikeker: exactly!
(9:00:48 PM) joelpittet: mikeker: same with the body field, if I let my users have Full HTML they can put XSS attacks in if they want!
(9:01:01 PM) joelpittet: mikeker: and I think they should be able
(9:01:01 PM) mikeker: joelpittet: Just to clarify: if someone types a <script>...</script> in a template that's passed to Twig, it gets escaped?
(9:01:12 PM) joelpittet: mikeker: nope
(9:01:24 PM) joelpittet: mikeker: if that is in a variable then yes
(9:03:05 PM) mikeker: joelpittet: Hmmm.... Currently the Views field tokens are passed as variables (which could contain user-generated content) but the rewrite field is processed as an inline template.
(9:03:45 PM) joelpittet: mikeker: if that is safe content you can |raw it, but not recommended
(9:06:03 PM) mikeker: joelpittet: My worry is that we used to filter out XSS vectors in rewritten fields and now we won't be. Is adding an extra filter to Xss that bad?
(9:06:55 PM) joelpittet: mikeker: to me it's bloat and begs the question why choose one over the other...
(9:07:28 PM) joelpittet: mikeker: so i'm trying at all possible to keep things simple(ish)
(9:07:40 PM) mikeker: joelpittet: But if we add Twig processing in elsewhere (including contrib) we'll need something....
(9:07:52 PM) mikeker: joelpittet: (Normally I'm all for simple!  :)
(9:09:27 PM) joelpittet: mikeker: probably need to do some work with twig sandboxing (haven't played enough with it yet to say for sure how to implement)
(9:09:31 PM) mikeker: joelpittet: I was thinking specifically about using Twig to process Token replacements (though I realize that's not making it in 8.0)
(9:10:04 PM) joelpittet: mikeker: sounds like lots of little twig files in your file system...
(9:10:23 PM) mikeker: joelpittet: it looks like a forest over here.
(9:10:45 PM) joelpittet: mikeker: haha
(9:11:07 PM) mikeker: joelpittet: Actually, they could be handled as inline templates -- despite the bad rap those have gotten with PHP Filter, I'm convinced we can do it right with Twig.
(9:12:21 PM) mikeker: joelpittet: The Views rewrite stuff is using inline templates.
(9:12:41 PM) joelpittet: mikeker: fewf
(9:13:55 PM) mikeker: joelpittet: And before you say "code in the database" I would agrue that it's display logic. :)
(9:14:16 PM) joelpittet: mikeker: I'd like to explore no filters there... but maybe run it by Fabianx or chx?
(9:14:45 PM) joelpittet: mikeker: I wouldn't say that.  XSS won't hurt my DB:)
(9:14:59 PM) mikeker: joelpittet: ha!
(9:15:12 PM) joelpittet: mikeker: all for raw in the DB and filter on the way out!
(9:15:26 PM) mikeker: joelpittet: Will do re: pinging Fabianx and/or chx.
(9:18:33 PM) mikeker: joelpittet: Thanks for the feedback!
(9:18:38 PM) mikeker: joelpittet++
(9:18:46 PM) joelpittet: mikeker: no worries, thanks for thinking about it!
(9:18:49 PM) joelpittet: mikeker++

Where would that filter be needed?

@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 and Xss::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!

@Fabianx: Thank you for the detailed feedback! Just a couple quick comments.

Use a #post_render function for the inline template, that filters the XSS of the 'executed' / 'rendered' twig template.

I like this idea. One concern is that the inline template marks the result as safe. If there were a "

" 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?

better token parsing (or rather just checking for % and @ as prefix and ignoring everything else)

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.

From #16:

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.

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. See Renderer::doRender.

Unless there are objections to the #post_render approach, I'll start writing some tests for this.

This patch fixes the test that shows as broken in the post_render version of #21. I'll look at items raised in #24 shortly.

Actually, this one is better.

re: #28

Could you explain why this fixes it?

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.

Could we add a test explicitly checking for '<script>' not being present in the template after #post_render?

Yes. See 2466931-31-xss-twig-postrender.patch and regular interdiff.

Could we try removing SafeMarkup::set() from twig_render_template() and see what breaks?

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

Updated issue summary and title based on the new (#post_render) approach.

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

Not sure what's happening, but this test is passing on my localhost...

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -341,15 +342,18 @@ public function globalTokenReplace($string = '', array $options = array()) {
+    // The "Administer views" permission is required to enter what ends up as
+    // $text so we can use the more permissive XSS filter.
+    $filtered_text = Xss::filterAdmin($text);
+    if (empty($tokens) || empty($filtered_text)) {
+      return $filtered_text;
     }
 

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

Yes, you're right.

That was interesting... randomName() returns, well, random characters which sometimes include those that would be escaped by Xss::filterAdmin(). Which causes the test to fail... sometimes.

My apologies, testbot, for cursing you when you were just doing what you were told! :)

I was close in #53... The underlying problem is that if the string does not have any Views token delimiters ({{, !, or %) it is not run through Xss::filterAdmin. But it is if it does. That's not good.

/me: starts writing more tests...

It seems the output from tokenizeValue is filtered later in the render pipeline, so this is only an issue for tests. I've updated StylePluginBase::tokenizeValue to return Xss::filterAdmin'ed text regardless of whether there are Views token delimiters or not.

Thanks for the review @joelpittet. Agreed about pinging @dawehner -- hadn't thought about @timplunkett, thanks for the suggestion.

Do we need to do this twice?

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.

Maybe worth keeping the simple test case in there too?

The names in the test are the names of the Beatles so this sorta keeps the simple test while adding Twig syntax. :)

Grrr... Figured there was some test somewhere that would get tripped up by empty. Using strlen instead.

re: #63:

Item 2:

Seems to be a little bit of an overoptimization, is there a reason for that? Do we often have an empty text there?

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:

Do we really need an empty #post_render entry here?

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.

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

Thanks for the review @alexpott. From #70:

Why is twig auto escape not protecting us?

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.

Can we call SafeMarkup::checkAdminXss instead here?

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?

@alexpott, thanks for the clarification. I'll reroll shortly...

Updated as per #75.

Seems like a random test failure -- it passes on my localhost.

Retesting....

As per #77.

Use Xss::filterAdmin() no need to use the SafeMarkup version.

Done.

Not sure what happened to my uploads. Here they are again.

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

As requested, this patch is essentially #84 but with s/SafeMarkup::checkAdminXss/Xss::filterAdmin/g. Let's see what the testbot says.

Ugh... That's what I get when I try to do things right before bed! :)

Not sure why this test is failing for the testbot. It passes on Simplytest.me and on my localhost... Retrying one more time.

New 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 of SafeMarkup::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?