in reply to Re: CGI and Traceroute
in thread CGI and Traceroute

...
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

Replies are listed 'Best First'.
RE: Possible Security Hole
by footpad (Abbot) on Oct 19, 2000 at 19:37 UTC
    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...)

[reply]
[d/l]
      This may work for you:

      use Untaint;

my @dirty_params = param('trace'); # your array
my @clean_params = untaint(qr(^[-.0-9a-zA-Z]+$), \@dirty_params);

      [download]

      Assuming all members of your array are launderable, you will be returned an array of clean values.

      Cheers,
      KM

[reply]
[d/l]

      Your parens are a bit off in the if. Try ...

      if ( $param_cmd and $param_cmd =~ /^([-.0-9a-zA-Z]+)$/ ) {

      Note the addition of parens in the regexp so that you save it in $1, as the way you had it would have set $param_cmd to be undefined.

      Also, there's no need for qw() which would've passed a different argument than you expected ...

      system( "/usr/sbin/traceroute", $param_cmd );

        --k.

      ( Ain't life^H^H^H^Hsecurity a biiyatch? :)
[reply]
[d/l]
[select]
[reply]
[d/l]

In Section Seekers of Perl Wisdom