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

Fellow Monks,

I recently completed my first Perl program. I have wrote a few minor scripts before, but this is my first full fledge program. I would appreciate constructive criticism on this program along with any suggestions.

Thanks,
Dru 
#!/usr/bin/perl -w

###################################################################
#
# fwanalysis - Program that uses fwlogsum's daily summary report to
# parse the exported log file for the top 15 dropped ip's
# and generate our standard Flagged Activity email for each
# ip with a sample of 15 entries from the fw log file 
# as "evidence". An email will be sent to the sysadmin
# of each network and to abuse@theirdomain.com. The email 
# address is obtained via a whois query. 
# IS Security will also receive a cc: of each email sent 
# out.
#
##################################################################
 
use strict;
 
# Assign Variables 
 
my $today=`date +%d%b%y`;
chomp($today);
my $reportfile = "/exported/analysis/$today.logsum";
my $topten = 0;
my $logfile = "/exported/$today.elog";
my @ips;
my $ip;
my $from_addr="IS Security<IS.Security\@mydomain.com>";
my $root="Root<root\@mydomain.com>";
 
my $arin="/usr/bin/whois -h whois.arin.net";
my $ripe="/usr/bin/whois -h whois.ripe.net";
my $apnic="/usr/bin/whois -h whois.apnic.net";
my $krnic="/usr/bin/whois -h whois.krnic.net";
 
my $email;
my $domain;
my $result;
 
 
# Get the ten ip addresses from the daily fwlogsum report and 
# store them in an anonymous array @ips

open REPORT, "$reportfile" or die "Can't open FWLOGSUM File: $!\n";
while (<REPORT>){
  chomp;
  $topten = 1 if m!^Users/Source Addresses!;
  next unless $topten;
  push @ips, [split /\s+/] if /^(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})/;
}
close REPORT;
 
# Use the evidence and whois subroutines for the first ip 
 
my $ip1 = $ips[0][0];
my $times = $ips[0][1];
whois($ip1);
evidence($ip1,$times);
 
 
# Second ip
 
my $ip2 = $ips[1][0];
my $times2 = $ips[1][1];
whois($ip2);
evidence($ip2,$times2);
 
# Third ip
 
my $ip3 = $ips[2][0];
my $times3 = $ips[2][1];
whois($ip3);
evidence($ip3,$times3);
 
# Fourth ip
 
my $ip4 = $ips[3][0];
my $times4 = $ips[3][1];
whois($ip4);
evidence($ip4,$times4);

# Fifth ip 
 
my $ip5 = $ips[4][0];
my $times5 = $ips[4][1];
whois($ip5); 
evidence($ip5,$times5);

# Repeats for 10 more ip's


############################################
#             SUBROUTINES                  #
############################################
 
 
#####################################################
# evidence: Open up the exported log file, search for 
#           the ip, add to array, split array into 15 
#           lines,replace firewall ip addresses, send 
#           email                                     
#####################################################
 
sub evidence {
  my $ip = $_[0];
  my $times = $_[1]; 
  my @fwlog;
  open LOG, "$logfile" or die "Can't open $logfile: $!\n";
  while (<LOG>){
  if ($_ =~ $ip){
      push (@fwlog, $_);
 } #end if
} #end while
close LOG;
 splice(@fwlog, 15);
 foreach (@fwlog){  
 s/192\.168\.14\.2/x.x.x.x/;
 s/192\.168\.2\.2/x.x.x.x/; 
 s/192\.37\.222\.46/x.x.x.x/; 
 s/192\.215\.150\.2/x.x.x.x/;
 } #end foreach
 
open(SENDMAIL, "|/usr/lib/sendmail -oi -t")
                      or die "Can't fork for sendmail: $!\n";
print SENDMAIL <<EOM;
From: $from_addr
To: $email$domain, abuse$domain
CC: $from_addr
Subject: Flagged Activity from $ip
System Administrator,
 
Cease or decease email.
 
Here are the details:
 
Source IP Address: $ip
Number of times: $times
 
Sample from the firewall logs (*note date/time are Eastern Daylight Ti
+me -4 GMT):
 
Explanation of Log Fields: (Line Number ; Date; Time; N/A; Type<log,al
+ert>; Action; N/A; Direction; Protocol; Source IP; Destination IP; De
+sti
nation Port; Source Port; N/A; N/A)
 
@fwlog
 
Thank You,
Security Team
My Company

EOM
close(SENDMAIL) or warn "sendmail didn't close nicely";
 
} #close evidence sub
 
####################################################
# whois: Obtain an email address from a whois query 
####################################################
 
