155793 - $::FORM is not tainted under perl 5.6.1

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

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

[Bradley Baetz (:bbaetz)]

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

Comment 2

[Bradley Baetz (:bbaetz)]

Oops, forgot to move to 2.16.

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, try this. If we use split instead of a regexp, we don't have to worry about
the perl bug at all

Comment 4

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

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

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

Comment 5

[Bradley Baetz (:bbaetz)]

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.

Comment 6

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

Tested and works.  r=myk for real, but this should get a second review.

Comment 7

[Jouni Heikniemi]

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

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

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

[Jouni Heikniemi]

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

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

[Bradley Baetz (:bbaetz)]

Attachment: v2

Comment nits fixed.

I just noticed that CGI.pm uses split, so this is definately the way to go.

Comment 10

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

Comment on attachment 90698 [details] [diff] [review]
v2

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

Comment 11

[Jouni Heikniemi]

Comment on attachment 90698 [details] [diff] [review]
v2

If cgi.pm does 'split' as well and Myk tested this on 5.6.0, I'm convinced. :-) 
r=jouni

Comment 12

[Bradley Baetz (:bbaetz)]

Checked into trunk + branch.