$::FORM is not tainted under perl 5.6.1

RESOLVED FIXED in Bugzilla 2.16



16 years ago
5 years ago


(Reporter: bbaetz, Assigned: bbaetz)


Bugzilla 2.16




(1 attachment, 1 obsolete attachment)

1.63 KB, patch
: review+
Jouni Heikniemi
: review+
Details | Diff | Splinter Review


16 years ago
While looking into why I didn't see bug 155700, it turns out that perl > 5.6.0
has broken |use taint 're'|, which we use to avoid tainting . I've filed a bug
with a test case - see the URL.

The workarround is to assign a known-tainted value to $item and $value first,
before. The alternate fix is to use split, rather than $1 (like CGI.pm does), or
to avoid using $1, and just assign from the result of the m// directly (which
appears to avoid triggering this bug)

I'll wait to see what the response is before deciding which one to do, but we
should do one of them for 2.16.

Comment 1

16 years ago
*** Bug 155791 has been marked as a duplicate of this bug. ***

Comment 2

16 years ago
Oops, forgot to move to 2.16.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16

Comment 3

16 years ago
Created attachment 90350 [details] [diff] [review]

OK, try this. If we use split instead of a regexp, we don't have to worry about
the perl bug at all
Comment on attachment 90350 [details] [diff] [review]

This looks good, makes sense, and is much simpler to boot, but since split
takes a regular expression separator are you sure it doesn't also detaint the
values?  How can I test this?

r=myk, but it should be tested before going in
Attachment #90350 - Flags: review+

Comment 5

16 years ago
myk: You can add some |print "\n\n" . is_tainted($::FORM('foo'))| lines Also,
the previous taint bug showed up on 5.6.1 with this patch. I guess we should
test 5.6.0 and 5.005, though.

I was going to |die of not is_tainted(...)| in CGI.pl, but decided that it
wasn't worth it.
Tested and works.  r=myk for real, but this should get a second review.

Comment 7

16 years ago
Comment on attachment 90350 [details] [diff] [review]

Tested on RH 7.3 Perl 5.6.1, this really fixes it. Nice debugging there,

I think a 5.0* test would be in order here, but unfortunately I can't access
anything but a 5.6.1 installation now. 

You have r=jouni, use it if you can't get anyone with a 5.0* Perl to test this.

Comment 8

16 years ago
Comment on attachment 90350 [details] [diff] [review]

Oops, I forgot to write down these nitty comments I prepared... So what I said
still stands, but please address these before checkin:

>+    # We must make sure that this CGI params remain tainted.

"this CGI params"... s/this/the/, or whatever was your intention.

>+    # That means that if you use a regexp here, you must
>+    # |use re 'taint'| _and_ make sure that you don't run into
>+    # http://bugs.perl.org/perlbug.cgi?req=bug_id&bug_id=20020704.001

What does this 'if you use a regexp here' mean? Why would somebody do this? If
there's no reason, write "using a regexp here triggers http://bugs..., so we
split instead." (or sth like that).

>+        $value = '' if not defined $value;

The Bugzillan way of writing this seems to be "$value |= ''".

>@@ -556,6 +545,11 @@
>     }
>     $user{'groups'} = \%groups;
>+    # We need this here so that we can display the "My Votes" link in the
>+    # footer
>+    GetVersionTable();
>+    $user{'use_votes'} = $::anyvotesallowed;
>     return \%user;
> }

Wipe this hunk out unless you've got a good reason for it. :-)

Comment 9

16 years ago
Created attachment 90698 [details] [diff] [review]

Comment nits fixed.

I just noticed that CGI.pm uses split, so this is definately the way to go.
Attachment #90350 - Attachment is obsolete: true
Comment on attachment 90698 [details] [diff] [review]

Comment-only fixes; my review still applies, and I have tested it on Perl 5.6.0
on Solaris, and everything works as expected.
Attachment #90698 - Flags: review+

Comment 11

16 years ago
Comment on attachment 90698 [details] [diff] [review]

If cgi.pm does 'split' as well and Myk tested this on 5.6.0, I'm convinced. :-) 
Attachment #90698 - Flags: review+

Comment 12

16 years ago
Checked into trunk + branch.
Last Resolved: 16 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.