Allow me to just put this out there: using handlerbars or mustache or anything similar to it is a *terrible* idea for MediaWiki. {{this is escaped}} and {{{this is not escaped}}}. The only difference is an extra brace on each side, and considering how many developers here are also familiar with writing wikitext, the probability of an accidental security vulnerability would increase significantly.
If we were to use a string-based templating engine (and looking at the progress of gwicke's work, it's more likely we'll go DOM-based), we'd want something that, at the very least, does not give the opportunity for a screwup like this.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science
On Sun, Mar 30, 2014 at 5:23 AM, Nuria Ruiz nuria@wikimedia.org wrote:
<div class={{something}}></div> is vulnerable, if something is set to "1234 onClick=doSomething()"
$html = Html::element( 'div', array( 'class' => $anything ),
$anythingElse
I see. Sorry but where I disagree is that the "quote me this
replacement"
is a lawful case for the template engine.
I'm not sure I understand what you're saying here. Do you mean makesafeString in your example shouldn't quote the text, but should
instead
remove space characters?=
What I am saying is that the parsing and escaping scheme we need is much simpler if you disallow the use case of passing the template engine something that is not data.
Let me explain as this as it has to do more with correctness that with security per se: A template engine objective is to separate data from markup. In your example you are passing the template 'class="anything"' or 'onclick="something"' neither "class" nor "onclick" are data. Thus what I am arguing is that the template engine should not support the use case of "add any random attribute or javascript to my html element with the right set of quotes" as a "lawful" use case. The template engine should not be expected to parse and insert code and "onclick" is code.
With a new template engine our main objective should be to separate data from markup, not supporting an style of coding that includes "onClick" attributes mixed with HTML which was prevalent years ago or css classes mixed with controller code.
On my experience reducing use cases for template engine to just data handling while having specific functions that deal with links and translations simplifies the escaping problem greatly as you do not need context aware escaping. You can "js-escape" any piece of data sent to the engine cause you know you do not support the use case of sending javascript to be inserted.
On Wed, Mar 26, 2014 at 6:41 PM, Chris Steipp csteipp@wikimedia.org wrote:
On Wed, Mar 26, 2014 at 10:30 AM, Nuria Ruiz nuria@wikimedia.org
wrote:
Additionally, how you escape a plain parameter like class vs. an href vs. a parameter that is inserted into a url vs. an id attribute
are
all different escaping strategies.
Urls in the template engine need to be handled on their own, sure. But
what
template engine does not work in this fashion? There are three separate "entities" you normally deal with when doing replacement: translations, urls and plain attributes.
$html = Html::element( 'div', array( 'class' => $anything ),
$anythingElse
I see. Sorry but where I disagree is that the "quote me this
replacement"
is a lawful case for the template engine.
I'm not sure I understand what you're saying here. Do you mean makesafeString in your example shouldn't quote the text, but should
instead
remove space characters?
The line above is doing a lot more than purely templating and on my opinion it does little to
separate
data and markup. Which is the very point of having a template engine.
But if you consider that one a lawful use case, you are right. The
example
I provided does not help you.
On Wed, Mar 26, 2014 at 6:15 PM, Chris Steipp csteipp@wikimedia.org wrote:
On Wed, Mar 26, 2014 at 9:44 AM, Daniel Friesen daniel@nadir-seen-fire.comwrote:
On 2014-03-26, 9:32 AM, Nuria Ruiz wrote:
> The issue is that they apply the same escaping, regardless of
the
> html context. So, in Twig and mustache, <div
class={{something}}></div>
is
> vulnerable, if something is set to "1234 onClick=doSomething()". Right, the engine would render:
<div class=1234 onClick=doSomething()> </div>
because it only escapes HTML by default. Now, note that the problem can be fixed with <div
class={{makeStringSafe
something}}>
Where "makestringSafe" is a function defined by us and executed
there
that
escapes to our liking.
How does a custom function jammed into the middle of a Mustache
template
fix the issue when the issue is not that foo={{something}} doesn't escape, but is that quoting is needed instead of escaping, and
Mustache
isn't context sensitive so neither Mustache or a custom function
know
that foo={{something}} is an attribute value in need of quoting?
Exactly. Additionally, how you escape a plain parameter like class
vs.
an
href vs. a parameter that is inserted into a url vs. an id attribute
are
all different escaping strategies. So there would be many different "makeXXXXStringSafe" and probably "quoteAndMakeXXXXStringSafe"
functions,
and code review would have to make sure the right one was being used
in
the
right place. Which means someone who is familiar with all of the xss techniques would need to code review almost all the templates.
For comparison, using our current html templating (as much as it
sucks):
$html = Html::element( 'div', array( 'class' => $anything ),
$anythingElse
);
The developer doesn't need to have any knowledge of what escaping
needs
to
apply to the class attribute vs the text.
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [
]
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l