| Subject: | security issues from the p5ee mailing list |
Date: Tue, 26 Nov 2002 16:26:13 -0700
From: Rob Nagler <nagler@bivio.biz>
Subject: Re: implementing a set of queue-processing servers
To: p5ee@perl.org
I looked briefly at UserBase.pm, because it seems to have something to
do with security. I came up with a few questions which weren't easily
resolved. There are probably good answers to all my questions, but
I'm a fairly experienced programmer and my casual observations didn't
find them. I wouldn't find easy answers for Apache either, but I
*trust* Apache from its reputation alone. That's the best I can do,
and that's what I've been arguing about.
Anyway, here's a quick list:
-d $heap->{Dir} || mkdir $heap->{Dir},0755;
Is $heap->{Dir} supposed to be readable by everybody? What is
$head->{Dir}? Will it contain data from the heap on disk? What if
there's a clear text password in the heap?
open FILE,">>$heap->{File}" or
croak qq($heap->{_type} could not open '>>$heap->{File}'.);
This contains a small error: there should always be a space after ">".
unlink "$heap->{Dir}/$href->{user_name}" if $href->{new_user_name};
What if $heap->{Dir} is misconfigured and set to /var/mail? Is POE
running as root?
sub poco_userbase_update {
my $heap = $_[HEAP];
my $protocol = $heap->{Protocol};
my %params = splice @_,ARG0;
for($heap->{Cipher}) {
$_ is set, and it isn't local($_). This is a problem, because other
code gets values. Always use lexically scoped variables. Dynamically
scoped variables are a major source of unexpected behavior. Nit:
Barewords are bad imiho. ARG0 and HEAP should be subroutines or
methods.
my $stm = <<_EOSTM_;
delete from $heap->{Table}
where $heap->{UserColumn} = '$href->{user_name}'
_EOSTM_
$stm .= qq[ and $heap->{DomainColumn} = '$href->{domain}'] if
$href->{domain};
This is naive SQL. What if the user_name or domain has a ' in it?
What if it contains arbitrary code such as:
dontcare' OR user_name like '%
Bad news. Use '?' for all arguments except constants. The result
isn't checked to see how many records were deleted either.