Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

I have a cgi script where I am passing variables like: script.pl?state=IL

I then create two variables, $state and $statename, and if the correct state abbrevieation is passed to my script I set the $state to the abbrevieation of the state and $statename to the states name. I'm doing this to prevent SQL injection attacks.

When I pass any variable to my script, $state and $statename allways end up being YU and Yukon Territories respectively. It looks like the last IF statement is the only one that executes and I can't figure out why.

I have posted the relevant script. Any help would be great.

#!/usr/local/bin/perl

use DBI;
use CGI;
use CGI::Carp qw(fatalsToBrowser);

$buffer = $ENV{'QUERY_STRING'};

@pairs = split(/&/, $buffer);
foreach $pair (@pairs) {
  ($name, $value) = split(/=/, $pair);
  $name =~ tr/+/ /;
  $name =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg;
  $value =~ tr/+/ /;
  $value =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg;
  $FORM{$name} = $value;
}


##Untaint Variables###################################################
+##########
if ($FORM{$state} == 'AL') {$state = 'AL'; $statename = 'Alabama'};
if ($FORM{$state} == 'AK') {$state = 'AK'; $statename = 'Alaska'};
if ($FORM{$state} == 'AZ') {$state = 'AZ'; $statename = 'Arizona'};
if ($FORM{$state} == 'AR') {$state = 'AR'; $statename = 'Arkansas'};
if ($FORM{$state} == 'CA') {$state = 'CA'; $statename = 'California'};
if ($FORM{$state} == 'CO') {$state = 'CO'; $statename = 'Colorado'};
if ($FORM{$state} == 'CT') {$state = 'CT'; $statename = 'Connecticut'}
+;
if ($FORM{$state} == 'DE') {$state = 'DE'; $statename = 'Delaware'};
if ($FORM{$state} == 'FL') {$state = 'FL'; $statename = 'Florida'};
if ($FORM{$state} == 'GA') {$state = 'GA'; $statename = 'Georgia'};
if ($FORM{$state} == 'HI') {$state = 'HI'; $statename = 'Hawaii'};
if ($FORM{$state} == 'ID') {$state = 'ID'; $statename = 'Idaho'};
if ($FORM{$state} == 'IN') {$state = 'IN'; $statename = 'Indiana'};
if ($FORM{$state} == 'IL') {$state = 'IL'; $statename = 'Illinois'};
if ($FORM{$state} == 'IA') {$state = 'IA'; $statename = 'Iowa'};
if ($FORM{$state} == 'KS') {$state = 'KS'; $statename = 'Kansas'};
if ($FORM{$state} == 'KY') {$state = 'KY'; $statename = 'Kentucky'};
if ($FORM{$state} == 'LA') {$state = 'LA'; $statename = 'Louisiana'};
if ($FORM{$state} == 'ME') {$state = 'ME'; $statename = 'Maine'};
if ($FORM{$state} == 'MD') {$state = 'MD'; $statename = 'Maryland'};
if ($FORM{$state} == 'MA') {$state = 'MA'; $statename = 'Massachusetts
+'};
if ($FORM{$state} == 'MI') {$state = 'MI'; $statename = 'Michigan'};
if ($FORM{$state} == 'MN') {$state = 'MN'; $statename = 'Minnesota'};
if ($FORM{$state} == 'MO') {$state = 'MO'; $statename = 'Missouri'};
if ($FORM{$state} == 'MS') {$state = 'MS'; $statename = 'Mississippi'}
+;
if ($FORM{$state} == 'MT') {$state = 'MT'; $statename = 'Montana'};
if ($FORM{$state} == 'NE') {$state = 'NE'; $statename = 'Nebraska'};
if ($FORM{$state} == 'NV') {$state = 'NV'; $statename = 'Nevada'};
if ($FORM{$state} == 'NH') {$state = 'NH'; $statename = 'New Hampshire
+'};
if ($FORM{$state} == 'NJ') {$state = 'NJ'; $statename = 'New Jersey'};
if ($FORM{$state} == 'NM') {$state = 'NM'; $statename = 'New Mexico'};
if ($FORM{$state} == 'NY') {$state = 'NY'; $statename = 'New York'};
if ($FORM{$state} == 'NC') {$state = 'NC'; $statename = 'North Carolin
+a'};
if ($FORM{$state} == 'ND') {$state = 'ND'; $statename = 'North Dakota'
+};
if ($FORM{$state} == 'OH') {$state = 'OH'; $statename = 'Ohio'};
if ($FORM{$state} == 'OR') {$state = 'OR'; $statename = 'Oregon'};
if ($FORM{$state} == 'OK') {$state = 'OK'; $statename = 'Oklahoma'};
if ($FORM{$state} == 'PA') {$state = 'PA'; $statename = 'Pennsylvania'
+};
if ($FORM{$state} == 'RI') {$state = 'RI'; $statename = 'Rhode Island'
+};
if ($FORM{$state} == 'SC') {$state = 'SC'; $statename = 'South Carolin
+a'};
if ($FORM{$state} == 'SD') {$state = 'SD'; $statename = 'South Dakota'
+};
if ($FORM{$state} == 'TN') {$state = 'TN'; $statename = 'Tennessee'};
if ($FORM{$state} == 'TX') {$state = 'TX'; $statename = 'Texas'};
if ($FORM{$state} == 'UT') {$state = 'UT'; $statename = 'Utah'};
if ($FORM{$state} == 'VT') {$state = 'VT'; $statename = 'Vermont'};
if ($FORM{$state} == 'VA') {$state = 'VA'; $statename = 'Virginia'};
if ($FORM{$state} == 'WA') {$state = 'WA'; $statename = 'Washington St
+ate'};
if ($FORM{$state} == 'DC') {$state = 'DC'; $statename = 'Washington DC
+'};
if ($FORM{$state} == 'WV') {$state = 'WV'; $statename = 'West Virginia
+'};
if ($FORM{$state} == 'WI') {$state = 'WI'; $statename = 'Wisconsin'};
if ($FORM{$state} == 'WY') {$state = 'WY'; $statename = 'Wyoming'};

## Canada ##################################

if ($FORM{$state} == 'AB') {$state = 'AB'; $statename = 'Alberta'};
if ($FORM{$state} == 'BC') {$state = 'BC'; $statename = 'British Colum
+bia'};
if ($FORM{$state} == 'LB') {$state = 'LB'; $statename = 'Labrador'};
if ($FORM{$state} == 'MB') {$state = 'MB'; $statename = 'Manitoba'};
if ($FORM{$state} == 'NB') {$state = 'NB'; $statename = 'New Brunswick
+'};
if ($FORM{$state} == 'NL') {$state = 'NL'; $statename = 'Newfoundland 
+and Labrador'};
if ($FORM{$state} == 'NS') {$state = 'NS'; $statename = 'Nova Scotia'}
+;
if ($FORM{$state} == 'NT') {$state = 'NT'; $statename = 'Northwest Ter
+ritories'};
if ($FORM{$state} == 'NU') {$state = 'NU'; $statename = 'Nunavut'};
if ($FORM{$state} == 'PE') {$state = 'PE'; $statename = 'Prince Edward
+ Island'};
if ($FORM{$state} == 'ON') {$state = 'ON'; $statename = 'Ontario'};
if ($FORM{$state} == 'QC') {$state = 'QC'; $statename = 'Quebec'};
if ($FORM{$state} == 'SA') {$state = 'SA'; $statename = 'Saskatchewan'
+};
if ($FORM{$state} == 'YU') {$state = 'YU'; $statename = 'Yukon Territo
+ry'};




##Start database connections##########################################
+##########
$database = "database";
$db_server = "x";
$user = "x";
$password = "x";

##Connect to database, insert statement, & disconnect ################
+##########
$dbh = DBI->connect("DBI:mysql:$database:$db_server", $user, $password
+);

$statement = "SELECT DISTINCT city FROM database WHERE state='$state' 
+ORDER BY city";


$sth = $dbh->prepare($statement) or die "Couldn't prepare the query: "
+.$sth->errstr;

$rv = $sth->execute or die "Couldn't execute query: ".$dbh->errstr;


######################################################################
+##########

[download]

Considered by Fletch: retitle (maybe "problem with multiple if statements") as has nothing to do with tainting
Unconsidered by castaway: Keep/Edit/Delete: 11/40/1 - untainting is in fact whats happening (IIRC), and since no editor has actually retitled..

Replies are listed 'Best First'.
Re: Untaint variables not working, IF statements.
by Sidhekin (Priest) on Jun 28, 2004 at 19:05 UTC

    It looks like the last IF statement is the only one that executes and I can't figure out why.

    Nope. All of the if-statements are executing, and since you are testing for numerical equivalence of two strings contain no numbers, they will all be satisfied too.

    I have not read all that code, but a first requirement to make it work would be to use eq instead of ==. But good grief, there must be a better way to untaint that data ...

    Update: Something along these lines, perhaps:

    # Assuming a %statename hash has been set up:
if ($FORM{$state} =~ /^(\w\w)$/ and $statename{$FORM{$state}}) {
  $state = $1;
  $statename = $statename{$state};
}

    [download]

    print "Just another Perl ${\(trickster and hacker)},"
    The Sidhekin proves Sidhe did it!

[reply]
[d/l]
[select]
Re: Untaint variables not working, IF statements.
by derby (Abbot) on Jun 28, 2004 at 19:09 UTC
    Well ... besides the classic critique of use CGI and a look-up table will be easier on the eyes in the longrun ... I think you need to look at perlop and the difference between == and eq.

    -derby
[reply]
Reinventing CPAN wheels
by Fletch (Bishop) on Jun 28, 2004 at 19:39 UTC
[reply]
Re: Untaint variables not working, IF statements.
by Enlil (Parson) on Jun 28, 2004 at 19:47 UTC
    It seems you are using CGI but then parsing the params yourself. Why not just use module. Another thing you might want to do is use if/elsif/else construct that way you don't keep comparing once a match is found. As others have mentioned the main problem is the difference between '==' and 'eq' and what they compare (numerical vs. text values). Lastly, to have perl help you check that everything that needs to be untainted is in fact untainted add a -T after your she-bang line. Anyhow, here is another way to accomplish what your doing (using hashes vs. many if statements, plus the -T) 
    #!/usr/local/bin/perl -T
use strict;
use warnings;
use DBI;
use CGI;

my $cgi = new CGI;
my %states = (
 'AL' => 'Alabama', 'AK' => 'Alaska', 'AZ' => 'Arizona',
 'AR' => 'Arkansas', 'CA' => 'California', 'CO' => 'Colorado',
 'CT' => 'Connecticut', 'DE' => 'Delaware', 'FL' => 'Florida',
 'GA' => 'Georgia', 'HI' => 'Hawaii', 'ID' => 'Idaho',
 'IN' => 'Indiana', 'IL' => 'Illinois', 'IA' => 'Iowa',
 'KS' => 'Kansas', 'KY' => 'Kentucky', 'LA' => 'Louisiana',
 'ME' => 'Maine', 'MD' => 'Maryland', 'MA' => 'Massachusetts',
 'MI' => 'Michigan', 'MN' => 'Minnesota', 'MO' => 'Missouri',
 'MS' => 'Mississippi', 'MT' => 'Montana', 'NE' => 'Nebraska',
 'NV' => 'Nevada', 'NH' => 'New Hampshire', 'NJ' => 'New Jersey',
 'NM' => 'New Mexico', 'NY' => 'New York', 'NC' => 'North Carolina',
 'ND' => 'North Dakota', 'OH' => 'Ohio', 'OR' => 'Oregon',
 'OK' => 'Oklahoma', 'PA' => 'Pennsylvania', 'RI' => 'Rhode Island',
 'SC' => 'South Carolina', 'SD' => 'South Dakota', 
 'TN' => 'Tennessee', 'TX' => 'Texas', 'UT' => 'Utah', 
 'VT' => 'Vermont', 'VA' => 'Virginia',
 'WA' => 'Washington State', 'DC' => 'Washington DC',
 'WV' => 'West Virginia', 'WI' => 'Wisconsin', 'WY' => 'Wyoming',
 
  ## Canada##

 'AB' => 'Alberta', 'BC' => 'British Columbia', 'LB' => 'Labrador',
 'MB' => 'Manitoba', 'NB' => 'New Brunswick',
 'NL' => 'Newfoundland and Labrador', 'NS' => 'Nova Scotia',
 'NT' => 'Northwest Territories', 'NU' => 'Nunavut',
 'PE' => 'Prince Edward Island', 'ON' => 'Ontario',
 'QC' => 'Quebec', 'SA' => 'Saskatchewan',
 'YU' => 'Yukon Territory', 
);

my ($state) = $cgi->param('state') =~ /([A-Z]{2})/;
unless ( defined $state and exists $states{$state} ) {
  die "State not found";
}
my $state_name = $states{$state};

############ DB STUFF
$database = "database";
$db_server = "x";
$user = "x";
$passwd = "x";

##Connect to database, insert statement, & disconnect 
$dbh = DBI->connect("DBI:mysql:$database:$db_server", $user, $passwd);

$statement = "SELECT DISTINCT city FROM database WHERE state = ? ORDER
+ BY city";


$sth = $dbh->prepare($statement) 
    or die "Couldn't prepare the query: ".$sth->errstr;

$rv = $sth->execute 
    or die "Couldn't execute query: ".$dbh->errstr;


################################################

    [download]
    HTH

    -enlil

[reply]
[d/l]
      Cool, thanks guys. This is my first program of any substance. You guys have been a big help.

      Thanks again,
      Adam

[reply]
      I don't understand what the '?' if for in the following statement: "SELECT DISTINCT city FROM database WHERE state = ? ORDER + BY city"

      Why wouldn't you just use '$state'?

      Thanks
      Adam

[reply]
Re: Untaint variables not working, IF statements.
by Happy-the-monk (Canon) on Jun 28, 2004 at 19:11 UTC

    I can't figure out why.

    You compare strings numerically with   ==   - they default to TRUE or 1 in a numerical sense and thus every comparison is successful and the following block is executed.

    To fix that problem, use the   eq   operator to compare the strings as strings. Read more about this in   perldoc perlintro   and   perldoc perlop.

    While you're at it you can fix the broken HTTP-GET/Querystring parser and use   CGI.pm   - the following code will do that:

    use CGI;
my $q = CGI::->new;
my %FORM = $q->Vars;

    [download]

    ...instead of the part after the last use-statement and before the "Untaint Variables"-part.

    Cheers, Sören

[reply]
[d/l]
Re: Untaint variables not working, IF statements.
by sgifford (Prior) on Jun 28, 2004 at 19:51 UTC

    In addition to the other comments, I think when you're getting the state setting from the form, you mean to say $FORM{state}; you currently say $FORM{$state}, and since $state isn't defined yet, you're looking at $FORM{''}. Using warnings would have told you that, BTW.

    And I suspect you don't know how to use a lookup table, since this is the perfect application for one. Essentially you would say: 

    %states = ( AL => ['AL', 'Alabama'],
            AK => ['AK', 'Alaska'],
...
            SA => ['SA', 'Saskatchewan'],
            YU => ['YU', 'Yukon Territory'],
);
if (my $st = $states{$FORM{state}})
{
  ($state,$statename)=@$st;
}
else
{
  die "Invalid state!"
}

    [download]
    As you can see, the code is much cleaner and easier to maintain, and will also be marginally faster in most cases.
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom