Note: This issue is not critical as it only appears in debug output that should never be active on production and is only possible when the following conditions hold:

- debug mode is on
- dynamic user input of templates is used (This is however active in core right now)
- sandbox mode is not used to restrict possible functions, but even then %trans would probably be allowed.

However, scenario:

- Dump production DB for local development
- Activate debug mode (or have it already active via settings.local.php)
- Get "owned" on your own machine ...

The same could happen on a -dev server or even staging environment.

Problem/Motivation

Given the following twig template:

{% trans %} Hi - ' . die('I am bad code!') . ' {% endtrans %}

the code is executed due to the debug output not being properly escaped for "'".

Proposed resolution

Remove debug output for now. Add it back in #2512672: Add secure debug output to twig trans extension.

Remaining tasks

Review.

User interface changes

- None

API changes

- None

Files: 
CommentFileSizeAuthor
#38 interdiff.txt2.7 KBlauriii
#38 arbitrary_code-2489024-38.patch6.5 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,219 pass(es). View
#38 arbitrary_code-2489024-test-only-38.patch5.45 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,203 pass(es), 1 fail(s), and 0 exception(s). View
#33 interdiff.txt667 byteslauriii
#33 arbitrary_code-2489024-33.patch5.22 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,242 pass(es). View
#31 interdiff.txt3.84 KBlauriii
#31 arbitrary_code-2489024-31.patch5.22 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,261 pass(es). View
#25 2489024-25.patch1.05 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,312 pass(es), 11 fail(s), and 0 exception(s). View
#3 arbitrary_code-2489024-3.patch982 bytesFabianx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,016 pass(es), 11 fail(s), and 0 exception(s). View

Comments

Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

Issue summary: View changes

Debug output is correct.

Fabianx’s picture

Status: Active » Needs review
FileSize
982 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,016 pass(es), 11 fail(s), and 0 exception(s). View

And here is a patch to fix it :).

Fabianx’s picture

Priority: Normal » Major

Status: Needs review » Needs work

The last submitted patch, 3: arbitrary_code-2489024-3.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: arbitrary_code-2489024-3.patch, failed testing.

lauriii’s picture

Issue tags: +Needs tests
Cottser’s picture

Assigned: Fabianx » Unassigned
Issue tags: +Novice
+++ 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 :)

zeropx’s picture

Starting on this at DrupalCon LA Sprints

zeropx’s picture

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.

Cottser’s picture

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

zeropx’s picture

@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

Fabianx’s picture

Title: Arbitrary code execution via 'trans' extension for dynamic twig templates » Arbitrary code execution via 'trans' extension for dynamic twig templates (when debug output is on)
Assigned: Unassigned » Fabianx
Fabianx’s picture

Assigned: Fabianx » Unassigned
Priority: Major » Critical
Issue summary: View changes
Issue tags: -Novice

Actually this is critical :(.

Scenario:

- Dump production DB for local development
- Activate debug mode (or have it already active via settings.local.php)
- Get "owned" on your own machine ...

The same could happen on a -dev server or even staging environment.

Fabianx’s picture

Issue tags: +security
Cottser’s picture

Issue tags: +Twig

:(

dawehner’s picture

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.

dawehner’s picture

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.

alexpott’s picture

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?

dawehner’s picture

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.

Cottser’s picture

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
Cottser’s picture

And on that note…

effulgentsia’s picture

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.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,312 pass(es), 11 fail(s), and 0 exception(s). View

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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Yeah +1 to the idea. RTBC if green.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

Gábor Hojtsy’s picture

@laurii: wanna take that on then?

The last submitted patch, 25: 2489024-25.patch, failed testing.

lauriii’s picture

Assigned: Unassigned » lauriii

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

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,261 pass(es). View
3.84 KB

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.

dawehner’s picture

Thank you gabor!

lauriii’s picture

FileSize
5.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,242 pass(es). View
667 bytes
Fabianx’s picture

Can we get a test-only patch, too, please?

lauriii’s picture

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

lauriii’s picture

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

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Needs review » Needs work

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

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
5.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,203 pass(es), 1 fail(s), and 0 exception(s). View
6.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,219 pass(es). View
2.7 KB

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

The last submitted patch, 38: arbitrary_code-2489024-test-only-38.patch, failed testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, fantastic work!

Thanks, laurii.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 2ac614c on 8.0.x
    Issue #2489024 by lauriii, Fabianx, Gábor Hojtsy, Cottser, dawehner,...
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-ui

Status: Fixed » Closed (fixed)

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