Potential XSS vulnerability

[Chris98]

I've found a potential XSS vulnerability with the extension..... this has nothing to do with the changes we made to the codebase several months back as it was there prior, but unfortunately it seems that anything that is put between the {% trans %}{% endtrans %} tags is NOT autoescaped.

This basically means that if your GetText language strings include HTML (which probably many do) there is no way to autoescape this by default or at all within these tags... which pretty much goes against everything that Twig stands for IMO.

So if a user is not careful and carelessly adds in user data into a langauge string - which in theory is about 99% going to happen - if it's not sanitized properly prior this leaves open the possibility of XSS attack.

My recommendation to fix this would be to change the trans tag so it automatically escapes all content, and if for some reason HTML needs to be added into a language string then a "raw" filter of some kind can be added to override this setting. And also allow the possibility of escaping or using the raw filter on variables used within the trans tag would also be extremely useful.

I'm going to look into this over the next few days for my own implementation, and if I get anywhere, I will let you know.

[williamdes]

What do you think about this @mauriciofauth ?

[Chris98]

I've taken a deep look into this and I don't believe it's something that is going to be easily fixed. It's certainly beyond my ability to fix, I've been at this for a week straight now and I'm just no further forward. The only successful implementation of something similar I can find of using filters in a trans tag to escape is in Drupal 8, but this is using the 1.x branch of Twig and even when I try to piggy-back some of their code and adapt it, there still seems to be something missing because it's not working as intended.

I don't believe that the Twig version or branch here is causing the difference, it can pick up the fact there is a filter, in fact Twig is already doing this with a little bit of change to the checkTransString method. But for some reason the filters are just not being applied and it doesn't want to autoescape anything.

When you do change to subcompile the variables then it's echo'ing them out into the template when you want them as function arguments. I tried this with a new trans tag completely from scratch with no other code, and the only successful implementation of getting the echo'd variables into the function call as argument I can see is to use output buffering as denoted here: https://stackoverflow.com/a/26215387/3557031

This for me is something that's too messy and doesn't really work because (i) I'm already using output buffering, and (ii) it's very messy especially if you use a lot of strings and it's adding a lot of otherwise unnecessary function calls into the templates on every page load.

Without using this, I seem to be able to get it to either call a filter to escape the variable (with no variable actually present) or the current setup, but there doesn't seem to be any way to achieve both.

As painful as it is, for my implementation for now I'm just going to go through all my templates and escape the variables before they're outputted in the trans tag. I see no other plausible way for now that actually functions efficiently.

[williamdes]

Thank you Chris for the research ! I think we can say that if po/mo files are controlled there is no security issue, right ?

[Chris98]

It depends really on whether there is any arbitrary HTML put into the trans tag in the template..... it is possible for a template designer for instance to accidently add in an XSS vulnerability in this way. For instance:

{% set disclaimer = '<a href="' ~ base['links']['site'] ~ 'disclaimer/">' %}
{% set privacy = '<a href="' ~ base['links']['site'] ~ 'privacy/">' %}
{% set enda = '</a>' %}

<div class="terms-conditions">{% trans from "core" %}{{ disclaimer }}Terms and conditions{{ enda }} - {{ privacy }}Privacy Policy{{ enda }}{% endtrans %}</div>

This would render as HTML and it's the same for any of the variables outputted in the trans tag. If you control the variables and you're 100% sure they're ALL escaped (or deliberately avoided) before they get to the trans tag, then it's safe.

[Chris98]

Ok: GREAT news. I've managed to get a working version of this. It's not perfect and for my own purposes I had to completely rewrite the plugin, so I'll only post the relevant bits below for you.

