238797 - edit Flag Types needs a filter for javascript

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[timeless]

create a flag with " in its name

Comment 1

[timeless]

fwiw, clicking delete
<http://bugzilla.mozilla.org/editflagtypes.cgi?action=confirmdelete&id=89>
gives me an ISE:

Internal Server Error
The server encountered an internal error or misconfiguration and was unable to
complete your request.

Please contact the server administrator, [e-mail] and inform them
of the time the error occurred, and anything you might have done that may have
caused the error.

More information about this error may be available in the server error log.

Apache/1.3.27 Server at bugzilla.mozilla.org Port 80

Comment 2

[timeless]

sorry, it's > that's doing the damage, not "

Comment 3

[Gervase Markham [:gerv]]

Timeless: can you please NOT use b.m.o. as a playground for testing possible
Bugzilla security bugs?

Everyone else: 

Our filters are in a twist again. We've got several problems here.

The problematic flag name is (to all intents and purposes) 
Foo'">Foo

Currently, we "FILTER js" the name. This puts a \ before the ", but that doesn't
stop it terminating the HTML attribute. Hence the dodgy rendering in the
"Delete" link.

It would seem we want "FILTER html". However, this doesn't do the trick either,
because that doesn't escape the single quote character, so the JS dies.

The right thing might be for the html filter to replace ' with ' (which does
work cross-browser - " doesn't) but it's a TT built-in so it's a bit tricky
to fix.

The workaround seems to be to FILTER html FILTER js. (The other way around works
too, but looks weirder.) Is this what we need to be doing for all JS which isn't
inside script tags?

The Developers Guide is silent on the HTML vs. JS filtering question.

I'm not sure what the Internal Server error is about. Nothing appears in mecha's
error log when I trigger it.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

Right, this is similar to the FILTER url FILTER html stuff mentioned in the TT
docs (which we avoid by using url_quote, but thats separate)

Comment 5

[Dave Miller [:justdave]]

fwiw, we already have FILTER html overridden in Bugzilla/Template.pm for
purposes of escaping the @ in email addresses.  Adding an escape for ' to our
existing override would be trivial.

Comment 6

[Jouni Heikniemi]

Note that this issue is already partially disclosed by bmo having a 'ignore me,
I'm just here to cause ">trouble' flag type that shows up in requests.cgi
dropdown. Not that it makes a lot of difference in practice, but yet another
reason for not testing security issues on bmo.

Comment 7

[Dave Miller [:justdave]]

(In reply to comment #3)
> The right thing might be for the html filter to replace ' with '

> The workaround seems to be to FILTER html FILTER js. (The other way around
> works too, but looks weirder.) Is this what we need to be doing for all JS
> which isn't inside script tags?

Actually, it doesn't work with the html first.  The js filter has to be first. 
With the ' patch in place on the html filter, html changes the ' to a ',
which now no longer looks like a ', so filter js doesn't escape it.  The browser
decodes the &#39 before the js interpreter sees it, and the unescaped ' is now a
problem.

So the proper way to do it is [% foo FILTER js FILTER html %]

We still need the FILTER html within <script> blocks, too, as otherwise an HTML
tag appearing inside a js string would get interpreted.  (I just tried it, with
</script> inside a product name, and the HTML parser found it and displayed the
rest of my javascript in the page content :)

Comment 8

[Dave Miller [:justdave]]

Attachment: Patch v1

This touches several templates, some are public-facing...

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 152662 [details] [diff] [review]
Patch v1

I considered updating test 8 to check for this, but in order to do it right, it
needs to know the filetype of the file being testing, and conditionally require
both IFF the file is html...  and that didn't look very obvious at first
glance.

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 152662 [details] [diff] [review]
Patch v1

Dave: I believe that the "</script>" example is actually a special one - AIUI,
this is the only bit of HTML which exhibits this problem. Browsers otherwise
treat the contents of <script> tags as all JS data. (Obviously, </script> has
to be special!) 

I don't know if this means that we actually need the HTML filtering inside
<script> tags or not. 

Do you need to update the escaping section in the 
Developers Guide?

The patch looks fine, assuming you've tested all the instances with various
bits of potentially unsafe data and nothing breaks.

Gerv

Comment 11

[Dave Miller [:justdave]]

Comments from the security/standards folks please?

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #10)
> (From update of attachment 152662 [details] [diff] [review])
> Dave: I believe that the "</script>" example is actually a special one - AIUI,
> this is the only bit of HTML which exhibits this problem. Browsers otherwise
> treat the contents of <script> tags as all JS data. (Obviously, </script> has
> to be special!) 
> 
> I don't know if this means that we actually need the HTML filtering inside
> <script> tags or not. 

Hmm, well, we probably at least need to filter for </script> then in those
cases.  Probably easier to just filter for HTML in general.  It doesn't seem to
break anything to do it.

> Do you need to update the escaping section in the 
> Developers Guide?

Yes, whatever we decide on here (I'd like input from Jesse and Ian on it) will 

Comment 13

[Dave Miller [:justdave]]

OK, talked with Jesse on IRC, html-encoding the stuff in <script> is bad,
because Mozilla doesn't decode &entities; within <script> blocks.

The suggestion from him is to make the js filter encode / with \ in front of it,
because javascript will just treat a \/ as a / but it'll defeat it from being
interpreted as HTML.

Comment 14

[Dave Miller [:justdave]]

Attachment: Patch v2

New patch, based on Jesse's recommendations.  Adds '/' to the list of escaped
characters in the js filter, adds the html filter to the two places that were
inside HTML attributes that were missing it.

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 152747 [details] [diff] [review]
Patch v2

Looks good. r=gerv.

Gerv

Comment 16

[Dave Miller [:justdave]]

OK, the issue this is fixing on the trunk also happens to be present on the 2.16
branch as well (though it has nothing to do with flags there, obviously).  Patch
coming up.

Comment 17

[Dave Miller [:justdave]]

Attachment: Patch for 2.16

Comment 18

[Vlad Dascalu]

Comment on attachment 152771 [details] [diff] [review]
Patch for 2.16

r=vladd

Comment 19

[Dave Miller [:justdave]]

trunk:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.18; previous revision: 1.17
done
Checking in template/en/default/admin/flag-type/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--
 knob.html.tmpl
new revision: 1.6; previous revision: 1.5
done

2.16 branch:

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.22; previous revision: 1.169.2.21
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.7.2.6; previous revision: 1.7.2.5
done


Log comment:
Minor adjustment to javascript filters to prevent tags inserted in product,
component, and flag names from causing problems.