in reply to Hacker Proofing My First Script

The *most* important missing element is using placeholders in the SQL ie 'INSERT INTO foo VALUES(?,?)'. As your code stands it is totally open to injection as I can pass any values and you don't check those values and you interpolate them directly. Your code is pretty much as insecure as it is possible to be. Placeholders will do the quoting and fix this. I recommend A short guide to DBI. You should also read Ovids CGI tutorial. This is how you might well write it in a few weeks/months.... 

#!/usr/bin/perl -wT

use strict;
use DBI;
use CGI;
use POSIX 'strftime';

my $database  = "database";
my $db_server = "localhost";
my $user      = "user";
my $password  = "password";
my $redirect  = 'http://www.yourname.com/index.html';
my @fields = qw(
    state       city        locationname    referencename       lat_de
+g
    lat_min     lat_sec     long_deg        long_min            long_s
+ec 
    xcoord      ycoord      ttime           level               radar
    laser       vascar      airplane        photo               roadbl
+ock
    unknown     comments    email           name                date_a
+dded
);

my $q = CGI->new();
my %INPUT = $q->Vars;

$INPUT{$_} ||= '' for @fields;
$INPUT{$_} ||= 'false' for qw( radar laser vascar airplane photo roadb
+lock unknown );

$INPUT{'date_added'} = strftime("%A, %B %d, %Y at %H:%M:%S", localtime
+() );

my $dbh = DBI->connect("DBI:mysql:$database:$db_server", $user, $passw
+ord);
my $sql = 'INSERT INTO speedtrap (' . ( join ',', @fields ) . ') ' .
          'VALUES (' . ( join ',', ('?') x scalar(@fields) ) . ')';
$dbh->do( $sql, @INPUT{@fields} );
$dbh->disconnect;

print $q->redirect($redirect);

[download]

I hope this shows you a few things. I would however suggest you store epoch time ie the 32 bit integer you get from time() in your database. It makes extracting data between two dates much easier and takes up much less room. If is trivial to format it into a nice string for output. You should validate data before you stuff it into your database or you will accumulate rubbish, better it never gets there in the first place.

cheers

tachyon

Replies are listed 'Best First'.
Re^2: Hacker Proofing My First Script
by awohld (Hermit) on Oct 04, 2004 at 03:59 UTC
    I've updated my script, how does this look? Did I use the DBI placeholder correctly, will I be immune from SQL injection attacks?

    Thanks for everyones help, I couldn't of done it without your support. 

    #!/usr/bin/perl -wT

use DBI;
use strict;
use CGI;
use POSIX 'strftime';

my $q = CGI->new();
my $sth;


my $state =        $q->param ('state');
my $city =        $q->param ('city');
my $locationname =    $q->param ('locationname');
my $referencename =     $q->param ('referencename');
my $lat_deg =        $q->param ('lat_deg');
my $lat_min =        $q->param ('lat_min');
my $lat_sec =        $q->param ('lat_sec');
my $long_deg =        $q->param ('long_deg');
my $long_min =        $q->param ('long_min');
my $long_sec =        $q->param ('long_sec');
my $xcoord =        $q->param ('xcoord');
my $ycoord =        $q->param ('ycoord');
my $ttime =        $q->param ('ttime');
my $level =        $q->param ('level');
my $radar =        $q->param ('radar');
my $laser =        $q->param ('laser');
my $vascar =        $q->param ('vascar');
my $airplane =        $q->param ('airplane');
my $photo =        $q->param ('photo');
my $roadblock =        $q->param ('roadblock');
my $redlight =        $q->param ('redlight');
my $unknown =        $q->param ('unknown');
my $comments =        $q->param ('comments');
my $email =        $q->param ('email');
my $name  =        $q->param ('name');

my $date = strftime("%A, %B %d, %Y at %H:%M:%S", localtime() );

##Set Radar Technology################################################
+##########
# If technology type isn't selected, it needs to be set to false.
if (!$q->param ('radar')) {$radar = "false"};
if (!$q->param ('laser')) {$laser = "false"};
if (!$q->param ('vascar')) {$vascar = "false"};
if (!$q->param ('airplane')) {$airplane = "false"};
if (!$q->param ('photo')) {$photo = "false"};
if (!$q->param ('roadblock')) {$roadblock = "false"};
if (!$q->param ('redlight')) {$redlight = "false"};
if (!$q->param ('unknown')) {$unknown = "false"};
######################################################################
+##########


##Start database connections##########################################
+##########
my $database = "database";
my $db_server = "localhost";
my $user = "user";
my $password = "password";

##Connect to database, insert statement, & disconnect ################
+##########
my $dbh = DBI->connect("DBI:mysql:$database:$db_server", $user, $passw
+ord);


my $statement = "INSERT INTO trap1 (state, city, locationname, referen
+cename, lat_deg, lat_min, lat_sec, long_deg, long_min, long_sec, xcoo
+rd, ycoord, ttime, level, radar, laser, vascar, airplane, photo, road
+block, redlight, unknown, comments, email, name, date_added) VALUES (
+?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)";

   $sth = $dbh->prepare($statement) or die "Couldn't prepare the query
+: $sth->errstr";

my $rv = $sth->execute($state,$city,$locationname,$referencename,$lat_
+deg,$lat_min,$lat_sec,$long_deg,$long_min,$long_sec,$xcoord,$ycoord,$
+ttime,$level,$radar,$laser,$vascar,$airplane,$photo,$roadblock,$redli
+ght,$unknown,$comments,$email,$name,$date) or die "Couldn't execute q
+uery: $dbh->errstr";

my $rc = $sth->finish;
   $rc = $dbh->disconnect;
######################################################################
+##########

print "Content-type: text/html\n\n";

print $q->redirect('http://www.site.com/index.html');

    [download]
[reply]
[d/l]

      Yes that is secure. Still a bit ugly, but secure nonethless. Why declare $sth at the top, miles from where you use it? Why collect the return codes in $rc - you don't *do* anything with them? It is much more convenient to put constants like your DB passwd etc at the top so you can find them.

      You have a bug. The $q->redirect header will never be acted upon as you terminate the headers with your Content-Type header. You don't need that header, just the redirect. Also print $q->header() will give you a valid header so if you are using CGI why not use it. <code>

      cheers

      tachyon

[reply]
[d/l]
        I use the $rc return code because I got it as an example out of a book. This is my first DB script so I'm still getting a handle on it.

        Thanks Adam

[reply]

In Section Seekers of Perl Wisdom