In TransNode.php, you want to modify part of the compile method to something like this:

	/* Do we have any variables? */
	if ($args)
	{
		$compiler->raw(', array(');

            foreach ($args as $arg)
			{
				if ($arg->getAttribute('name') === 'count')
				{
					$compiler
                        ->string('%count%')
                        ->raw(' => abs(')
                        ->subcompile($this->hasNode('count') ? $this->getNode('count') : null)
						->raw('), ');
                }
				else
				{
					$compiler
                        ->string('%'.$arg->getAttribute('name').'%');

					if ($arg->hasAttribute('raw'))
					{
						$compiler
							->raw(' => ')
							->subcompile($arg)
							->raw(', ');
					}
					else
					{
						$compiler
							->raw(' => twig_escape_filter($this->env, ')
							->subcompile($arg)
							->raw('), ');
					}
                }
            }

Next, I modified part of the compileString method to be like this to set the raw attribute:

if (count($translation))
		{
			$msg = '';

			foreach ($translation as $node)
			{
				if ($node instanceof \Twig\Node\PrintNode)
				{
					$n = $node->getNode('expr');
					if (!$n instanceof \Twig\Node\Expression\FilterExpression)
					{
						$isRaw = true;
					}
					else
					{
						$isRaw = false;
					}

					while ($n instanceof \Twig\Node\Expression\FilterExpression)
					{
						$n = $n->getNode('node');
					}

					while ($n instanceof \Twig\Node\CheckToStringNode)
					{
						$n = $n->getNode('expr');
					}

                    $msg .= sprintf('%%%s%%', $n->getAttribute('name'));

					$expr = new \Twig\Node\Expression\NameExpression($n->getAttribute('name'), $n->getTemplateLine());
					if ($isRaw)
					{
						$expr->setAttribute('raw', true);
					}

                    $vars[] = $expr;
                }
				else
				{
					$msg .= $node->getAttribute('data');
                }
            }
        }
		else
		{
			$msg = $translation->getAttribute('data');
        }

The only other thing left to do is add in the ability to allow functions in the Token Parser checkTrans ... etc function. So much for being out of my ability then (ha!)

I hope this helps!

[Chris98]

A note though that this will add the raw tag on for ANY filter, not just when you add "|raw". For some reason it still can't pick up filters so use with caution....

[williamdes]

Thank you for all your work @Chris98 ! Would you mind opening a PR for it ?

[Chris98]

Sorry I've just seen this. Do you still need a PR?

[Chris98]

Just realised I uploaded my solution to Github here: https://github.com/Chris98/TwigI18nExtension

I'm afraid I don't have the time to do a proper PR, but my full solution is here which you can use, if you still need a one. Hope that helps.

[williamdes]

Just realised I uploaded my solution to Github here: https://github.com/Chris98/TwigI18nExtension

I'm afraid I don't have the time to do a proper PR, but my full solution is here which you can use, if you still need a one. Hope that helps.

Can you make it public please ?

[williamdes]

Hi @Chris98 This was my last item on a very long TODO list. I can not merge nor understand your changes, could you fork this repository and makes the changes on top of our history ? You repository seems to be very different from ours

[williamdes]

I added this test, you are right using tags the contents are not escaped: 26fac73178882cd796e2385750a383e67768da11

[MauricioFauth]

I think we should deprecated the usage of tags, introduce a trans function, and use only the function and the filter. The tag does not respect the auto-escaping because tags are not supposed to have outputs.

[williamdes]

I think we should deprecated the usage of tags, introduce a trans function, and use only the function and the filter. The tag does not respect the auto-escaping because tags are not supposed to have outputs.

I agree

[Chris98]

I think we should deprecated the usage of tags, introduce a trans function, and use only the function and the filter. The tag does not respect the auto-escaping because tags are not supposed to have outputs.

I suppose this really depends from your point of view how important this functionality is, and how much it is used in PhpMyAdmin. I haven't reviewed the full codebase so can't really comment on that. However in my opinion broadly speaking, I would say this is a mistake because there are some use cases which the tag can handle, which e.g filters cannot - one being plurals. The tag also has the greatest flexibility going forwards for any future changes you might make or other properties you might want to include. In my application I use this to load multiple domains and provide a great deal of organisation for localisation.

The actual XSS here is easy enough to fix, offhand I can't remember whether this is only affecting the trans tag or the functions and filters as well - I think it might be come to think of it. Again I can't remember as it's been a while since I looked at this, but I think the filter only handles a single argument as well, such as a domain. In that case if you wanted to for instance, add an argument that one specific string should NOT be autoescaped, or some other argument in future, it could cause problems.

Also re the tag not producing output that isn't strictly true as there are examples where this is the case in Twig itself, one being the autoescape tag: https://twig.symfony.com/doc/3.x/tags/autoescape.html

Unfortunately my main development machine is down at the moment so I'm not currently in a position to commit any code. However I will see what I can do at some point about stripping out the parts of my code which fix this and putting them here for you - although most are explained above.

[Chris98]

To fix it, you want to change the checkTransString function in TransTokenParser to something like this:

    protected function checkTransString(\Twig\Node\Node $body, int $lineno)
    {
        foreach ($body as $i => $node)
		{
			if ($node instanceof \Twig\Node\TextNode || ($node instanceof \Twig\Node\PrintNode && $node->getNode('expr') instanceof \Twig\Node\Expression\NameExpression) || $node instanceof \Twig\Node\PrintNode && $node->getNode('expr') instanceof \Twig\Node\Expression\FilterExpression)
			{
				continue;
			}

			throw new \Twig\Error\SyntaxError('The text to be translated with "trans" can only contain references to simple variables', $lineno);
        }
    }

The rest is taken care of inside TransNode with the code from this post above: https://github.com/phpmyadmin/twig-i18n-extension/issues/9#issuecomment-881004043

Note that if you're building some extremely complex data structure, it may be possible to get around this. I haven't tested that so I'm not sure whether there may be some very obscure case where this breaks. However this would only be possible if this was created on purpose by the template developer. I've been using this myself for 2 years now and in my application it works just fine on every case I need.

Good luck and I hope this helps.

[MauricioFauth]

I'm closing this issue as this extension is on maintenance mode only and it was completely removed from phpMyAdmin's master branch.