http://qs1969.pair.com?node_id=58477

SilverB1rd has asked for the wisdom of the Perl Monks concerning the following question:

I would like to thank you for all the help.
I have tryed to implement the security stuff you guys talked about, some of it I could not get to work or find the documentation, So here is my new script! Any major holes in this one?
#!/usr/bin/perl -Tw #warnings and taint mode now enabled use CGI; use strict; $CGI::POST_MAX=1024 * 500; # max 500k post $CGI::DISABLE_UPLOADS = 1; # No uploads print "Content-type:text/html\n\n"; my $temp; $temp = "$ENV{'QUERY_STRING'}"; $temp =~ tr/\/A-Za-z0-9_.-//dc; $temp =~ s/\.+\///g; if( $temp =~ m#(^.+\.{1}?\w+)# ) { $temp= "../www$1"; } else { dienice "Invalid template file name..."; } #you may now use the CGI methods. my ($query) = new CGI; my (@values, $key, $i, @fary); foreach $key ($query->param) { $i = $query->param($key); if ($key =~ /required/i) {if ( ($i eq "") && ($i == "") ) {failure();} +} } open(INF, "< $temp") or dienice("Cant open $temp"); seek(INF,0,0); @fary = <INF>; close (INF); foreach $key ($query->param) { @values = $query->param($key); foreach $i (@fary) {$i =~ s/\[$key\]/@values/g;} } my $mailprog = '/usr/lib/sendmail'; open (MAIL, "|$mailprog -t") or dienice("Can't access $mailprog!\n"); foreach $i (@fary) { print MAIL "$i"; } close (MAIL); my $url = $query->param('success'); print "<Meta HTTP-EQUIV=\"Refresh\" CONTENT=\"0;URL=$url\">\n\n"; print "<a href=\"$url\">If you are not forwarded in 5 seconds, please +click here.<\/a>"; exit; sub failure { my $url = $query->param('failure'); print "<Meta HTTP-EQUIV=\"Refresh\" CONTENT=\"0;URL=$url\">\n\n"; print "<a href=\"$url\">If you are not forwarded in 5 seconds, please +click here.<\/a>"; exit; } sub dienice { my($errmsg) = @_; my($webmaster) = 'webmaster@mydomain.com'; print <<Eof; <html><head><title>Error!!</title></head><body>The error was $errmsg</ +body></html> Eof exit; }

Replies are listed 'Best First'.
Re: Second hack at Secure Mailer
by MeowChow (Vicar) on Feb 15, 2001 at 03:49 UTC
    Well, the good news is that you use CGI.pm. The bad news is that you don't use it.

    You shouldn't scrub the query string directly; let CGI.pm deal with that. In fact, your initial tr/// of the query string completely breaks most requests. Moreover, if you were to add needed characters like [%&?] to your regex, it still would be unsafe, because of the unescaping performed by CGI.pm. Now, onto the following line:

    $temp =~ s/\.+\///g;

    Remember what you've been told regarding code checks for specific hacks. It's never something you should do, and that is what you are doing here. Consider how this expression would perform if given "..\..\" instead of "../../" on an NT platform- you can't possibly check for all the sploits, so don't even try. It would only serve to make your code more complex, buggy, and insecure. Now, moving on to this comment:

    #you may now use the CGI methods.
    You can always use the CGI.pm methods. CGI.pm is quite secure, and neeedn't launder any raw request data before it gets to see it. Launder the data that you get back from CGI.pm.

    Also, as a matter of style, you should consider using CGI.pm's functions like print $query->header for some of your code. There's really no need to type alot that stuff out.

       MeowChow                                   
                   s aamecha.s a..a\u$&owag.print
Re: Second hack at Secure Mailer
by saucepan (Scribe) on Feb 15, 2001 at 03:56 UTC
    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!

      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 cgi.pm is that it did not work it kept giving me a script error on the server. I use the CGI.pm 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.
        Are you doing:
        print $query->header;
        or
        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.
           MeowChow                                   
                       s aamecha.s a..a\u$&owag.print
        Ok, this problem is that if you POST and have a query string, by default CGI.pm 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 CGI.pm file and uncomment the next line of code.

        $code or die
        Using perl at
        The Spiders Web
Re: Second hack at Secure Mailer
by Masem (Monsignor) on Feb 15, 2001 at 03:38 UTC
    The only thing that gets me is the starting point, namely this template file. The fact that you're getting the name of the file directly from the CGI QUERY_STRING, even with all the various s/// operators in there, can still *possibly* leave you open for the "; rm -rf /" attacks. You may have that fixed, but for some reason that concerns me.

    Here's my suggestion: Since this appears to be selecting a template file from a limited number of choices, how about creating a hash that maps a template keyword to the actual filename that you use, so that the open call will only see a filename that *you* specify and doesn't at all come from the CGI query. The only major check you'll need is to add a default option if the template keyword parameter is not in your hash, but this is trivial. eg:

    my %template_hash = ( default => "www/default.tmp", detailed => "www/detail.tmp", brief => "www/brief.tmp" ); # $temp still gotten as before, could also be cgi->param my $template_file = ( defined $template_hash{ $temp } ) ? $template_hash{ $temp } : $template_hash{ 'default' }; # Continue on as above.