bungle / lua-resty-template

Templating Engine (HTML) for Lua and OpenResty.
BSD 3-Clause "New" or "Revised" License
909 stars 204 forks source link

HTML escaping #9

Closed isage closed 9 years ago

isage commented 9 years ago

Why are '/' escaped? And why function results are not?

bungle commented 9 years ago

Hi,

'/' is espaced, because it may help to end html-entity, see Rule 1: https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet

Function(s) that template calls if you do like:

{{func}}

... are not escaped because one use of functions in that way is to late-bind html rendering code. So, we kinda trust that if the user can create their own functions, they can escape the results as well, if needed. That way the it doesn't matter if the function is inside {* ... *} or {{ ... }}. And you can have results escaped by calling that function:

{{func()}}

And if you don't want to write your own escaping you can always use the templates:

local escape = require "resty.template".escape
local function func() {
    ....
    return escape(...)
} 

So I kinda think this is just the way it is. We can always discuss this more, but it feels a lot like a non-issue to me. Try to convince me otherwise, :-).

isage commented 9 years ago

Hm, okay, i'm convinced about functions. Still, some additional work, but ok. Still not sure about '/' though. Looking at other languages (php, rails, mojolicious) - they do not escape it. Even if we do something like [code] template.render([[<sometag {{data}}>]], { data = "/>oops" }) [/code] it won't (or shouldn't) break this tag, because '>' are escaped.

bungle commented 9 years ago

This is hypothetical example, but let's say you have this:

<div id={{something_untrusted}}>
  <section>
    ...
  </section>
</div>

Now say that this something_untrusted is something user has set or something, now if we don't escape forward slash, it might then break your whole page at least UI-wise, eg. something_untrusted = "plaaplaa /", will result:

<div id=plaaplaa />
  <section>
    ...
  </section>
</div>

Now this is fucked up html, see the first div element is now closed. So this is just another barrier of safety here. The current implementation will result this:

<div id=plaaplaa &#47;>
  <section>
    ...
  </section>
</div>

Not nice, but at least it would not screw rendering. I think that in some weird scenarios this may even lead to some vulnerabilities.

isage commented 9 years ago

Hm. You're right. But i went ahead and checked: chrome/opera/firefox simply ignore that '/' (because div is paired tag, and can not be closed this way), at least with html5 doctype. Don't know about IE, though.

bungle commented 9 years ago

That could be. Do you have some real world issues because of this? I'd rather leave it there, because I don't see how it is hurting, and guys at OWASP may have better examples. This was just one example that popped up from my head (that's why I said it is hypothetical). There are a lot of HTML reading clients, I assume that at least someone has screwed their implementation, see people parse HTML even with regexes.

isage commented 9 years ago

No, no issues, really. It's just, like a said, was a little unusual.