Arbitrary code execution via 'trans' extension for dynamic twig templates (when debug output is on)

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
@@ -94,7 +94,7 @@ public function compile(\Twig_Compiler $compiler) {
-      $compiler->raw(" -->\n'");
+      $compiler->raw(" . '-->\n'");

Fixing the minor whitespace here can be done by a new contributor and should fix the tests :)

Starting on this at DrupalCon LA Sprints

After spending considerable time today on the ticket with Joel and Lauriii. This is what we came up with.

This patch does address the noted bug. This also introduces other things that affect the build of the test output when options

A noted way it breaks:
When the test has some $options, it breaks here. There is an extra quote being added to the $singular variable that causes the code being generated to break here when trying to concat the string together.

Adding addcslashes() around the $singular variable will give a green pass on the local tests but not the best idea, but proves a point.

Kinda like the following:

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
@@ -85,7 +85,7 @@ public function compile(\Twig_Compiler $compiler) {
     // Append translation debug markup, if necessary.
     if ($compiler->getEnvironment()->isDebug()) {
       $compiler->raw(" . '\n<!-- TRANSLATION: ' . ");
-      $compiler->subcompile($singular);
+      $compiler->subcompile(addcslashes($singular));
       if (!empty($plural)) {
         $compiler->raw(', PLURAL: ')->subcompile($plural);
       }

Essentially, the goal is to escape any passed in extra quotes.

How I was testing. With using the GUI in Drupal sometimes too.

php core/scripts/run-tests.sh --url http://d8.local/ --color --verbose --class "Drupal\system\Tests\Theme\TwigTransTest::testTwigTransDebug"

Side note: It was also brought up, that it would be good to write a test for the this ticket's original focus:

'{% trans %}Hello sun. - \' . die(\'I am bad code!\') . \'{% endtrans %}' => '<!-- TRANSLATION: "Hello sun. die(\'I am bad code!\')" -->',

At this point, I am trying to just finish up the thoughts and learning so if i missed anything, sorry. I'll look again this weekend sometime.

@zeropx I really didn't intend to throw you into the deep end but you can swim quite well ;) thanks for all that!

@Cottser no worries! I really enjoyed the experience. In fact, if i wasn't put on something like this, I wouldn't of met so many people like that. Got to learn a bunch of other things as well during the process.

Between you, @joelpittet and @laurii I got to really expand and play with other areas I have not yet explored. I can't take credit for all of it either, just wrote up the summary of what we collectively found out.

So thanks!. :D

:(

In case someone wants to work on the issue but not know how to fix the problem, first try to write a test for it.

First step: Add an entry to \Drupal\system\Tests\Theme\TwigTransTest::checkForDebugMarkup which looks like the issue described in the issue queue.
Maybe choose debug() as it should be easier to test.

Just an idea how to fix it in a different way: Write an object which captures which values are passed into t() / formatPlural (this should implement \Drupal\Core\StringTranslation\TranslationInterface and use that as part of the debug statement to know which values got passed / returned.

I discussed this with @effulgentsia, @catch and @xjm. Is this actually exploitable by a site builder - or someone who writes twig templates? I.e. how exploitable is this and is it exploitable through the front-end. I guess maybe through the twig templating abilities in views?

As far as I understand you need to either have access to the twig templates or use the views UI and maybe contemplate or similar modules in the future.
At least for views, we need to be aware that this is a restricted permission already.

Might be also worth mentioning that Twig debug is off by default and enabling it requires altering services.yml (and rebuilding the container). It's also documented as not recommended for production right above the setting:

    # Not recommended in production environments
    # @default false
    debug: true

And on that note…

Twig debug is off by default

I don't think that alone should demote this issue's priority, because arbitrary PHP execution on a dev environment or local machine is still pretty nasty.

you need to either have access to the twig templates

If this were the only pathway, then I'd recommend demoting to Major.

or use the views UI ... this is a restricted permission already.

D8 core no longer has the PHP module, so I think arbitrary PHP execution needs to be closed to all UI users, even those with a restricted permission. Therefore, I think this should remain critical if it's possible to exploit via Views UI.

and maybe contemplate or similar modules

If the Views UI pathway doesn't exist, but such contrib pathways do, then I'm conflicted on whether this should be Critical or not.

Based on the discussion on the security meeting today, the fastest way forward is to temporarily remove the debug output and add it back in a normal task. Opened #2512672: Add secure debug output to twig trans extension for that. Now needs reviews. No interdiff, because this is a trivial new patch.

Yeah +1 to the idea. RTBC if green.

I think we should still have test coverage that this does not happen after removing that piece of code.

@laurii: wanna take that on then?

I can try to find a way to figure out how to test that. I will also take a look on failing tests

There's no way to test that this doesn't happen with twig debug because the functionality is removed in the patch. However the original report didn't know whether its with Twig debug on or not so this tests it without Twig debug now. I did also fix the failing tests.

Thank you gabor!

There's no failing test because the bug doesn't exist. There is still no test coverage for that AFAIK so now there is :)

There's no failing test because the bug doesn't exist. There is still no test coverage for that AFAIK so now there is :)

alexpott told me how to do this. Will be working on this

Never write something is impossible to d.o because someone will prove you its possible :)

Committed 2ac614c and pushed to 8.0.x. Thanks!