Only allow access to view contexts own properties

[sjwalker189]

Templates are not sandboxed and should not be trusted when compiling templates from users, for example, in a CMS. This is very problematic and exposes XXS attacks into a node environment.

I have created an example here and compared it with twig which does sandbox the template context properly. https://codesandbox.io/s/bold-hofstadter-1p03t?file=/src/index.js

[harttle]

Since liquidjs is expected to behave the same way as Shopify/liquid, it's by design not to escape values (i.e. {{ foo }}) by default. And there're escape and escape_once filters for doing so.

It means the author of templates is responsible for escaping values wherever it's untrusted. On the otherway, it's possible to provide a LiquidJS option to enable default escaping. I'll keep this issue open for more discussions.

[sjwalker189]

Thanks for keeping the ticket open.

The issue is more around sandboxing rather than escaping values. For example, escaping a value using the escape filter does not prevent access to the values constructor because of how the JavaScript language works (every data type has a function constructor). You would still be vulnerable to XSS with an escaped value. ({{ "".constructor }} would still work)

Another library that implements this style of sandboxing is https://github.com/NightlyCommit/twing

[Prinzhorn]

I think you are misunderstanding each other. What @samuelthompson189 is talking about is RCE (remote code execution), not XSS (unless you are using liquid in the browser). You can access the Function constructor via {{ __proto__.constructor.constructor }} but I haven't found a way to execute it (which would lead to RCE on the server, not XSS on the client, which escape is for). It could also lead to information disclosure if you can somehow access globals like process.

I'm not sure what @samuelthompson189 means with sandboxing. What I think liquidjs needs to do is only allow access to own properties

https://github.com/harttle/liquidjs/search?q=hasOwnProperty&type=code vs https://github.com/twigjs/twig.js/search?q=hasOwnProperty&type=code

Edit: but I'm not sure if that's actually how twig does it twigjs/twig.js#32 twigjs/twig.js#204

[sjwalker189]

@Prinzhorn Yes, thank you. You are absolutely correct.

My terminology could have been better. Sorry about that!

[Prinzhorn]

@samuelthompson189 I think your title is still a little sensational. So far there is no RCE. All we can do is access properties that arguably shouldn't be accessible.

I also think GitHub issues are not the place to report security vulnerabilities. In the future you might want to check if there is a security policy. If not, try to contact the maintainers through other channels. You can also report issues with modules via https://hackerone.com/nodejs-ecosystem?type=team (your report in the current form would probably be rejected because your example doesn't show any vulnerability)

@harttle maybe you can add a security policy to this repo so people know who and how to contact

[sjwalker189]

@Prinzhorn Thanks, I will take your advice next time and use the channel's linked above. 🙌

[harttle]

Thank you @Prinzhorn, I've added a security policy for subsequent reports.

So it seems the question is: "is reading inherited properties safe?" As mentioned in twigjs/twig.js#32, it's useful to allow inheritance and twig did make it happen via twigjs/twig.js@2ca6349.

The properties of scope objects is intended to be read arbitrarily. And it's not executed, just read. If I understand correctly, it'll be a vulnerability only when something like process or LiquidJS internals can be read and executed. And if that's the case, I need a specific use case.