I'll take a crack at reviewing your code for you.
The m#(^.+\.{1}?\w+)# regexp
confuses me, although it's possible that you're using
a trick I'm unfamiliar with. You've got a literal dot
followed
by {1} and a question mark. Do you want exactly one
dot in the filename, or do you want to allow an optional
one? If the latter you can just omit the {1}, and if
the former you can omit both quantifiers.
Here you are giving the attacker control over which
fields are required to have a value:
foreach $key ($query->param) {
$i = $query->param($key);
if ($key =~ /required/i) {if ( ($i eq "") && ($i == "") ) {failure()
+;}}
}
That may be fine for your purposes, just as long as you are
aware of the issue. (update: of course, if they
change the name of the variable, the template expansion
will fail later on. Doh! Never mind this one. :)
Uh oh.. in this code you pass user-supplied data into a
regexp:
foreach $key ($query->param) {
@values = $query->param($key);
foreach $i (@fary) {$i =~ s/\[$key\]/@values/g;}
}
$key can contain anything here, including
regexp special characters. This isn't necessarily a
total disaster (look for use re 'eval' in re
if you'd like to turn it into one :) ) but you are exposing
the regex engine to the whims of the attacker. It's easy
to construct input that will cause this script to chew up
RAM and CPU, and it may be possible to construct input that
exploits bugs in the RE engine.
A brute force fix would be to add something like
my $safekey = $key =~ tr/a-zA-Z0-9_//cd;
at the top of the loop, and then use $safekey
instead of $key in the regex.
Also, be careful when passing user-supplied data into
sendmail -t. If you have a template that contains something
like To: [recipient] the attacker can provide
an enormous comma seperated list of email addresses to
spam or mailbomb through your script. (You'd also be relying
on sendmail to Do The Right Thing when confronted with
addresses like
|rm -rf /* which, believe it or not, used to be
a big problem.)
And finally a general suggestion: rather than
filtering all your input for anything that looks like
it might be suspicious,
it may be easier to keep a list of valid options (template
filenames, template variable names) and then just verify
that each selected option matches one which is available.
I see that Masem has kindly provided an example.
Good luck with your project! |