problems using taint to check an array be created by cgi

[phill hw]

Hello Usenet Perl,
I have a html form which produces a load of checkboxes. They all have the
same name (sports) and if a check box is ticked(checked) it holds a numeric value
which represents the id of the sport.:

Pseudo CGI FORM:
<FORM>
<input type="text" name="s" value="aslakslad1231">
<p><input type="checkbox" name="sport" value="1">Football</p>
<p><input type="checkbox" name="sport" value="2">Basketball</p>
<p><input type="checkbox" name="sport" value="3">Hockey</p>
<SUBMIT BUTTON>
</FORM>

When the form data is send to the cgi script for processing:

This works with taint on, but I have to parse the values in the @sports
array and put them into another array (@tsports) before I can use them. If I do
not use this second array the data in the @sports array is still considered
tainted and I cannot use it:

#!/usr/bin/perl -Tw

use strict;
use CGI qw/:standard :html3/;
use CGI::Carp 'fatalsToBrowser';

if (param())
{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');
my (@sports) = $query->param('sports');
my (@tsports);

foreach(@sports)
{
if ($_ =~ /^([\d]+)$/)
{
push(@tsports, $_);
}
}
}


If I use the following script, I cannot use the data contained in the @sports array
as it is still considered tainted.

#!/usr/bin/perl -Tw

use strict;
use CGI qw/:standard :html3/;
use CGI::Carp 'fatalsToBrowser';

if (param())
{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');
my (@sports) = $query->param('sports')=~ /^([\d]+)$/ if $query->param('sports');

}


How can I correctly parse the @sports array to allow for numbers only without
having to construct a second array? Is this possible or doe I hav to
parse the contents of the first array and effectively do a taint check on each value
contained in the first array?

Thankyou
Phill

 

[A. Sinan Unur]

Hello Usenet Perl,
I have a html form which produces a load of checkboxes. They all have
the same name (sports) and if a check box is ticked(checked) it holds
a numeric value which represents the id of the sport.:

Pseudo CGI FORM:
<FORM>
<input type="text" name="s" value="aslakslad1231">
<p><input type="checkbox" name="sport" value="1">Football</p>
<p><input type="checkbox" name="sport" value="2">Basketball</p>
<p><input type="checkbox" name="sport" value="3">Hockey</p>
<SUBMIT BUTTON>
</FORM>

May I suggest that this is probably not a good way to set up the form? Life
would probably be easier if you had, e.g.

This works with taint on, but I have to parse the values in the

You are not parsing them, you are untaiting them. The difference is
important.

Here is how I might do it although I would urge you to take it with a grain
of salt:

use strict;
use warnings;

use Data:umper;

my %valid = map { $_ => 1 } qw(Basketball Football Hockey);
my @sports = qw(Basketball Hockey /etc/password);
@sports = map { $valid{$_} ? ($_) = ( $_ =~ /^(.+)$/ ) : '' } @sports;

print Dumper \@sports;

__END__

All of the code I show below is untested.

#!/usr/bin/perl -Tw

use warnings;

rather than -w.

use CGI qw/:standard :html3/;

Below, you use only the object interface, so a

use CGI;

would suffice here.

use CGI::Carp 'fatalsToBrowser';

if (param())

This is very unnecessary. Just contruct your CGI object.

