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.

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]

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

I'd advise you to do some reading (there are many good tutorials here, search for security or CGI or DBI).

update : added link around the word tutorials, thanks PodMaster :-)

# Your code
use CGI;

read(STDIN, $buffer, $ENV{'CONTENT_LENGTH'});
@pairs = split(/&/, $buffer);
foreach $pair (@pairs) {
        ($name, $value) = split(/=/, $pair);
        $value =~ tr/+/ /;
        $value =~ s/%([a-fA-F0-9][a-fA-F0-9])/pack("C", hex($1))/eg;
}
[download]

my $comments = $q->param('comments');
[download]

my %params = map { $_ => $q->param($_) } $q->param;

# comments stored in $params{'comments'}
[download]

You are absolutely correct that if the OP is planning on parsing CGI form input, a much safer and more reliable way to do so (instead of inventing ones own possibly flawed solution) is to use the CGI module. ...however...

Someone here (I can't remember who) showed me a way to store the submitted values to a hash:

my %params = map { $_ => $q->param($_) } $q->param;
[download]

Someone's going to be glad you couldn't remember who (s)he was. The POD for CGI is mandatory reading material if you plan to use the CGI module:

FETCHING THE PARAMETER LIST AS A HASH:

    $params = $q->Vars;
    print $params->{'address'};
    @foo = split("\0",$params->{'foo'});
    %params = $q->Vars;

    use CGI ':cgi-lib';
    $params = Vars;
[download]

Many people want to fetch the entire parameter list as a hash in which the keys are the names of the CGI parameters, and the values are the parameters' values. The Vars() method does this. Called in a scalar context, it returns the parameter list as a tied hash reference. Changing a key changes the value of the parameter in the underlying CGI parameter list. Called in a list context, it returns the parameter list as an ordinary hash. This allows you to read the contents of the parameter list, but not to change it.

The snippet in the POD shows four or five methods. I prefer the $href = $params->Vars(); most of the time, since it doesn't pass around a copy.




Dave

Another major aspect is never visible in the script source code, and that is to set up your server so the CGI never runs as the web server user. For every task set up a different fake user and use something like suexec to run your CGI as that user. That way if you do have a hole, your server isn't directly compromised (though even getting local user access is already a huge step for a hacker). Here it will also allow you to set the permissions of your SQL server so that this user (and therefore CGI script) may only do the things he needs to do here. That DOES help somewhat against SQL injection (though the real solution to that is still using placeholders) and even against someone taking over your script and now trying to access the SQL server.

I just have one point to add to the excellent advice given so far. It's actually database related, not anything in your Perl script:

Make sure that the database userid is as unprivileged as possible. I notice you're using mysql and I don't know much about how that compares to other RDBMS but I assume you can have different levels of user, such as sa/dbo/normal, and that you can grant and/or revoke permissions for select/insert/update on individual tables. So make this user as "low" as you can and make sure it doesn't have permission to do anything except insert into the speedtrap table.

Of course, if I'm talking cobblers regarding mysql, then apologies...

More of a style nit, but all of your if (!$INPUT{'radar'}) {$INPUT{'radar'} = "false"}; statements could be replaced with:

use constant TECHNOLOGY_TYPES =>
    qw( radar laser vascar airplane photo roadblock unknown );

for my $tech ( TECHNOLOGY_TYPES() ) {
  $INPUT{ $tech } ||= "false";
}
[download]

Of course that sidesteps the problem that you've used CGI and yet for some reason are doing your own query parsing (which is in itself a red flag) . . .