[% DEFAULT foo = bar FILTER html %] in bug/field-label.html.tmpl doesn't correctly filter the variable

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Bugzilla-General
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: John Lightsey, Unassigned)

Tracking

(Depends on: 1 bug)

4.5.5
Bugzilla 5.0

Details

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; Touch; .NET4.0E; .NET4.0C; Tablet PC 2.0; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; rv:11.0) like Gecko

Steps to reproduce:

In template/en/default/bug/field-label.html.tmpl, the DEFAULT Template::Toolkit syntax does not behave in the fashion the author expected:

[% DEFAULT field_name = field_descs.${field.name} FILTER html %]

The actual behavior is that the filtering is applied to the results of the variable assignment, not the data being assigned.

To reproduce:

Add a custom field with a description "<script>alert(document.domain)</script>" and set the option to make it mandatory for all products. Then open the new bug submission screen and the XSS will pop.

This quirk of the Template::Toolkit syntax is in Template::Toolkit's RT tracker as https://rt.cpan.org/Public/Bug/Display.html?id=59208

You get this odd behavior from both SET and DEFAULT.

I've tried several times to come up with a fix for TT's LALR parser to make filters with setlists behave as users expect them to behave, but it's a little beyond my ability. If anyone with Bugzilla wants to give it a try, there is a commit and revert in TT's github repo specifically attempting to address this issue.

It can be worked around by applying the filter before or after DEFAULT or SET:

[% DEFAULT field_name = field_descs.${field.name}; field_name = field_name FILTER html  %]

Comment 1

4 years ago
This is indeed a bug (which I played with for months already as I have a custom field description with <script> in it), but this is not a security issue. Custom fields are created by admins only, and those admins can already customize templates using dedicated parameters, including injecting XSS, if that's what they want.

It would still be good to fix, though. Instead of your workaround, one may also write:

[% DEFAULT field_name = field_descs.${field.name} %]
[% field_name = field_name FILTER html %]

which is more in line with our guidelines.
Group: bugzilla-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: stored-xss via field_descs → [% DEFAULT foo = bar FILTER html %] doesn't correctly filter the variable

Comment 2

4 years ago
Bug 1015226 introduced this code, so only 4.5.5 and 4.5.6 are affected. There are no other places where we write [% DEFAULT foo = bar FILTER html %]. I will give a look at [% SET foo = bar FILTER html %] too.
Severity: normal → minor
Depends on: 1015226
Target Milestone: --- → Bugzilla 5.0
Version: 4.5.6 → 4.5.5

Comment 3

4 years ago
(In reply to Frédéric Buclin from comment #2)
> %]. I will give a look at [% SET foo = bar FILTER html %] too.

No problem found with SET.
Summary: [% DEFAULT foo = bar FILTER html %] doesn't correctly filter the variable → [% DEFAULT foo = bar FILTER html %] in bug/field-label.html.tmpl doesn't correctly filter the variable

Comment 4

4 years ago
Bug 1015226 has been backed out.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.