sub whois{
 
my $ip = $_[0];
$result =  `$arin $ip`;
 
# If the whois matches a ripe address, then perform whois against 
# RIPE database and match email address.
 
if ($result =~ m/European Regional Internet Registry/){
   $result = `$ripe $ip`;
 if (match()){
 } #close if match
} #close if result
 
# If the whois matches a apnic address, then perform whois against 
# apnic database and match single email address. If the apnic whois 
# matches a krnic address, then perform whois against the krnic databa
+se.
 
elsif ($result =~ m/Asia Pacific Network Information Center/){
    $result = `$apnic $ip`;
    if ($result =~ m/Allocated to KRNIC Member/){
    $result = `$krnic $ip`;
   } #end KRNIC match
    if (match()){
 } #end if match
} #end elsif
 
# If the whois returns 2 handles, then perform whois against 2nd handl
+e and 
# match email address.
 
elsif (handles($arin)){
  if (match()) { 
 } #end if match
} #end elsif
 
# If whois does not return a emailbox, send email to IS Security notif
+ying them
 
elseif ($result =~ m/No mailbox/) {
  open(SENDMAIL, "|/usr/lib/sendmail -oi -t")
      or die "Can't fork for sendmail: $!\n";
print SENDMAIL <<EOM;
From: $root
To: $from_addr 
Subject: Can't perform a whois query for the ip $ip
EOM
close(SENDMAIL) or warn "sendmail didn't close nicely";
  exit 
 
 } #end else 
} #close whois sub
 
# If the whois returns just a normal arin response, then match the ema
+il.
 
elsif (($email, $domain) = $result =~ m/([-.\w]+)(\@[-.\w]+)/) {
     print "The 1st email address is: $email$domain\n";
     print "Email will also be sent to: abuse$domain\n";
} #end elsif
 
 
###################################################
# match: Obtain an email address from a whois query 
###################################################
 
sub match {
  ($email, $domain) = $result =~ m/([-.\w]+)(\@[-.\w]+)/; 
} #end sub match
 
#################################################
# handles: Obtain the handles from a whois query 
#################################################
 
sub handles {
  my $registry = $_[0];
  $result =~ m/xxx/;
  my @handle = $result =~ m/\((.*?)\)/g;
    $result = `$registry $handle[1]`;
} #end handles

[download]

Replies are listed 'Best First'.
Re: Constructive Criticism for My First Perl Program
by wog (Curate) on Sep 13, 2001 at 02:28 UTC 
    =-> perl -c fwanalysis
elseif should be elsif at fwanalysis line 197.
syntax error at fwanalysis line 197, near ") {"
syntax error at fwanalysis line 209, near "}"
fwanalysis had compilation errors.

    [download]

    You should fix that problem. And the other compliation error that will appear after you fix that one. Please, test your code before showing it to us.

    That said, don't make external calls to date like that, try Date::Format, or strftime from the POSIX module (use POSIX qw(strftime); my $today = strftime('%d%b%y',localtime);). Doing that should make your code more portable.

    Also, Mail::Send is probably a better thing to use then sendmail directly (since some systems don't have sendmail, of course.) That may also make your code more portable.

    I would also strongly reccommend you indent your subroutines consistently. (Personally I prefer that all subroutines be indented, but I can see a good reason to not do that, in some cases.)

    update: Looking at your code with more detail I see this: 

    
my $ip1 = $ips[0][0];
my $times = $ips[0][1];
whois($ip1);
evidence($ip1,$times);
 
 
# Second ip
 
my $ip2 = $ips[1][0];
my $times2 = $ips[1][1];
whois($ip2);
evidence($ip2,$times2);
 
# Third ip
 
my $ip3 = $ips[2][0];
my $times3 = $ips[2][1];
whois($ip3);
evidence($ip3,$times3);
 
# ...

    [download]

    This is MUCH better written as a loop. For example: 

    foreach my $i (0..14) {
  my($ip,$times) = @{$ip[$i]};
  whois($ip);
  evidence($ip,$times);
}

    [download]

    And that's still terribly written, too. You should try to make your subroutines be independent of each other -- avoid global variables.

    update2: (... pass things as arguments to subroutines instead, or use OO if that is more appropriate.) Another potential problem I see is this: 

      if ($_ =~ $ip){

    [download]

    The problem with this is that regex metacharacters will be interperated in the IP address, so the .s in the IP address will match any character. For example, if the IP address were 1.2.4.8 then lines of the logfile for the IP address 162.4.83.1 would match that. You might be better of using something like $_ =~ /\Q$ip\E/, or index($_, $ip) != -1.

    On this issue of your regex for extracting the domain, etc from the email, note that there are many valid emails that your regex won't match (search for a node on e-mail validation to see why). Consider using Mail::Address instead of a regex.

[reply]
[d/l]
[select]
      wog,

      Thanks for all your useful suggestions. The elseif statement was a goof on my part. I originally tested the code without this statement, but added it afterwards to catch emails with no email addresses and didn't test it since. Of course it failed along with a few other things that I found wrong with my code.

      I was thinking about adding a foreach loop also, I've just been spending so much time trying to get the whois working. I think I will use Mail::Send instead of sendmail. The reason I use sendmail is because the perldoc FAQ recommends it: perldoc -q "send mail" but I figure this was written before Mail::Send because they also recommend Mail::Mailer.

      note that there are many valid emails that your regex won't match (search for a node on e-mail validation to see why).

      I already inquired about the email problem Matching any Email Address. I know this regex would not match all email addresses, but it has matched everyone for me so far.

      Again, thanks for the help and I plan on implementing most if not all of your recommendations.

      -Dru
[reply]

Back to Seekers of Perl Wisdom