in reply to Second hack at Secure Mailer

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!

Replies are listed 'Best First'.
Re: Re: Second hack at Secure Mailer
by SilverB1rd (Scribe) on Feb 15, 2001 at 04:12 UTC
    Thanks for the help, I would really like to see how you would write it.

    The reson I did not use the header option in the is that it did not work it kept giving me a script error on the server. I use the to process the form data.

    I can not have a static list of templates such as default extra, because this script would be used on a number of different forms.

    Like I said before I'm only trying to making this script work the way the first script worked.
      Ok, this problem is that if you POST and have a query string, by default will only put the form values that are "POSTed" into the param() function, and ignore the query string.

      There is a hack provided by the author that will allow it to process both the query string and the POSTed values together. You can check the CGI docs for more info, but basically search for "cake and eat it" in the file and uncomment the next line of code.

      $code or die
      Using perl at
      The Spiders Web
      Are you doing:
      print $query->header;
      print header;
      If the latter, you will need to change your use CGI into:
      use CGI qw(:standard);
      Or something of that sort, as CGI is not exporting any functions by default.
                     s aamecha.s a..a\u$&owag.print
        I'm was using print $query->header;

        I dont have access to the perl files on my web server. Second I dont think it would pull the information off the commmand line like I need it to. The command line would look like this "/myfolder/mytemplate.txt".