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()
+;}}
}
[download]

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;}
}
[download]

$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!

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.