Insecure dependency in piped open

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

Update: here is the corrected code in its entirety

ps: original post is below this code snippet

#!/usr/bin/perl -Tw

use strict;
## Path set to minimal default
$ENV{PATH} = "/usr/bin:/bin:/usr/sbin";

use warnings;
use subs qw(isHostValid);
use CGI qw( :standard );
use CGI::Carp qw(fatalsToBrowser);

## Capture error message
BEGIN{
    CGI::Carp::set_message(\&carp_error);
}

## Path of nslooku. Change this to the path of your nslookup
my $NSLOOKUP = '/usr/bin/nslookup';
## Path of sendmail. change this to the path of your sendmail
my $SENDMAIL = '/usr/sbin/sendmail';
## Set your email address here
my $RECIP = 'youremail@here.com';


############################################################
#                    ACTION HANDLER                        #
############################################################
#
#
if($ENV{REQUEST_METHOD} eq 'POST'){
        ## Fetch form data input
        my $name_in = param('name');
        my $name = q{};
        my $email_in = param('email');
        my $email = q{};
        my $comments_in = param('comments');
        my $comments = q{};
        
        
        ## Check for html tags in name field
        if($name_in !~ /<.*>/){
                $name_in =~ /(.*)/;
                $name = $1;
        }else{
            die ("oops! you have html tags. naughty, naughty!");
        }
        
        ## Check to see if email is valid. 
        ## Does not match email addresses using an IP address instead 
        ## of a domain name.
        if ($email_in =~ m/\b[a-z0-9._%-]+@[a-z0-9.-]+\.[a-z]{2,4}\b/)
+{
            
            $email_in =~ /(.*)/;
            $email = $1;
        }else{
            die ("oops! your email address is not valid one");
        }
        
        ## Check for html tags in name field;
        if($comments_in !~ /<.*>/ ){
        $comments_in =~ /(.*)/;
        $comments = $1;
        }else{
            die ("oops! you have html tags. naughty, naughty!");
        }
        
        ## Okay! you have passed the tests. now the ultimate test.
        my @result = split(m/@/, $email);
        
        if(!isHostValid($result[1])) {
                die ("Oops! invalid host name");
        }
        
        ## Send form data to your email address
        open (MAIL, "|$SENDMAIL -t");
        print MAIL "To: $RECIP\n";
        print MAIL "Reply: $email\n";
        print MAIL "Subject:email from web form\n";
        print MAIL "\n\n";
        print MAIL "name: ". $name."\n" ;
        print MAIL "emial: ".$email."\n" ;
        print MAIL "comments: ".$comments."\n" ;
        print MAIL "\n\n";
        close (MAIL);

        ## Display confirmation message
        print header;
        print start_html;
        print "Thanks you for using the comment form.
                We are going to get back to you as soon
                as we can say thank you again.";
        print end_html;
}else{
    ## Display form
        print header;
        print start_html;
        print start_form(-method => "post", -action => "");
        print h4("Contact Form");
        print "Name: ", textfield(-name => "name"), br;
        print "E-mail: ", textfield(-name => "email"), br;
        print "Enter your comments:", br;
        print textarea(-name => "comments", -rows => "5", -column => "
+50"), br;
        print submit(-value => "Submit");
        print end_form;
        print end_html;
}

##
# Subroutine checks if the host is valid
#
# @param    host
#
sub isHostValid{
        my $host = shift;
    
      $/='';
    open(my $fh, "-|", $NSLOOKUP, "-type=any", $host)
          or die "unable to exec $NSLOOKUP: $!";
    my @response = <$fh>;
          close $fh;
          $/='\n';
          
          return 1 if (grep /Name:\s+$host/, @response);   
          return 0;
}

##
# Subroutine displays error message
#
# @param     error_message
#
sub carp_error{
    my $error_message = shift();
    
    print start_html("Error") .
                h1("Error") .
                p("Sorry, the following error has occurred: ") .
                p(i($error_message)) .
                end_html;
}
[download]

##############Original Post#######################

elloo,
i need tiny bit of help with security issue in my coding. i've created a simple script for form emailing and i think i've covered most of the security issues however i've one issue i cannot resolve. in the section below i've "insecure dependency in piped open". my question is how can i get around this issue

sub isHostValid{
        my $host = shift;
    my $NSLOOKUP = '/usr/bin/nslookup';
    
      $/='';
    my $fh = new IO::File "$NSLOOKUP -type=any $host 2>&1 |";
    my @response = <$fh>;
          close $fh;
          $/='\n';
          
          if (grep /Name:\s+$host/, @response){
              return 1;   
          }
          
          return 0;
}
[download]

Here is all my code below. please feel free to comment on the code.

many thanks in advance

#!/usr/bin/perl -w

use strict;
use warnings;
use CGI qw( :standard );
use CGI::Carp qw(fatalsToBrowser);
use IO::File;
use English qw(-no_match_vars);
local $OUTPUT_AUTOFLUSH = 1;

## Path set to minimal default
$ENV{PATH} = "/usr/bin:/bin:/usr/sbin";


## Capture error message
BEGIN{
    CGI::Carp::set_message(\&carp_error);
}


############################################################
#                    ACTION HANDLER                        #
############################################################
#
#
if($ENV{REQUEST_METHOD} eq 'POST'){
        ## Fetch form data
        my $name = param('name');
        my $email = param('email');
        my $comments = param('comments');
        
        ## Check for html tags
        if($name =~ /<.*>/ || $comments =~ /<.*>/ ){
                die ("oops! you have html tags. naughty, naughty!");
        }
        ## Check to see if email is valid. 
        ## Does not match email addresses using an IP address instead 
+of a domain name.
        if ($email !~ m/\b[a-z0-9._%-]+@[a-z0-9.-]+\.[a-z]{2,4}\b/){
                die ("oops! your email address is not valid one");
        }
        
        ## Okay! you have passed the test. you have a valid email addr
+ess.
        my @result = split(m/@/, $email);
        
        if(isHostValid($result[1])) {
                die ("Oops! invalid host name");
        }
        
        # Change this to the path of your sendmail
        my $mail_prog = '/usr/sbin/sendmail';
        # Change this to your email address
        my $recip = 'youremail@host.com';

        open (MAIL, "|$mail_prog -t");
        print MAIL "To: $recip\n";
        print MAIL "Reply: $email\n";
        print MAIL "Subject:email from web form\n";
        print MAIL "\n\n";
        print MAIL "name: ". $name."\n" ;
        print MAIL "emial: ".$email."\n" ;
        print MAIL "comments: ".$comments."\n" ;
        print MAIL "\n\n";
        close (MAIL);

        
        ## Display confirmation message
        print header;
        print start_html;
        print "Thanks you for using the comment form.
                We are going to get back to you as soon
                as we can say thank you again.";
        print end_html;
}else{
    ## Display form
        print header;
        print start_html;
        print start_form(-method => "post", -action => "");
        print h4("Contact Form");
        print "Name: ", textfield(-name => "name"), br;
        print "E-mail: ", textfield(-name => "email"), br;
        print "Enter your comments:", br;
        print textarea(-name => "comments", -rows => "5", -column => "
+50"), br;
        print submit(-value => "Submit");
        print end_form;
        print end_html;
}

##
# Subroutine checks if the host is valid
#
# @param    host
#
sub isHostValid{
        my $host = shift;
    my $NSLOOKUP = '/usr/bin/nslookup';
    
      $/='';
    my $fh = new IO::File "$NSLOOKUP -type=any $host 2>&1 |";
    my @response = <$fh>;
          close $fh;
          $/='\n';
          
          if (grep /Name:\s+$host/, @response){
              return 1;   
          }
          
          return 0;
}

##
# Subroutine displays error message
#
# @param     error_message
#
sub carp_error{
    my $error_message = shift();


    
    print start_html("Error") .
                h1("Error") .
                p("Sorry, the following error has occurred: ") .
                p(i($error_message)) .
                end_html;
}
[download]

Comment on Insecure dependency in piped open Select or Download Code

Replies are listed 'Best First'.
Re: Insecure dependency in piped open by runrig (Abbot) on Jun 29, 2008 at 00:37 UTC
perlsec describes how taint mode works, and how to untaint a variable (BTW I don't see "-T" in the #! line, but the error sounds like a taint error). AFAICT, your insecure dependency comes from the $host variable in the system command, which originally was derived from the $email variable, which came from user input and was never untainted. You can't just match the variable against a regular expression...you have to capture something from the regular expression, and then set a variable to that captured value...again, read the docs.	[reply]
Re^2: Insecure dependency in piped open by gugubanana (Acolyte) on Jun 29, 2008 at 00:59 UTC
thanks runrig, for the pointer. i've read through the doc and resolved the issue. ta.	[reply]
Re: Insecure dependency in piped open by pc88mxer (Vicar) on Jun 29, 2008 at 00:41 UTC
There are two security related issues: one is a taint issue and the other has to do with not using the multi-argument version of `exec` to spawn programs. First the taint problem: the value passed into `$host` is probably tainted and you are not untainting it. See `perldoc perlsec` for more info on how to untaint data. Another security issue is your use of `exec()` with a single arguement. You should use something like: `open(my $fh, "-\|", $NSLOOKUP, "-type=any", $host) or die "unable to exec $NSLOOKUP: $!";` [download] This ensures that `$host` will not be interpreted by the shell. If you really need to re-direct `STDERR`, consider using something like `IPC::Open3`. Here is a good write-up of the security issues around tainting and calling other processes written by brian_d_foy: Secure Programming Techniques	[reply] [d/l] [select]
Re^2: Insecure dependency in piped open by gugubanana (Acolyte) on Jun 29, 2008 at 12:46 UTC
thanks pc88mxer, for your comments. your right that the `$host` is tainted and i've fixed the issue. whats more, `my $fh = new IO::File "$NSLOOKUP -type=any $host 2>&1 \|";` is same as `open(my $fh, "-\|", $NSLOOKUP, "-type=any", $host)` however only difference from the two is that yours is more optimised than mine. so i've decided to use yours. if your have more ideas of improvement or security issues please let me know. many thanks	[reply] [d/l] [select]
Re^3: Insecure dependency in piped open by sgifford (Prior) on Jun 29, 2008 at 19:37 UTC
Actually those two are subtly different. One of them has a single string, and Perl will hand that string to the shell to parse. If `$host` contains any special shell characters, the shell will interpret them; for example if `$host` was set to: `www.google.com; rm /path/to/your/script` [download] the shell will see something like: `/bin/nslookup -type=any www.google.com; rm /path/to/your/script 2>&1 \| +";` [download] and will go ahead and try to remove your script, if it has permission. That is why Perl won't let you do it with taint mode on. The multi-argument piped open doesn't send anything to the shell, and so avoids this problem. There is also a difference in where standard error goes. In the first example it will be read from the pipe; in the second it will go to the original program's standard error, perhaps to a Web server error log. Finally, for this particular purpose, there is probably a module available on CPAN (like Net::DNS) that will do the work without using an external program at all. Good luck! -- sgifford's Web page	[reply] [d/l] [select]