looping through hash with regex.

[bazz]

Is this the best way to clean out potential db-breaking stuff?


my $query = CGI->new;
my %params = $query->Vars;

# loop thru hash and remove dodgey chars
while (my ($key, $value) = each(%params)){
if ($key =~ /^\;+$/)
{
print qq( dodgey input);
$key =~ s/;//g;
}

if ($value =~ /^\;+$/)
{
print qq( dodgey input);
$value =~ s/;//g;
}
}



bazz

[derzok]

What database module are you using? When using DBI I always use the quote function from the database handler (not statement handler). However it's usually best to just use prepared statements so that you don't have to worry about injection at all.

http://search.cpan.org/dist/DBI/DBI.pm#quote

http://search.cpan.org/dist/DBI/DBI.pm#prepare

[bazz]

Thanks, I'll read up on those but I am already confused.

How would a prepared statement protect me from injection, when the data is being inputted by the user and needs to be cleaned/tested for cleanliness?

bazz

[derzok]

use strict;
use DBI;
use warnings;

my $dsn = "DBI:mysql:database=dbname;host=localhost;port=3306";
my $dbh = DBI->connect($dsn,'username','userpass');
$dbh->{'mysql_auto_reconnect'} = 1;
my $query = "INSERT INTO `table` (date,col2,col3,col4) VALUES (?,?,?,?)";
my $sth = $dbh->prepare($query);
$sth->execute(time(),$unescaped,$also_unescaped,'butts',$dangerous_vars);


I've got a lot of examples of SQL interfacing in an IRC bot that I wrote, feel free to borrow or steal any code/ideas from it: http://svn.zoklet.net/zb3/trunk/ Also have a look at the cpan docs here: http://search.cpan.org/~timb/DBI/DBI.pm (ctrl+f on that page can find you just about anything you're looking for)

[FishMonger]

Is this the best way to clean out potential db-breaking stuff?


my $query = CGI->new;
my %params = $query->Vars;

# loop thru hash and remove dodgey chars
while (my ($key, $value) = each(%params)){
if ($key =~ /^\;+$/)
{
print qq( dodgey input);
$key =~ s/;//g;
}

if ($value =~ /^\;+$/)
{
print qq( dodgey input);
$value =~ s/;//g;
}
}



bazz

This test if ($key =~ /^\;+$/) doesn't make any sense. $key is the value you defined for the name attribute of your input fields. Why would any of those be nothing more than 1 or more semi colons?


$value =~ s/;//g; would be more efficiently written as $value =~ tr/;//;

foreach my $key ( keys %params ){
if ($params{$key} =~ s/^\;+$//) {
print qq( dodgey input);
}
}

[bazz]

I have amended the regex.

Can you please explain whether two foreach loops is better than 1 while loop.

I need to clean both keys and values.


my $query = CGI->new;
my %params = $query->Vars;

# loop thru hash and remove dodgey chars
while (my ($key, $value) = each(%params))
{
#$key="Flowers; delete";
#$value = "bouquet; delete";
unless ($key =~ /^[-\s\w0-9]+$/)
{
print qq( dodgey key);
}

unless ($value =~ /^[-\s\w0-9]+$/) # permit only - || space || letters and _ || 0-9
{
print qq( dodgey value);
}
}

[FishMonger]

A single foreach (or while loop) with 2 conditional tests is all that's needed.

You'd also want to do the DB insertion/queries within the loop, especially if you use my ($key, $value) = each(%params), otherwise your hash would remain unchanged and would still contain the tainted data.

If $key contains unwanted characters, then clearly the user is attempting to inject something, so the submission should be immediately rejected (perform a redirect) at the first indication. This is probably a matter of personal preference on style, but I'd test the $key by using a "valid_keys" hash and if the submitted key is in the hard coded "valid_keys" hash, then it's good.

If $value contains tainted data, you have 4 options.

Perform a redirect
Capture the first portion of the data that is valid (up to the invalid character) and ignore everything else
Strip out all invalid characters and retain the rest
Escape the data when performing the DB calls


my $query = CGI->new;
my %params = $query->Vars;
my %valid_keys = (key1 => 1,
key2 => 1,
);

# loop thru hash and remove dodgey chars
foreach my $key ( keys %params ){

if ( not exists $valid_keys{$key} ) {
$query->redirect("http://mysite.com/rejection.html");
}

# this test would be modified to suit one of the options I mentioned
# note that I removed the unneeded 0-9 from the regex (\w includes those numbers)
unless ($params{$key} =~ /^[-\s\w]+$/) { # permit only - || space || letters and _ || 0-9
print qq( dodgey value);
}
}

[bazz]

Thank you FishMonger. :thumbsup:

Not only have you cleared this up for me but, I think you have explained the answer to my next question, which I asked in another thread.

I think I'll be able to make some progress with this now.


bazz