178800 - Graphical Charts fail with taint error with perl 5.6.0

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect, P1)

Description

[Joel Peshkin]

On sites with 5.6.0 (mothra), queries for graphical charts such as severity
versus status are tainted.

This is most likely due to taints being propagated through 
 my @selectnames = map($columns{$_}, @axis_fields);
in report.cgi

Comment 1

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

Attachment: patch v1: fixes problem per bbaetz' suggestion

Comment 2

[Joel Peshkin]

Comment on attachment 105429 [details] [diff] [review]
patch v1: fixes problem per bbaetz' suggestion

r=joel

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 105429 [details] [diff] [review]
patch v1: fixes problem per bbaetz' suggestion

a=justdave

but I want a second r= from someone besides me and joel before it gets checked
in.

Comment 4

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

Attachment: patch v2: w/validation

Here's an approach that validates the data before detainting and using it.

Comment 5

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

Attachment: patch v3: validation plus filters out html

Comment 6

[Gervase Markham [:gerv]]

Comment on attachment 105433 [details] [diff] [review]
patch v3: validation plus filters out html

>+# Validate the values in the axis fields or throw an error.
>+!$row_field 
>+  || ($columns{$row_field} && trick_taint($row_field))
>+  || ThrowCodeError("report_axis_invalid", 
>+                    { val=>$row_field, fld=>"horizontal axis"});

This won't localise properly; you either need three errors, or to pass an
additional tag ("horizontal_axis", "mult_tables" etc.) which you can fork on in
the error template.

Other than that, looks good.

Gerv

Comment 7

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

Attachment: patch v3: localizable

Comment 8

[Gervase Markham [:gerv]]

Comment on attachment 105459 [details] [diff] [review]
patch v3: localizable

>+# Validate the values in the axis fields or throw an error.
>+!$row_field 
>+  || ($columns{$row_field} && trick_taint($row_field))
>+  || ThrowCodeError("report_axis_invalid", { fld=>"x", val=>$row_field});

Nit: inconsistent spacing inside {} brackets.

Fix that, and r=gerv.

Gerv

Comment 9

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

Attachment: patch v4: consistent spacing

re: inconsistent spacing; good catch, done.

Did you really mean to call this a "nit" or withhold review because of it? 
When I use that word it means "I want you to change that, but I won't deny you
review if you don't."  We should be consistent in how we use it, or we're going
to confuse patch authors.

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 105491 [details] [diff] [review]
patch v4: consistent spacing

My definition of "Nit" is "small thing that the author will think I'm being
really petty about unless I label it as a nit". I tend to specify things which
need to be fixed, and things which only could be fixed, independent of that
word.

So you are right - maybe we do need a definition :-)

Gerv

Comment 11

[Dave Miller [:justdave]]

a=justdave

Comment 12

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

Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v  <--  report.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.22; previous revision: 1.21
done