297857 - incorrect filter name detection

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, defect)

Description

[Frédéric Buclin]

If an invalid filter name is the concatenation of a valid filter name followed
by any string (such as "htmllpsolit", "html" being a valid filter name),
008filter.t does not complain because its regexp is:

    return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_class_quote|
                                        ics|quoteUrls|time|uri|xml|lower|
                                        obsolete|inactive|closed|unitconvert|
                                        none)/x;

meaning that it looks for a substring at the beginning of the filter name only,
as in my example above.

Moreover, this test cannot catch an invalid filter name if this one is mixed
with a valid one, such as "FILTER html FILTER lpsolit".

This may lead to security holes, as unfiltered data may be displayed, see for
instance bug 297845, even if I wonder whether Template.pm would complain or not.

Setting the security flag till bug 297845 is fixed and till we determine if this
is exploitable or not.

Comment 1

[Frédéric Buclin]

ok, it's not exploitable. Templates.pm complains if it detects invalid filter
names. Bug 297845 is invalid because my testcase isn't due to a bad filter name
detection but due to the fact that the milestone URL is not filtered in
editproducts.cgi (this file really need to be templatized asap).

Comment 2

[Max Kanat-Alexander]

We could probably fix this just by adding a /s+ at the beginning and end of the
regex.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

this patch prevents bad filter names such as FILTER htmli, which isn't detected actually.

Comment 4

[Marc Schumann [:Wurblzap]]

Comment on attachment 202357 [details] [diff] [review]
patch, v1

r=wurblzap by inspection.

Comment 5

[Frédéric Buclin]

incorrect filtering may have security implications, even if TT should complain that some given filter doesn't exist. The patch applies on 2.18 too.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

I wonder if there's some reason why we did it like we did.  cc:ing gerv, who might know.  This patch certainly looks like the right thing to do.

Comment 7

[Frédéric Buclin]

tip:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.21; previous revision: 1.20
done

2.20:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.17.6.2; previous revision: 1.17.6.1
done

2.18.4:

Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.12.2.2; previous revision: 1.12.2.1
done

Comment 8

[Gervase Markham [:gerv]]

No particularly good reason, if the current patch works :-)

Gerv