I seem to see this all too often. Input is filtered against a few regexes and assumed safe for processing. the true solution to security involves refusing input that doesn't match a criterion and that's IT. In attempt for userfriendliness, it is sometimes attempted to work around this as i see above but this can and will result in security holes that you really can't afford. So again, one filter- pass/fail.

AgentM Systems nor Nasca Enterprises nor Bone::Easy is responsible for the comments made by AgentM.

Lots of good advice, but I'm surprised no one mentioned avoiding the shell entirely by using the multi-argument forms of system() or exec() to call traceroute directly.

Put together with the above advice and some sugar, you get something like ...

#!/usr/bin/perl -wT

$|++;

use strict;
use CGI qw( header start_html end_html param p );

print header, start_html( "RavenGate TraceRoute Results" );

if ( param('trace') and param('trace') =~ /^[-.0-9a-zA-Z]+$/ ) {
    print p( "Hello There- I am writing this from "
           . "scratch so please be patient. Thanks!" ),
        "<pre>";

    system( "/usr/sbin/traceroute", param('trace') );

    print "</pre>";
    print p( "Oops, there was a problem running traceroute" ) if $?;
} elsif ( param('trace') ) {
    print p( "Oops,", param('trace'), "contains illegal characters" );
} else {
    print p( "Oops, you forgot to give me a host to trace to." );
}
[download]

As for that one line that "doesn't work", you need to escape the pipe (ie, $value =~ s /\|/ /g;).

...
if ( param('trace') and param('trace') =~ /^[-.0-9a-zA-Z]+$/ ) { 
    print p( "Hello There- I am writing this from " 
           . "scratch so please be patient. Thanks!" ), 
        "<pre>"; 
 
    system( "/usr/sbin/traceroute", param('trace') ); 
...
[download]

Hoo boy. Watch out. You've used param() in both a scalar and a list context. In your security test, you look for things of interest, but in a scalar context, which means you are testing the first param value for trace if multiple values are defined. But then, you suck down all the param-values for trace when you fire off the traceroute. Guess what? You are now passing untested data to traceroute!

And given that there were some buffer overflow problems with traceroute if I recall my BUGTRAQ listings, giving root access to local users, you've just handed the keys to the kingdom to all-comers. Whee!

Security - it's not just for breakfast any more.

-- Randal L. Schwartz, Perl hacker

Would this be a better approach?


Update: corrected the code (thanks merlyn); sorry for the delay...

   # assuming -T
   ...
   my $param_cmd = param( 'trace' );
   if ( $param_cmd ) and ( $param_cmd =~ /^[-.0-9a-zA-Z]+$/ )
   {
      $param_cmd = $1;
      print p( "Hello There- I am writing this from " 
               . "scratch so please be patient. Thanks!" ), 
               "<pre>"; 
        
      system( "/usr/sbin/traceroute", $param_cmd ); 
      ...
   }
[download]

(Sorry it this seems basic, but I'm trying to make sure I've learned the right things from the various FAQ's and nodes on the subject...)

How about something like: $value =~ m/([-.0-9a-zA-Z]+)/; $value = $1; That would only allow dashes, dots, and alphanumerics, which IIRC are all you are allowed to use in a fully-qualified domain name.

unless($value =~/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/) {
       ##reject this...
}
[download]

if ($value =~ /\/|;|#|!|"|'|`|\|)/) {
   print "Invalid parameter";
   # perhaps log this "malicious" attempt
} else {
   processhappily();
}
[download]