Closed Bug 238797 Opened 19 years ago Closed 19 years ago

edit Flag Types needs a filter for javascript

Categories

(Bugzilla :: Administration, task)

2.17.6
task
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: timeless, Assigned: justdave)

References

()

Details

(Whiteboard: [fixed in 2.16.6] [fixed in 2.18rc1])

Attachments

(2 files, 1 obsolete file)

create a flag with " in its name
Flags: blocking2.18?
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, webmaster@mozilla.org 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
sorry, it's > that's doing the damage, not "
Severity: major → critical
Flags: blocking2.18? → blocking2.18+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
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 &#39; (which does
work cross-browser - &quot; 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
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)
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1]
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.
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.
(In reply to comment #3)
> The right thing might be for the html filter to replace ' with &#39;

> 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 &#39; patch in place on the html filter, html changes the ' to a &#39;,
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 :)
Attached patch Patch v1 (obsolete) — Splinter Review
This touches several templates, some are public-facing...
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.
Attachment #152662 - Flags: review?(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
Attachment #152662 - Flags: review?(gerv) → review+
Comments from the security/standards folks please?
(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 
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.
Attached patch Patch v2Splinter Review
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.
Attachment #152662 - Attachment is obsolete: true
Attachment #152747 - Flags: review?(gerv)
Comment on attachment 152747 [details] [diff] [review]
Patch v2

Looks good. r=gerv.

Gerv
Attachment #152747 - Flags: review?(gerv) → review+
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.
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1] → [wanted for 2.16.6] [ready for 2.18rc1]
Attached patch Patch for 2.16Splinter Review
Attachment #152771 - Flags: review?(gerv)
Comment on attachment 152771 [details] [diff] [review]
Patch for 2.16

r=vladd
Attachment #152771 - Flags: review?(gerv) → review+
Whiteboard: [wanted for 2.16.6] [ready for 2.18rc1] → [ready for 2.16.6] [ready for 2.18rc1]
Flags: approval?
Flags: approval2.18?
Flags: approval2.16?
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.
Flags: blocking2.16.7+
Flags: approval?
Flags: approval2.18?
Flags: approval2.16?
Flags: approval2.16+
Flags: approval+
Whiteboard: [ready for 2.16.6] [ready for 2.18rc1] → [fixed in 2.16.6] [fixed in 2.18rc1]
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Flags: blocking2.16.7+
Group: webtools-security
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.