Prevent command injection through the variable option

[stof]

This fixes the issue disclosed at https://app.snyk.io/vuln/SNYK-JS-LODASH-1040724

For now, I used a regexp allowing any ES5 identifier for the variable name. The advantage of that is that it avoids BC breaks if some code uses weird variable names. The drawback is that the regexp is big.
If reducing the range of allowed variable names is OK, a smaller regex could be used.

[stof]

Looks like there is no CI running on the 4.17 branch anymore as the migration to github actions has been done only in master...

[Alan-Liang]

The regex is of ~10k in size, and is still incomplete. Example:

> var 𝐀 = 1
undefined
> re.test('𝐀')
false

[stof]

@Alan-Liang I indeed took the ES 5 regex, not the ES6 one (which allowed even more chars.

I'll let the maintainers decide what they want to allow for this variable option.

[Alan-Liang]

Would it be OK to just ban the following characters: ), = (default parameter), , (delimiter)? I think an attacker cannot execute any code without using one of these, and a normal bad identifier will cause a SyntaxError to be thrown once being eval()ed.

Is there any current ES proposal that extends this list of characters?

[bnjmnt4n]

What do you think about only accepting ASCII characters for the variable name, and instead of throwing an error, just defaulting to the with(obj){ } branch?

Any change here to fix this CVE would be considered breaking, but I don't want to introduce additional explicit errors.

/cc @jdalton @falsyvalues for any inputs.

[stof]

@bnjmnt4n defaulting to with (obj) would still produce a broken compiled template, as using variable or not using it leads to writing the source of the template differently.

[bnjmnt4n]

Yeah, the generated template function would be broken, but it would be a runtime error instead of an error during _.template. However, now that I think about it, it's probably fine to throw an error at "compile time" since there's a likelihood that our Function call would do that also, so keeping the error throwing behaviour should be fine.

[stof]

And throwing a compile-time error at least allows to throw a meaningful error, explaining what went wrong.
Also, projects not allowing a user-defined variable name are probably using a working one.

[stof]

I updated the PR to use a much smaller regex forbidding some offending chars instead of the huge ES identifier regex

[falsyvalues]

Main problem is that we don't know what users put there, its configuration layer. Honestly I'm not really sure why this is even reported as Command Injection since its configuration layer.

@bnjmnt4n I agree we should avoid throwing new errors and go with default path. Maybe it would be easier to use negative check and detect characters that cannot be part of variable name in ES6.

[stof]

@falsyvalues lodash cannot enforce that no user-controlled values is ever used by projects when passing the variable.

I agree we should avoid throwing new errors and go with default path.

having a variable name or not having one produce a different template compilation, for a different kind of usage. You cannot consider the with (obj) to be the default case of a variable name. that's a different kind of template compilation. A dedicated error is better than weird errors due to accessing undefined variables inside the generated code during the template execution.

Maybe it would be easier to use negative check and detect characters that cannot be part of variable name in ES6.

This has now been done (with a set of specific forbidden chars) to avoid the huge regexp.
Also, this should reduce the BC impact, as the only projects that would get the new error would be project that currently inject broken variables (and so are either subject to the reported security issue with carefully crafted config or are generating broken code)

[falsyvalues]

@falsyvalues lodash cannot enforce that no user-controlled values is ever used by projects when passing the variable.

Sorry, but I don't know what are you trying to say. To which part of comment are you referring?

[stof]

@falsyvalues that was the answer to the first paragraph of your previous comment.
The fact that lodash does not know what gets passed there is precisely the reason why it should not trust the value to be valid.

[bnjmnt4n]

I think the list of forbidden characters used is a good compromise between having to embed a large ES identifier regex, and maximizing back-compat. This does mean that we might have to update the regex in the future if there are any changes to allowed ECMAScript syntax.

[bnjmnt4n]

Thanks for the PR, merged in 3469357.

[lukaw3d]

I noticed there is no check that variable is typeof 'string'

[Alan-Liang]

RegExp.prototype.test coerces its argument to a string before making matches.