Clarify _.template security

[jgonggrijp]

Once a year or so (on average), someone reports a vulnerability because they noticed that _.template creates a string from user input and passes it to the Function constructor. These reports are well-intended and understandable, but they are nevertheless false reports. Such logic might be considered a vulnerability in another context, but _.template is supposed to do this by design.

This pull request adds two pieces of documentation, with the aim to clarify _.template's contract to security researchers. The aim is not to discourage reports; the misunderstanding does not happen often enough for this to be necessary.

I would like to ask reviewers three questions:

1. If you imagine being a security researcher (or if you are one), do you think that the additions would help you assess whether you found a real vulnerability in _.template? Do you think I could further improve on this?
2. Would you feel discouraged to report a vulnerability after reading the addition to the SECURITY.md? If yes, what should I remove or change in order to stop it from being discouraging?
3. Do you think the amount of text is proportional to the aims? What could be removed or expanded on to make it more proportional?

I welcome reviews from anyone. I will mention the following people because I think/hope they might be interested: @colingm @ByamB4 @GammaGames @Yahasana

[jgonggrijp]

To anyone reading this: please also take a look at #3014.

[GammaGames]

I think it's a good note, it at least gives an place to point when someone tries to submit an issue. If people really care about it they can disable templating with _.templateSettings = null anyway 🤷

[jgonggrijp]

@GammaGames

I think it's a good note, it at least gives an place to point when someone tries to submit an issue.

Thanks for the feedback!

If people really care about it they can disable templating with _.templateSettings = null anyway 🤷

Huh, I don't think that works? Did you mean _.template = null?

[GammaGames]

It broke all my templates when they ran, I got the variable from the docs:

[colingm]

1. Yes I think it is helpful to have this sort of text. I think the problem is just a lot of scans find the use of a function constructor and instantly become reports. Additional context like this helps declare that the function can be a vulnerability in an application if used unsafely is a welcome note.

2. It is very clear in your note to err on the side of caution and report something if you aren’t sure.

3. I think the only addition/change I would consider would be to be explicit that when used on untrusted or unsanitized user input, the usage of template can become a XSS vulnerability in your application.

We have often had reports against us simply because static scans of our code found the use of a function constructor. For that reason alone we actually patch our included lib version of underscore to remove it entirely (since we didn’t need to use it anyways). I imagine that there are many reports that happen for similar reasons and it makes me wonder if we should/could investigate a built version of underscore that doesn’t even include template so that others that don’t use it can avoid frivolous reports against their own app. Here is how we patch it out (so that us and our customers are alerted if someone tries to use it):

const standards = createReplacePatches([
    {
        include: 'libs/underscore.js',
        patterns: [{
            // underscoreFunctionConstructor
            test: /(\s*).*new\s+Function\([^;]*;/g,
            replace: '$1throw new Error("Function constructor not supported");'
        }]
    }
]);

[jgonggrijp]

Thanks @colingm!

3. I think the only addition/change I would consider would be to be explicit that when used on untrusted or unsanitized user input, the usage of template can become a XSS vulnerability in your application.

Alright, I'll add a sentence to emphasize this. It's not just XSS but any form of arbitrary code injection.

We have often had reports against us simply because static scans of our code found the use of a function constructor. For that reason alone we actually patch our included lib version of underscore to remove it entirely (since we didn’t need to use it anyways). I imagine that there are many reports that happen for similar reasons and it makes me wonder if we should/could investigate a built version of underscore that doesn’t even include template so that others that don’t use it can avoid frivolous reports against their own app.

I've been thinking the same. For Underscore 1.x this would be more hassle than I'm willing to go through, but for 2.0 I'm somewhat inclined to just not import it in the index.js. I would still maintain it as a standalone module. Another option, which can exist at the same time, is to add a variant of _.template, say _.trustedTemplate, that embraces the trusted types API.

Here is how we patch it out (so that us and our customers are alerted if someone tries to use it):

const standards = createReplacePatches([
    {
        include: 'libs/underscore.js',
        patterns: [{
            // underscoreFunctionConstructor
            test: /(\s*).*new\s+Function\([^;]*;/g,
            replace: '$1throw new Error("Function constructor not supported");'
        }]
    }
]);

Crude but effective. It's actually possible to build a custom Underscore that cleanly omits _.template, but that is admittedly a bit more code. This will be simpler with Underscore 2.0 as well, where I plan to disable chaining by default in favor of #2809.

[colingm]

It's not just XSS but any form of arbitrary code injection.

True, I just know that is probably one of those buzz words that will make it more immediately recognizable for some individuals. Thank you!

For Underscore 1.x this would be more hassle than I'm willing to go through

Oh definitely not worth it for 1.x and would be problematic for a lot of people I imagine. I have seen elsewhere that you have the goal of making 2.x tree shakeable and that is probably one of the simpler ways to handle it that meets the criteria for it still being easily available if needed.

Crude but effective. It's actually possible to build a custom Underscore that cleanly omits _.template, but that is admittedly a bit more code.

Haha yeah we definitely just took the quick and dirty approach when reacting to security reports. I like your reference for building a custom underscore which we might have to look at though like you said since it will be easier in 2.x we will probably take that approach when we migrate.