Closed
Bug 238797
Opened 21 years ago
Closed 20 years ago
edit Flag Types needs a filter for javascript
Categories
(Bugzilla :: Administration, task)
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)
2.45 KB,
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
goobix
:
review+
|
Details | Diff | Splinter Review |
create a flag with " in its name
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
Assignee | ||
Updated•21 years ago
|
Severity: major → critical
Flags: blocking2.18? → blocking2.18+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Comment 3•21 years ago
|
||
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•21 years ago
|
||
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)
Assignee | ||
Updated•21 years ago
|
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1]
Assignee | ||
Comment 5•21 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 7•20 years ago
|
||
(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 ' 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 :)
Assignee | ||
Comment 8•20 years ago
|
||
This touches several templates, some are public-facing...
Assignee | ||
Comment 9•20 years ago
|
||
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 10•20 years ago
|
||
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+
Assignee | ||
Comment 11•20 years ago
|
||
Comments from the security/standards folks please?
Assignee | ||
Comment 12•20 years ago
|
||
(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
Assignee | ||
Comment 13•20 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #152747 -
Flags: review?(gerv)
Comment 15•20 years ago
|
||
Comment on attachment 152747 [details] [diff] [review]
Patch v2
Looks good. r=gerv.
Gerv
Attachment #152747 -
Flags: review?(gerv) → review+
Assignee | ||
Comment 16•20 years ago
|
||
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]
Assignee | ||
Comment 17•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #152771 -
Flags: review?(gerv)
Comment 18•20 years ago
|
||
Comment on attachment 152771 [details] [diff] [review]
Patch for 2.16
r=vladd
Attachment #152771 -
Flags: review?(gerv) → review+
Updated•20 years ago
|
Whiteboard: [wanted for 2.16.6] [ready for 2.18rc1] → [ready for 2.16.6] [ready for 2.18rc1]
Updated•20 years ago
|
Flags: approval?
Flags: approval2.18?
Flags: approval2.16?
Assignee | ||
Comment 19•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Flags: blocking2.16.7+
Assignee | ||
Updated•20 years ago
|
Group: webtools-security
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•