Autoescaping doesen't work in <script> tag context

[Hurtak]

Hello,

while trying out Marko I tested how it's autoescaping works in different contexts.

The contexts I tested it in:

• inside html tag
• inside html attribute
• inside html script tag

In first two Marko correctly detected context and chose proper escaping functions (I saw escapeXml and escapeXmlAttr in compiled templates), but in the last one I was able to do XSS.

You can replicate it on http://markojs.com/try-online/

marko template

<p onclick='alert("${ data }")'>${ data }</p>
<script>var x = "${ data }"</script>

data

"\"; alert('xss');\""

HTML output

<p onclick="alert(&quot;&quot;; alert(&#39;xss&#39;);&quot;&quot;)">"; alert('xss');"</p>
<script>
    var x = ""; alert('xss');""
</script>

[patrick-steele-idem]

@Hurtak this isn't actually a problem Marko can solve since putting dynamic values inside a <script> block is inherently unsafe. Wherever you are allowing user data to be inserted into a script block you should be using JSON.stringify() to ensure that it is only data and not code:

<script>var x = $!{JSON.stringify(data)}</script>

In this case, HTML escaping is disabled because encoded HTML entities are not decoded if they are inside <script> tags.

However, if the stringified data contains an ending </script> tag then that will prematurely end the <script> block so you need to do the following:

<script>var x = $!{JSON.stringify(data).replace(/<\//g, '<\\u002F')}</script>

With that said, using <script> tags with dynamic code is still very dangerous. It's best to put user data into DOM attributes so that it is guaranteed not be executed. For example:

<div data-foo=JSON.stringify(data)>

The data can then be extracted from the DOM using code similar to the following:

var foo = JSON.parse(el.getAttribute('data-foo'));

I'm going to close this issue because it doesn't require any changes to Marko, but we should probably put together a more complete security guide.

[patrick-steele-idem]

@Hurtak also, if you have any thoughts or suggestions on how we might be able to avoid the security problems associated with <script> tags please share. If we can provide a helper of some sort then that might not be a bad idea.

[Hurtak]

@patrick-steele-idem thanks for the detailed reply,

Is it really inherently unsafe to put user data in script tag? Even if you escape all characters like > into unicode? This seems to be what some template engines are doing.

Some provide escapejs filter - https://docs.djangoproject.com/en/1.9/ref/templates/builtins/#escapejs
Some even do this automatically by detecting context where variable is used https://latte.nette.org/en/#toc-context-aware-escaping

But you seem to be much more knowledgeable about this topic than me, so I am not sure if these filters catch 100% of XSS attackers inputs

[patrick-steele-idem]

@Hurtak The escapejs filter that django provides is a little odd since it assumes that the character being escaped inside a JavaScript string, but after giving it some thought that's probably an okay assumption. I think I'm coming around to escaping special HTML characters using the JavaScript string escape sequence in the context of a <script> block. It's definitely better than the current code that escapes special HTML characters inside <script> blocks using HTML entities which is absolutely not what we want.

For example, given the following template:

<script>
    var foo = ${JSON.stringify(data.foo)};
</script>

And the given data:

{
    foo: {
        name: 'Evil </script>'
    }
}

The output is currently the following:

<script>
    var foo = {"name":"Evil &lt;/script>"};
</script>

(browsers do not decode HTML entities inside <script> blocks... <script> tags are special)

The output should be the following:

<script>
    var foo = {"name":"Evil \u003C/script>"};
</script>

I'm going to reopen this issue and investigate a proper fix. Thanks for opening this issue and thank you for your persistence :)

[patrick-steele-idem]

New version published: marko@3.7.1

See CHANGELOG.MD for a longer description of what exactly changed:

[yomed]

@patrick-steele-idem With this fix in place, is there still any reason to avoid putting data in inline scripts? The strategy of using empty dom elements with data-attributes always seemed strange to me.

[patrick-steele-idem]

@yomed

With this fix in place, is there still any reason to avoid putting data in inline scripts?

This was done in preparation for support CSP. However, as long as we put a CSP nonce attribute on the inline <script> tag then we are still able to use inline <script> tags. In a future version of Marko Widgets we plan on switching to using <script> tags to pass data down to the browser for performance reasons. So to answer your question, no, there is no good reason to avoid putting data in inline script tags.