{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

A few errors and style issues here. Your code would fail to untaint $s if
$query->param('s') returned '0' or '0E0' or '0 but true'. Also, I remember
discussions where it was mentioned that

my $s = 'something' if some other thing;

is not really a safe construct.

You are better off actually spelling out what you are doing:

my $s = $query->param('s');

if(defined $s and $s =~ /^(\w+)$/) {
$s = $1;
} else {
$s = '';
}

\w itself is a character class. I am not sure [\w] is an error but is
unnecessary and confusing.

my (@sports) = $query->param('sports');

I'll recommend setting up a hash of acceptable values for sports as above
and using the keys to validate the values. If the value is a key in %valid,
then you know it is safe to blindly capture it.

if ($_ =~ /^([\d]+)$/)

Again, \d is a character class of its own. And, by the way, is
987327332882727273774746655222411313232638482929399949872873498238971232838
123847328723984792837482374982374621347326473624723648723648732648726423444
23424 a valid value for sports?

Sinan.

 

[phill hw]

Am Mon, 15 Nov 2004 23:43:05 +0000 schrieb A. Sinan Unur:

Hello Usenet Perl,
I have a html form which produces a load of checkboxes. They all have
the same name (sports) and if a check box is ticked(checked) it holds
a numeric value which represents the id of the sport.:

Pseudo CGI FORM:
<FORM>
<input type="text" name="s" value="aslakslad1231">
<p><input type="checkbox" name="sport" value="1">Football</p>
<p><input type="checkbox" name="sport" value="2">Basketball</p>
<p><input type="checkbox" name="sport" value="3">Hockey</p>
<SUBMIT BUTTON>
</FORM>

May I suggest that this is probably not a good way to set up the form? Life
would probably be easier if you had, e.g.

This works with taint on, but I have to parse the values in the

You are not parsing them, you are untaiting them. The difference is
important.

Here is how I might do it although I would urge you to take it with a grain
of salt:

use strict;
use warnings;

use Data:umper;

my %valid = map { $_ => 1 } qw(Basketball Football Hockey);
my @sports = qw(Basketball Hockey /etc/password);
@sports = map { $valid{$_} ? ($_) = ( $_ =~ /^(.+)$/ ) : '' } @sports;

print Dumper \@sports;

__END__

All of the code I show below is untested.

#!/usr/bin/perl -Tw

use warnings;

rather than -w.

use CGI qw/:standard :html3/;

Below, you use only the object interface, so a

use CGI;

would suffice here.

use CGI::Carp 'fatalsToBrowser';

if (param())

This is very unnecessary. Just contruct your CGI object.

{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

A few errors and style issues here. Your code would fail to untaint $s if
$query->param('s') returned '0' or '0E0' or '0 but true'. Also, I remember
discussions where it was mentioned that

my $s = 'something' if some other thing;

is not really a safe construct.

You are better off actually spelling out what you are doing:

my $s = $query->param('s');

if(defined $s and $s =~ /^(\w+)$/) {
$s = $1;
} else {
$s = '';
}

\w itself is a character class. I am not sure [\w] is an error but is
unnecessary and confusing.

my (@sports) = $query->param('sports');

I'll recommend setting up a hash of acceptable values for sports as above
and using the keys to validate the values. If the value is a key in %valid,
then you know it is safe to blindly capture it.

if ($_ =~ /^([\d]+)$/)

Again, \d is a character class of its own. And, by the way, is
987327332882727273774746655222411313232638482929399949872873498238971232838
123847328723984792837482374982374621347326473624723648723648732648726423444
23424 a valid value for sports?

Sinan.

Thanks Sinan
You have pointed out a few interesting things that I will look
further into and been very helpful. I find it really difficult to get
clear answers with the taint subject. Most people just write
"Oh here is a regexp ... blah .. blah but IMHO it still cannot be considered safe".
Which does not really help at all.


My code:
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

is from the perl cookbook http://www.hk8.org/old_web/linux/cgi/ch08_04.htm
so I consider it safe as I am very restrictive on params.
I have to admit your method reads more clearly as the action
on the undefined $s is immediately apparent. I usually add a default value
or fail the script shortly after. It depends on the script.


my $s = $query->param('s');

if(defined $s and $s =~ /^(\w+)$/) {
$s = $1;
} else {
$s = '';
}

I will test your method:

use Data:umper;

my %valid = map { $_ => 1 } qw(Basketball Football Hockey);
my @sports = qw(Basketball Hockey /etc/password);
@sports = map { $valid{$_} ? ($_) = ( $_ =~ /^(.+)$/ ) : '' } @sports;

print Dumper \@sports;

but what concerns me here is that the values are stored in the cgi script
which I find hard to maintain. That is why I only pass numberic values back
because they are then matched to values in a db. Hockey, Football and
Hockey are easy examples.

What about if my form looked like:

Which of the following have you backed up?
<p><input type="checkbox" name="passwd" value="/etc/passwd">etc/p>
<p><input type="checkbox" name="passwd" value="/var/mysql.users">mysql/p>

Again, \d is a character class of its own. And, by the way, is
987327332882727273774746655222411313232638482929399949872873498238971232838
123847328723984792837482374982374621347326473624723648723648732648726423444
23424 a valid value for sports?

Opps, I really do need to check the length! Thanks for pointing that out.

Phill

 

[A. Sinan Unur]

Am Mon, 15 Nov 2004 23:43:05 +0000 schrieb A. Sinan Unur:

news[email protected]:

....

May I suggest that this is probably not a good way to set up the
form? Life would probably be easier if you had, e.g.

<p><input type="checkbox" name="sport" value="Hockey">Hockey</p>

....

My code:
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

is from the perl cookbook
http://www.hk8.org/old_web/linux/cgi/ch08_04.htm so I consider it safe
as I am very restrictive on params.

On the other hand, using that page is tantamount to stealing from the
authors. Please don't use those pages, and complain loudly when you
encounter them. The authors of these books need to eat too.

And, it does not change my opinion that the line above is too long,
unnecassarily uses [\w+] etc.

I have to admit your method reads more clearly as the action on the
undefined $s is immediately apparent.

It is something I learned reading this group. I would rather read

my $s = $query->param('s');

if(defined $s and $s =~ /^(\w+)$/) {
$s = $1;
} else {
$s = '';
}

than the method you have above.

I will test your method:

use Data:umper;

my %valid = map { $_ => 1 } qw(Basketball Football Hockey);
my @sports = qw(Basketball Hockey /etc/password);
@sports = map { $valid{$_} ? ($_) = ( $_ =~ /^(.+)$/ ) : '' } @sports;

print Dumper \@sports;

but what concerns me here is that the values are stored in the cgi
script which I find hard to maintain. That is why I only pass numberic
values back because they are then matched to values in a db. Hockey,
Football and Hockey are easy examples.

But you are going to maintain a mapping between those values and the actual
names anyway.

What about if my form looked like:

Which of the following have you backed up?
<p><input type="checkbox" name="passwd" value="/etc/passwd">etc/p>
<p><input type="checkbox" name="passwd"
value="/var/mysql.users">mysql/p> <p><input type="checkbox"
name="passwd" value="/etc/pppoe.secrets">pppoe/p>

Well, so long as those were the valid inputs, then you would untaint
against them. I do not see what difference it makes in terms of untainting.
By untainting, you are making sure that the input you are receiving is
something you are expecting and know how to deal with in your script. So,
no one help you if, after verifying that the user sent the input
'/etc/passwd', decide to echo the contents of that file. The taint
mechanism makes sure you won't inadvertently do so.

Opps, I really do need to check the length! Thanks for pointing that
out.

More than just length, there must be rule that tells you exactly which
numbers are valid, but length is a consideration too.

Sinan.

 

[Gunnar Hjalmarsson]

If I use the following script, I cannot use the data contained in the @sports array
as it is still considered tainted.

#!/usr/bin/perl -Tw

use strict;
use CGI qw/:standard :html3/;
use CGI::Carp 'fatalsToBrowser';

if (param())
{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');
my (@sports) = $query->param('sports')=~ /^([\d]+)$/ if $query->param('sports');

The m// operator shall be applied to a string, not a list or array.

How can I correctly parse the @sports array to allow for numbers only without
having to construct a second array?

You need to untaint each element separately. The map() function comes to
mind:

my @sports = map { /^(\d+)$/ } $q->param('sport');

 

[A. Sinan Unur]

my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

See http://tinyurl.com/4vr53

Sinan

 

[phill hw]

Am Wed, 17 Nov 2004 03:30:53 +0100 schrieb Gunnar Hjalmarsson:

If I use the following script, I cannot use the data contained in the @sports array
as it is still considered tainted.

#!/usr/bin/perl -Tw

use strict;
use CGI qw/:standard :html3/;
use CGI::Carp 'fatalsToBrowser';

if (param())
{
my ($query) = new CGI;
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');
my (@sports) = $query->param('sports')=~ /^([\d]+)$/ if $query->param('sports');

The m// operator shall be applied to a string, not a list or array.

How can I correctly parse the @sports array to allow for numbers only without
having to construct a second array?

You need to untaint each element separately. The map() function comes to
mind:

my @sports = map { /^(\d+)$/ } $q->param('sport');

Thats the answer I was looking for

Thankyou

Phill

 

[phill hw]

My code:
my ($s) = $query->param('s') =~ /^([\w]+)$/ if $query->param('s');

is from the perl cookbook
(link) so I consider it safe
as I am very restrictive on params.

On the other hand, using that page is tantamount to stealing from the
authors. Please don't use those pages, and complain loudly when you
encounter them. The authors of these books need to eat too.

Yes you are probably right about the link - Blam Google. Luckily the
"security chapter" is available from oreilly as the sample chapter for the
second edition.

http://www.oreilly.com/catalog/cgi2/chapter/ch08.html

The first edition of this book is completely available as an openbook
http://www.oreilly.com/openbook/cgi/ because its out of print.


I know the authors need to eat thats why I always buy and have purchased
plus many others from oreilly. I just wanted to demonstrate that it is common
method of writing a line of code for checking tainted data. I was
not complaining loudly as I use the method whether is visually appealing
is not of the upmost concern to me for my own scripts. You link to
http://tinyurl.com/4vr53 which my newsserver did not display until
today well after your second post is a super example about problems
finding things in documentation.

I could not find in any of the Perl books "I have purchased" an
example of untainting an array. Maybe I need to read them again.
Thats why I asked this newsgroup. If anyone knows one with its
ISBN number I will buy a copy of it . My original question was if
there is a similar way to untaint the data in an
array without having to use another array.

Thanks

Phill

 

[phill hw]

Again, \d is a character class of its own. And, by the way, is
987327332882727273774746655222411313232638482929399949872873498238971232838
123847328723984792837482374982374621347326473624723648723648732648726423444
23424 a valid value for sports?

Sinan.

Theoretically it is, as long as its unsigned

Phill