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(a)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(a)wikimedia.org>
wrote:
On Wed, Mar 26, 2014 at 10:30 AM, Nuria Ruiz
<nuria(a)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(a)wikimedia.org>
> wrote:
>
> > On Wed, Mar 26, 2014 at 9:44 AM, Daniel Friesen
> > <daniel(a)nadir-seen-fire.com>wrote;wrote:
> >
> > > 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) [
http://danielfriesen.name/
]
>
>
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l