1102842 - t/008filter.t has several unsafe filters whitelisted

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

return 1 if $directive =~ /FILTER\ (html|csv|js|base64|css_class_quote|ics|
                                        quoteUrls|time|uri|xml|lower|html_light|
                                        obsolete|inactive|closed|unitconvert|
                                        txt|html_linebreak|markdown|none|null)\b/x;

Several of the filters listed above are totally unsafe and should not be whitelisted: lower, obsolete, inactive, and closed. 'none' is also unsafe, but adding FILTER none to a directive is intentional and so is whitelisted on purpose.

obsolete, inactive and closed were added to Bugzilla 2.18 in bug 232397 to fix bustage. Of course, the right fix should have been to append FILTER html or FILTER none instead of whitelisting them to make runtests.pl happy.

lower was added to Bugzilla 2.18 in bug 174942 for cryptic reasons. Probably for laziness too.


IMO, the obsolete, inactive and closed filters should go away. They look like hacks to me and are almost not used:

FILTER obsolete is only used in attachment/list.html.tmpl and attachment/show-multiple.html.tmpl.

FILTER inactive is only used in list/edit-multiple.html.tmpl.

FILTER closed is only used in bug/dependency-tree.html.tmpl.

FILTER lower is an upstream filter so it's fine.

Comment 1

[Matt Selsky [:selsky]]

Attachment: v1

Comment 2

[Frédéric Buclin]

Comment on attachment 8532888 [details] [diff] [review]
v1

>diff --git a/t/008filter.t b/t/008filter.t

>     return 1 if $directive =~ /FILTER\ (html|csv|js|base64|css_class_quote|ics|
>                                         quoteUrls|time|uri|xml|lower|html_light|

You forgot to also exclude 'lower', which is not a safe filter. Each occurence of 'FILTER lower' must be inspected, and the appropriate filter must be added (most probably FILTER html rather than FILTER none).



>diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl

>+          <b>[% '<span class="bz_obsolete">' IF attachment.isobsolete %][% attachment.description FILTER html %][% '</span>' IF attachment.isobsolete %]</b>

This line is way too long. Also, I much prefer:

  <b>
    [% IF attachment.isobsolete %]
      <span class="bz_obsolete">
    [% END %]
    [% attachment.description FILTER html %]
    [% IF attachment.isobsolete %]
      </span>
    [% END %]
  </b>

This is much easier to parse for developers than writing full HTML entities inside [% IF %] directives. Also, the hardcoded <b> should go away in favor of a CSS class. You can probably put bold in the bz_obsolete CSS class.


The same comment about moving HTML entities outside [% IF %] applies to all other changes you did in other templates.

Comment 3

[Gervase Markham [:gerv]]

I did a review, but didn't get a chance to post it until LpSolit beat me too it. Still, here it is:

This is a great start, but when LpSolit said that lower "is fine", he meant that we don't have code for it to remove. It's still a security risk, he's right. We need to remove it from the whitelist, and then change all callsites to use an additional filter such as 'html' or something else appropriate.

So r+ on all the work you've done already, but it needs this additional change to finish it off. 

Gerv

Comment 4

[Matt Selsky [:selsky]]

Attachment: bug-1102842-v2.patch

I left the <b> tag since not all instances of bz_obsolete use it.  Do you still want that converted to css?  We would need to add an additional span in order to style it.  Is that what you want?

Comment 5

[Gervase Markham [:gerv]]

Comment on attachment 8532919 [details] [diff] [review]
bug-1102842-v2.patch

Review of attachment 8532919 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv. That'll do nicely.

Gerv

Comment 6

[Gervase Markham [:gerv]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   7b8a5d1..4d1c399  master -> master

Gerv