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.
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).
Severity: normal → minor
We could probably fix this just by adding a /s+ at the beginning and end of the regex.
Created attachment 202357 [details] [diff] [review] patch, v1 this patch prevents bad filter names such as FILTER htmli, which isn't detected actually.
Assignee: zach → LpSolit
Status: NEW → ASSIGNED
Attachment #202357 - Flags: review?(wurblzap)
Comment on attachment 202357 [details] [diff] [review] patch, v1 r=wurblzap by inspection.
Attachment #202357 - Flags: review?(wurblzap) → review+
Target Milestone: --- → Bugzilla 2.22
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.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.18
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.
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: 126.96.36.199; previous revision: 188.8.131.52 done 2.18.4: Checking in t/008filter.t; /cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v <-- 008filter.t new revision: 184.108.40.206; previous revision: 220.127.116.11 done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
No particularly good reason, if the current patch works :-) Gerv
You need to log in before you can comment on or make changes to this bug.