translate-params-* allows usage of HTML content under certain conditions (potential security issue)

[anx-ckreuzberger]

The Issue

Assuming this is your controller:

myApp.controller('TestController', function ($scope) {
    $scope.one = 1;
    $scope.someUnsafeHtml = "<b>{{ one }} + {{ one }} = {{ one + one }}</b>";
});

And this is your HTML

<div translate>
    {{ someUnsafeHtml }}
</div>

AngularJS and/or angular-gettext will make sure, that someUnsafeHtml is properly escaped. So far, so good.

However, as soon as you use the translate-params-some-unsafe-html="someUnsafeHtml" attribute, someUnsafeHtml is no longer escaped.

<div translate translate-params-some-unsafe-html="someUnsafeHtml">
    {{ someUnsafeHtml }}
</div>

Interesting enough, the string also seems to be not escaped, when using a different attribute name and empty value (translate-params-nothing):

<div translate translate-params-nothing>
    {{ someUnsafeHtml }}
</div>

I believe that this behaviour is not the intended behaviour, as this means that there is a potential security issue as soon as one uses the translate-params- attributes is used with user generated content (of course, one should always sanitize input variables before adding them to the database).

I have created a jsfiddle with Angular 1.5.10 and angular-gettext 2.3.10 that displays some other cases aswell: https://jsfiddle.net/wy0fu3hd/9/

I believe that it should be possible to render HTML, if and only if you specifically mark the content as trusted HTML content using $sce

myApp.controller('TestController', function ($scope) {
    $scope.one = 1;
    $scope.someHtml = $sce.trustAsHtml("<b>{{ one }} + {{ one }} = {{ one + one }}</b>");
});

and specially use the translate-params- attribute

<div translate translate-params-some-html="someHtml">
    {{ someHtml }}
</div>

Though it might make sense that this is only supported using some additional attribute, e.g.:

<div translate translate-htmlparams-some-html="someHtml">
    {{ someHtml }}
</div>

Also paging @IShotTheSheriff on this, as the original concept is from him (issue #285).

Analysis

This is happening because of the getString method in gettextCatalog (see https://github.com/rubenv/angular-gettext/blob/master/src/catalog.js#L243-L249):

        getString: function (string, scope, context) {
            var fallbackLanguage = gettextFallbackLanguage(this.currentLanguage);
            string = this.getStringFormFor(this.currentLanguage, string, 1, context) ||
                     this.getStringFormFor(fallbackLanguage, string, 1, context) ||
                     prefixDebug(string);
            string = scope ? $interpolate(string)(scope) : string;
            return addTranslatedMarkers(string);
        },

Especially the part where it says $interpolate(string)(scope).
The $interpolate factory/service is called when a scope is passed to the getString function. This happens when using the translate-params-* attribute, as this defines an interpolationContext:

    function handleInterpolationContext(scope, attrs, update) {
        // ...
        var interpolationContext = angular.extend({}, scope);
        var unwatchers = [];
        attributes.forEach(function (attribute) {
            var unwatch = scope.$watch(attrs[attribute], function (newVal) {
                var key = getCtxAttr(attribute);
                interpolationContext[key] = newVal;
                update(interpolationContext);
            });
            unwatchers.push(unwatch);
        });
        // ...
    }

Here var interpolationContext = angular.extend({}, scope); creates a new scope, which is then getting the values of the passed parameters of translate-params-*: interpolationContext[key] = newVal;
Then the update(interpolationContext) method is called, which calls the getString() method from above with the interpolationContext as scope.

By calling $interpolate here, we will end up with an interpolated string, which is eventually set as the main content of the element in the update() method:

                    function update(interpolationContext) {
                        interpolationContext = interpolationContext || null;
                        // ...
                       translated = gettextCatalog.getString(msgid, interpolationContext, translateContext);
                        // ...
                        var newWrapper = angular.element('<span>' + translated + '</span>');
                        $compile(newWrapper.contents())(scope);
                        var newContents = newWrapper.contents();
                        // ...
                      }

Solution

I am not sure, how to solve this problem yet. Surely it has something to do with the combination of $interpolate and $compile aswell as the concatenation '<span>' + translated + '</span>'.
I am however sure, that it should be possible to inject HTML, if specified by the developer.

[anx-ckreuzberger]

FYI, here is a fiddle demonstrating the attack using an injected JavaScript alert():
https://jsfiddle.net/rodcwqdb/3/

Again: obviously the javascript code should have never made it to the database, but it COULD have made it to the database, therefor this poses a security risk!

[anx-ckreuzberger]

As a potential way to fix:

1. Do not use $interpolate in getString - this method is bad and is the root of all evil here!
2. In handleInterpolationContext, Instead of "copying" the scope as a context-object with var interpolationContext = angular.extend({}, scope);, create a new child scope with var interpolationContext = scope.$new(); (ToDo: Check if this causes a memory leak, and if we need to destroy the scope properly)
3. In function update(interpolationContext) {, call $compile with the interpolationContext:

$compile(newWrapper.contents())(interpolationContext || scope);

This fixes the XSS problem in the first place, but I'm not sure if it creates new problems.

[xamino]

Hi – I'm one of the developers who'd like to retain the ability to inject HTML. In our use case we write icons into the string as variables for example, or links. Fixing it via $sce is possible but not very nice – are there plans for a HTML attribute which would allow this?

[anx-ckreuzberger]

Hi!

I actually had the same use-case (and that was the way I found this vulnerability too).

I have added a translate-html-params-* directive into my fork of angular-gettext, which solves this issue in a declarative way (as in: declare that a parameter is actually HTML):

<div translate translate-html-params-my-icon="myIcon">
    I have a nice icon {{ myIcon }}
</div>

$scope.myIcon = "<span class=\"fa fa-some-icon\"></span>";

If people are interested, I can make a Pull Request with documentation and tests into this repo.

[rubenv]

I'm no fan of unneeded features, but this one definitely serves a purpose. Would gladly accept a PR!

[rhuitl]

@anx-ckreuzberger count me in as interested ;-) Do you have a fork with that feature?

[anx-ckreuzberger]

Yes I do.
https://github.com/anx-ckreuzberger/angular-gettext#htmlparams2

my package.json contains this:

    "angular-gettext": "git+https://github.com/anx-ckreuzberger/angular-gettext#htmlparams2",

[rhuitl]

@rubenv do you think there would be more work to be done in order to merge a pull request with this diff?
master...anx-ckreuzberger:htmlparams2

[rubenv]

Hi all, sorry for the late reply. I've been travelling and well... I don't have the needed time anymore.

For that reason, please have a look at #365.