perlmunky wrote:
On Jan 11, 2008 6:47 PM, Chris Charley <[EMAIL PROTECTED]> wrote:
See this colimn by Randal L. Schwartz. It descibes injection attacks.
http://www.stonehenge.com/merlyn/UnixReview/col58.html
Thanks, I read the page, alongside the wikipedia page and I think I
understand what an sql injection is. My code follows. The first section is
for a text suggestion drop-down menu - it produces a selection based on the
current user input in a text box and runs a DBI query to get suggestions for
a user. The details are passed by a javascript routine to catalyst.
sub _build_suggestion_list {
my ($self, $c) = @_;
my $table_and_col_name = $c->req->params->{column_name};
my $current_text = $c->req->params->{text_input};
my @well_column_names = @{_get_column_names($c,"T1")};
my @gi_column_names = @{_get_column_names($c,"T2")};
my %genuine_columns = ();
for (@gi_column_names) { $genuine_columns{${$_}[0]} = 1; }
for (@well_column_names) { $genuine_columns{${$_}[0]} = 1; }
Why copy two the two arrays when you can just use the references
directly? The dereferencing syntax is usually $_->[0] not ${$_}[0].
You can remove a lot of the duplication by doing it like this:
my %genuine_columns = map { $_->[0] => 1 }
map @{ _get_column_names( $c, $_ ) },
qw/T1 T2/;
#I dont think I need the {1,}
if ($current_text =~ /\:{1,}|\--{1,}|\#{1,}|\;|insert|drop|create|^$/ig)
The characters ':', '-', '#' and ';' are not special in regular
expressions so they don't have to be escaped. The quantifier '{1,}'
could be written more simply as '+'. The pattern is evaluated in
boolean context so the /g option is extraneous.
{
my $tc = "<ul><li>Invalid Input (: ; --; #)</li></ul>";
$c->res->body($tc);
} else {
if ( exists $genuine_columns{$table_and_col_name}) {
my ($table, $column) = split /\./, $table_and_col_name;
my $model = $c->model(q(DB));
my $dbh = $model->storage->dbh;
my $select = ("select distinct $table\.$column from $table where
$table\.$column LIKE \'\%$current_text\%\'");
The characters ".", "'" and "%" are not special in a double quoted
string so they don't have to be escaped.
my $sql_exe = $dbh->prepare($select); $sql_exe->execute();
my $results = $sql_exe->fetchall_arrayref();
my $options = "<ul>";
for (@$results) {
$options .= "<li>${$_}[0]</li>";
${$_}[0] is usually written as $_->[0].
}
$options .= "</ul>";
$c->res->body($options);
} else {
my $tc = "<ul><li>The column names are invalid - please see
admin</li></ul>";
$c->res->body($tc);
}
}
}
The next section contains the query constructor. This takes all the user
input from multiple rows (users can create as many as they want). I have
limited the options but this, i think, doesn't help because someone could
craft their own post and submit that. The query is run in another subroutine
which returns the data to the template toolkit end of things.
sub _query : Local {
my ( $self, $c ) = @_;
my $href_params = ($c->request->parameters());
delete $$href_params{'button'};
$$href_params{'button'} is usually written as $href_params->{button}
my @well_column_names = @{_get_column_names($c,"T1")};
my @gi_column_names = @{_get_column_names($c,"T2")};
my %genuine_columns = ();
for (@gi_column_names) { $genuine_columns{${$_}[0]} = 1; }
for (@well_column_names) { $genuine_columns{${$_}[0]} = 1; }
Same as with the other subroutine:
my %genuine_columns = map { $_->[0] => 1 }
map @{ _get_column_names( $c, $_ ) },
qw/T1 T2/;
#Are the columns the ones that they should be?
@{$$href_params{'select_these'}} = grep { $genuine_columns{$_} }
@{$$href_params{'select_these'}};
@{$$href_params{'select_these'}} is usually written as @{
$href_params->{ select_these } }
[ The same applies to every other instance where this occurs. ]
my $select = "";
if ( ref($$href_params{'select_these'}) =~ 'ARRAY' or
ref($$href_params{'select_these'}) =~ 'REF') {
You are using '=~' when you should be using 'eq'.
if ( ref( $href_params->{ select_these } ) eq 'ARRAY' or ref(
$href_params{ select_these } ) eq 'REF' ) {
if ( @{$$href_params{'select_these'}} == 0 ) { $select = " T2.y ";}
else { $select = join(" ,", @{$$href_params{'select_these'}}); }
} else {
if ($$href_params{'select_these'} eq "") { $select = "T1.x"; }
else {
if ( exists $genuine_columns{$$href_params{'select_these'}}) {
$select = $$href_params{'select_these'};
} else {
$select = "T1.x";
}
}
}
#Two blocks - one for where there are multiple user conditions and the other
for singles
my $conditional = "";
if ( ref($$href_params{'conditional_type'}) =~ 'ARRAY' or
ref($$href_params{'conditional_type'}) =~ 'REF') {
You are using '=~' when you should be using 'eq'.
for (my $i = 0; $i < @{$$href_params{'conditional_type'}}; $i++) {
That is usually written as:
for my $i ( 0 .. $#{ $href_params->{ conditional_type } } ) {
$conditional .= ${$$href_params{'conditional_type'}}[$i] .
' ';
$conditional .= ${$$href_params{'select_column_name'}}[$i] .
' ';
#Check for foul play
if (${$$href_params{'user_text'}}[$i] =~
/\;|:|--|#|insert|drop|create|\'|\"|null/ig ) {
The characters ';', "'" and '"' are not special in regular expressions
so they don't have to be escaped. The pattern is evaluated in boolean
context so the /g option is extraneous.
${$$href_params{'user_text'}}[$i] = "null"; #This can't
be 'null'
}
if ( ${$$href_params{'case_option'}}[$i] =~ /^like$/ig ) {
The pattern is evaluated in boolean context so the /g option is
extraneous. Since the pattern is anchored at beginning and end you
should probably use 'eq' instead:
if ( 'like' eq lc $href_params->{ case_option }[ $i ] ) {
$conditional .= "${$$href_params{'case_option'}}[$i]
'\%${$$href_params{'user_text'}}[$i]\%'" . ' ';
The '%' character is not special in a double quoted string.
${$$href_params{'user_text'}}[$i] is usually written as
$href_params->{user_text}[$i]
} else {
$conditional .=
${$$href_params{'case_option'}}[$i] . ' ';
$conditional .=
${$$href_params{'user_text'}}[$i] . ' ';
}
}
} else {
if ($$href_params{'user_text'} =~
/\;|:|--|#|insert|drop|create|\'|\"|null/ig ) {
The characters ';', "'" and '"' are not special in regular expressions
so they don't have to be escaped. The pattern is evaluated in boolean
context so the /g option is extraneous.
$$href_params{'user_text'} = "null";
}
if ($$href_params{'case_option'} =~ /like/ig) {
Use 'eq' instead:
if ( 'like' eq lc $href_params->{ case_option } ) {
$$href_params{'user_text'} =
"'\%$$href_params{'user_text'}\%' ";
}
$conditional = $$href_params{'conditional_type'} . " " .
$$href_params{'select_column_name'} . " " . $$href_params{'case_option'} . "
" . $$href_params{'user_text'};
You could use a hash slice to simplify that:
$conditional = "@{ $href_params }{ qw/conditional_type
select_column_name case_option user_text/ }";
}
_construct_query($self, $c, \$select, \$conditional);
Do you really need to pass references to scalars?
#pass to Catalyst stash + template
}
John
--
Perl isn't a toolbox, but a small machine shop where you
can special-order certain sorts of tools at low cost and
in short order. -- Larry Wall
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
http://learn.perl.org/
