securing code

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

I run a small mailing list for a few friends. A few months ago we encountered a problem whereby one of the recipients companies introduced a mail filter which searches mail for certain words. If it finds those words, it quarantines the message and the staff member gets into some trouble. In order to avoid words like that from slipping through, I wrote the following perl script to manage the list and filter the words that the filter seems to catch. I would like to apologise for anyone that may be offended by some of the contents of this code.

The addresses have been taken out and replaced with fictional ones:

#!/usr/bin/perl
#
# Written by: Marc Silver <marcs@draenor.org>
#
# $Id: filter-mail-authenticate.pl,v 1.25 2001/08/08 08:58:24 marcs Ex
+p $;
#
# #

my( $sendmail ) = "/usr/sbin/sendmail"; #location of sendmail
my( $logfile ) = "/tmp/filter.log"; #location of logfile
my( $myaddress ) = "list\@draenor.org"; #list address
my( $listadmin ) = "marcs\@draenor.org"; #admin address
my( $modify_subject ) = 0; #modify the subject?
my( $approved ) = 0; #this must be 0
my( $log ) = 1; #set to 1 to log

umask 0133;

@real_recipients = ( "user\@draenor.org",
                     "erp\@iafrica.com",
                     "user\@iafrica.com"
                   );

$message = "";

open( LOG, ">> $logfile" ) if( $log );

while( <> ) {
  if( /^From ([-a-zA-Z0-9_.@]+) .+$/  && ! $sender ) {
    $sender = $1;  
    print LOG "Sender found: $sender\n" if( $log );
    next;
  }
  if( /^Subject: (.+)$/ ) {
    $original_subject = $1;
    if( $modify_subject ) {
      print LOG "Subject modified\n" if( $log );
      s/$1/[friends]: $original_subject/;
    }
  } 
  s/fuck/f***/ig;
  s/shit/s***/ig;
  s/shat/s***/ig;
  s/cock/c***/ig;
  s/cunt/c***/ig;
  s/pussy/p****/ig;
  s/whore/wh***/ig;
  s/bitch/b****/ig;
  s/asshole/a**hole/ig;
  s/bastard/b*st*rd/ig;
  s/crap/cr*p/ig;
  s/^\.$/. /;
  $message .= $_;
}

if( $sender ) {
  foreach $recipient( @real_recipients ) {
    if( $recipient eq $sender ) {
      $approved++;
      print LOG "Sender matched: $recipient\n" if( $log );
    }
  }
} else {
  print LOG "ERROR: No sender found\n" if( $log );
  open( MAIL, "|$sendmail $listadmin" );
  print MAIL "No sender found.\n\n";
  print MAIL $message;
  close( MAIL );
  exit 0;
}

if( $approved ) {
  open( MAIL, "|$sendmail @real_recipients" );
  print MAIL $message;
  close( MAIL );
  print LOG "Message [$original_subject] sent\n" if( $log );
} else {
  print LOG "Sender not matched\n" if( $log );
  open( MAILADMIN, "|$sendmail -f $myaddress $listadmin" );
  print MAILADMIN "$sender was rejected sending to the friends list.\n
+\n";
  print MAILADMIN "Message contents:\n\n $message\n";
  close( MAILADMIN );
  open( MAIL, "|$sendmail -f $myaddress $sender" );
  print MAIL "You are not subscribed to this list and may not post to 
+it.\n";
  close( MAIL );
  print LOG "ERROR: Message denied. Admin / Sender notified\n" if( $lo
+g );
}

close( LOG ) if( $log );
[download]

My question is this: is this script secure? Is there a better way to do this while still validating the addresses? Security is my main concern here, so I wanted to run this past those with more knowledge than me. :)

Thanks,
Marc

Edit: chipmunk 2001-08-12

Comment on securing code Download Code

Replies are listed 'Best First'.
Re: securing code by abstracts (Hermit) on Aug 11, 2001 at 22:18 UTC
Hello I would like to mention 2 potential security risks: The substitution in the following segment is dangerous. It causes malformed Subject headers (feed it "`Subject: .`"). While I did not manage to get it to take "`Subject: (?{print "Hello"})`", there may be a way around it to cause potential security hole. `if( /^Subject: (.+)$/ ) { $original_subject = $1; if( $modify_subject ) { print LOG "Subject modified\n" if( $log ); s/$1/[friends]: $original_subject/; } }` [download] Consider using: `s/Subject: /Subject: [friends]: /;` [download] The pipes you open at the end of the script. You're feeding $sender to the shell. `open( MAIL, "\|$sendmail -f $myaddress $sender" );` [download] While I know sender is matched at the beginning with `/^From ([-a-zA-Z0-9_.@]+) .+$/` [download] there are better ways to do this. One of them is using the sender in the From header before printing the message. Hope this helps... Aziz,,, P.S. I suggest removing all the s/f.../f**/ stuff and replacing it with a hash of words=>substitues.	[reply] [d/l] [select]
Re: securing code by damian1301 (Curate) on Aug 11, 2001 at 22:46 UTC
You should always use Taint checking when dealing with CGI scripts or scripts that interact with the server. Plus, its just a good habit to get into anyway. Taint checking doesn't allow any information from outside of the Perl program (parameters, ENV vars, etc) to be used in certain functions or in system calls. Example: This will die under -T: `#!/usr/bin/perl -wT my $paco = shift; # get first argument from @ARGV, or the command line +. unlink $paco;` [download] After running this, even without anything in @ARGV, it will give you an error like: `Insecure dependency in unlink while running with -T switch at test.pl +line 3` [download] Now that you (partially) understand taint checking, always be sure to add warnings as well. You do this by just adding the -w flag to your shebang like so: `#!/usr/bin/perl -wT` [download] Now that script has warnings and taint checking activated. What warnings does is make it easier to debug your program. If you are above 5.006 then you can just add `use warnings;` to your script. Ok, now, another problem with your script is taht you dont `use strict;`. There are many articles on it, so here are some perllib and strict.pm. Another problem with your script is that you don't check to see if your program opened up one of the many files correctly. You also don't check to see if they close correctly. You can help yourself and your program by adding this to your open and close calls: `open(FILE,$file) \|\| die "Could not open $file because: $!"; close(FILE) \|\| die "Could not close $file because: $!";` [download] This way it will check to see if they did what they were supposed to do and save you some problems later. `$_.=($=+(6<<1));print(chr(my$a=$_));$^H=$_+$_;$_=$^H; print chr($_-39); # Easy but its ok.` [download]	[reply] [d/l] [select]