Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning against running untrusted templates #17

Closed
vjeux opened this issue Oct 13, 2014 · 16 comments
Closed

Add warning against running untrusted templates #17

vjeux opened this issue Oct 13, 2014 · 16 comments

Comments

@vjeux
Copy link

@vjeux vjeux commented Oct 13, 2014

I talked to people that are under the impression that it's safe to feed user-defined templates to nunjucks. However it is not. It may be good to add a warning about this.

Proof of concept to run arbitrary code on viewers: http://jsfiddle.net/vjeux/q55ads7r/
Proof of concept to run arbitrary code on the execution environment: http://jsfiddle.net/vjeux/2kcjjgt2/

@jlongster
Copy link
Member

@jlongster jlongster commented Oct 13, 2014

Good point! I wonder if other templating autoescaping escapes = and quotes, which would fix this. I will definitely add a warning if nothing changes, but this feels like something that should be handled by autoescaping.

@vjeux
Copy link
Author

@vjeux vjeux commented Oct 13, 2014

Auto-escaping works on the variables you feed into the template, not in the template itself. (and correctly fixes the problem: http://jsfiddle.net/vjeux/3rzsn4sy/ ).

What I'm trying to point here is that it's not safe to have the user define the template itself.

@jlongster
Copy link
Member

@jlongster jlongster commented Oct 13, 2014

Oh, duh, right. I misinterpreted. Yeah, of course by default it will output HTML. I will add a warning to the docs.

@vjeux
Copy link
Author

@vjeux vjeux commented Oct 13, 2014

I think the fact that rendering a template can execute arbitrary javascript code on the server is even more unexpected

nunjucks.renderString('{{a.constructor.__proto__.constructor("alert()")()}}');
@jlongster
Copy link
Member

@jlongster jlongster commented Oct 13, 2014

We parse a grammar similar to JS, and output the corresponding JS to the AST, so yes, those holes are exposed. Accessing native methods is an easy way to get a lot for free, so users can just do {{ arr.length }} and stuff. You're right though; we should blacklist a few properties that allow you to access this so that this safety is on by default.

I know other engines have a "sandboxed" mode which we could also support. Is there anything other than constructor that you can think of that we need to blacklist?

Regardless, I will add a warning that we don't guarantee a full sandboxed mode yet.

@vjeux
Copy link
Author

@vjeux vjeux commented Oct 13, 2014

Blacklisting properties is very dangerous. If you ever attempt to have a sandboxed mode, I would highly recommend you talk to someone with a web security background. Cure53 audited most of the template engines out there. https://code.google.com/p/mustache-security/ They also privately audited React and had a lot of good suggestions to improve it's security edge cases :)

@jlongster
Copy link
Member

@jlongster jlongster commented Oct 13, 2014

Yeah, blackboxing is just a false sense of security. Nunjucks does not really provide a solution for user-defined templates that should not be able to run arbitrary JS. Some of the current products that use nunjucks are CMS-es where this doesn't matter, because you are using it to customize your own site and you could freely run JS in just a <script> tag.

Handlebars is a better solution for a completely safe user-defined template. I will add notes about this in the docs somewhere.

@jlongster
Copy link
Member

@jlongster jlongster commented Apr 3, 2015

I added a warning in the updated docs (published soon, thanks!)

@jlongster jlongster closed this Apr 3, 2015
@brianmhunt
Copy link

@brianmhunt brianmhunt commented Jun 8, 2016

For those reading this, there are two bits of info that may be helpful:

  1. On the client-side, you can use a Content Security Policy to limit the sources of scripts i.e. it can prohibit executing scripts from arbitrary urls. You cannot compile nunjucks scripts in the browser when the CSP uses unsafe-eval (which would be the "gold standard" of protections, and what is needed for some applications like Chrome Apps) at this time, per mozilla/nunjucks#298.
  2. You can use a Javascript-like language, such as the one I wrote for knockout-secure-binding, which is CSP-safe (though arguably it simply usurps the protections CSP provides – know your vectors! 😄 ).
@nebrelbug
Copy link

@nebrelbug nebrelbug commented Jul 6, 2019

Hi @vjeux, what exactly is this supposed to do, and how does it work?

nunjucks.renderString('{{a.constructor.__proto__.constructor("alert()")()}}');

I checked out the JSFiddle and it doesn't do anything. I'm guessing it's because I'm on a newer version of Chrome?

@vjeux
Copy link
Author

@vjeux vjeux commented Jul 8, 2019

The variable a used to be defined by an earlier version of nunjucks (the jsfiddle include path is not versioned). I replaced a with ({}) and it reproduces again: http://jsfiddle.net/vjeux/p0gz9j6k/

@nebrelbug
Copy link

@nebrelbug nebrelbug commented Jul 8, 2019

Thanks @vjeux!

@vjeux
Copy link
Author

@vjeux vjeux commented Jul 8, 2019

In JavaScript, you can define a function statically:

function() { alert() }

or dynamically from a string

Function("alert()")

I'm using alert() to show that we can run arbitrary code, but you can put anything in there, including reading cookies, sending http requests...

Once you have a function created like this, you can call it

Function("alert()")()

in order to execute it. So now, the game is all about figuring out a way to access the Function object. nunjucks won't let you use a global variable this way, so you need to be more creative.

If you have access to any function, you can get Function by accessing the .constructor field on it.

(function() {}).constructor === Function // true

nunjucks won't let you define anonymous functions like that either. If it would, we could just have done (function() { alert() })() and be done.

What we can do is to create a new object and ask for its constructor, which is a function. We don't really care what that function does, the important thing is that it's a function.

({}).constructor // function

from there, we can access .constructor to get the Function function.

({}).constructor.constructor === Function // true

At this point we won, now we just need to assemble everything back together

({}).constructor.constructor("alert()")()

and we're done.

In the example above I added a .__proto__ level of indirection that's not needed. I just know that __proto__ has good properties for this kind of things so I tend to go there first when writing this kind of exploits.

@nebrelbug
Copy link

@nebrelbug nebrelbug commented Jul 8, 2019

Thanks so much @vjeux!

I'm re-writing my template engine, Squirrelly, and wondering if I should implement some of these protections like preventing the user from accessing global variables, etc.

Currently, I'm thinking that I'll just have Squirrelly as a tool to output untrusted JavaScript, and a tool which shouldn't be used with untrusted data or templates. In your opinion, does this sound like a bad idea?

@vjeux
Copy link
Author

@vjeux vjeux commented Jul 8, 2019

You should likely treat the data that you pass as untrusted: if someone sets his username as <script>alert();</script>, you don't want to it to running arbitrary JavaScript.

For templates, if they are part of a build system and only requiring sources checked in the repo. Then this is trusted and fine. But for a lot of template systems, it's just a function call with a string. So it's --very easy-- to have someone that work in your codebase pass a dynamic string that is built with user-provided information. Then you're screwed if you don't have a proper sandbox, which all the templating engines that were written pre-react that I know about didn't have.

If you go that route, be aware that doing proper protection is --very hard--. You should look at projects like https://developers.google.com/caja/ instead of trying to fix it yourself.

@epicfaace
Copy link

@epicfaace epicfaace commented Aug 13, 2019

Can't you just use something like DOMPurify as a wrapper to fix this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.