Security and Style for CSV to mySQL script

[jerrygarciuh]

Esteemed Monks,

I have been written this script (with kind advice from Zaxo and dws) to aid a client in uploading a CSV and importing it into their mySQL db. I am very interested to here opinions on it's security and style. The script will be protected via .htaccess and is not linked from any page. The secondary password is stored encrypted in a db table. The file must have a specific name to be accepted and uploaded, so I haven't used taint-checking on the filename and I believe I don't need to.

Any advice and opinions much appreciated!

TIA
jg

#!/usr/local/bin/perl -w
use strict;


use CGI::Carp qw/fatalsToBrowser /;
use Text::CSV;
use DBI;
use File::Copy;
use POSIX qw(strftime);

use CGI qw/:standard /;

$" = ',';  #list seperator
my $q = CGI->new();
my $csv = Text::CSV->new();


my $target_directory = "/home";
my $copy_directory = $target_directory . "/old/";
my $correct_filename = "whatever.csv";

my $database = "whatever";
my $hostname = "mysql.whatever.com";
my $password = "whatever";
my $user = "whatever";
my $table="whatever";
my @columns= ("mid","name","sex");  ##list of column names
my $pass_table ='whatever_pass';

my $num_values = @columns;  ##how may columns?
my @question_marks;

while ($num_values > 0) {

    push (@question_marks, "?");
    $num_values--;
}

my $dsn = "DBI:mysql:database=$database;host=$hostname";

my $dbh = DBI->connect($dsn, $user, $password, {'RaiseError' => 1});

if ($q->param('action') eq 'upload') {
    password_check();
} elsif ($q->param('action') eq 'update') {
    update_db();
}

exit;



sub update_db {

    open (FH, $target_directory . $correct_filename) or die "File Not 
+Found. : $!";
    while (<FH>) {
        my $line = $_;
        my $status = $csv->parse($line);         # parse a CSV string 
+into fields

        my @csv_values = $csv->fields();            # get the parsed f
+ields
        my $bad_argument = $csv->error_input();  # get the most recent
+ bad argument
        if ($bad_argument) {
            die "Error Parsing CSV: $bad_argument\n$!";
        }
        my $sth = $dbh->prepare("SELECT * FROM $table WHERE mid=?");
        $sth->execute($csv_values[0]);
        my $found = $sth->fetchrow_hashref();
        if ($found) {
            $sth = $dbh->prepare("UPDATE $table SET
            name=?,
            sex = ?
            WHERE mid = ?");
            $sth->execute($csv_values[1],$csv_values[2], $csv_values[0
+]);
        } else {
            $sth = $dbh->prepare("INSERT INTO $table (@columns) VALUES
+ (@question_marks)");
            $sth->execute(@csv_values);
        }
    }
    print $q->header, start_html, h2({-align=>'center'},"Database Upda
+ted."), p({-align=>'center'},
    "Thanks.  Have a nice day."), end_html;

    my $timestamp = strftime( "%m_%d_%Y_%I_%M_%p", localtime );
    copy ($target_directory.$correct_filename, $copy_directory.$timest
+amp) ;
    unlink $target_directory.$correct_filename;
}




sub file_up {
    my $upload = $q->param('upload_file');
    if ($upload !~ /\Q$correct_filename\E$/) {
        print $q->header, h2("Error 321. Please Consult Manual.");
        exit;
    }
    local $| = 1;
    my ($bytesread,$buffer,$file);
    my $fh = $q->upload('upload_file');
    open(OUTF, '>' . $target_directory . $correct_filename);
    while ($bytesread = read($fh, $buffer, 1024)) {
        print(OUTF $buffer);
    }
    close(OUTF);
    if (!$file && $q->cgi_error) {
        print($q->header(-status=>$q->cgi_error));
        exit 0;
    }
    print $q->header,start_html, h2({-align=>'center'},"File Received.
+"), start_form,
            hidden( {-name=>'action',-value=>'update',-force=>1} ),div
+({-align=>'center'},
            submit("Update Database")), end_form, end_html;
}

sub password_check {

    my $pass = crypt($q->param('pass'), 'Ll');
    my $sth = $dbh->prepare("SELECT * FROM $pass_table WHERE uid = ?")
+;
    $sth->execute('master');
    my $ref = $sth->fetchrow_hashref();
    if ($pass eq $ref->{pass}) {
        file_up();
    } else {
        print $q->header, h2("Error 123. Please Consult Manual.")."Ple
+ase try again.";
    }
}

_____________________________________________________
Think a race on a horse on a ball with a fish! TG

[neilwatson]

For security reasons you should always have the authentication information stored in a separate, secure file. Call the file using "require". If this is a cgi script then the authentication file should be stored outside of the webserver's document tree. You don't want to give anyone the chance to read it.

Neil Watson
watson-wilson.ca

[jerrygarciuh]

Thank you! Definitely should have thought of that.
jg
_____________________________________________________
Think a race on a horse on a ball with a fish! TG

[neilwatson]

Also, (this may seem to obvious, if so I apologize) make sure the user account you use to access mysql has only the permissions it needs. So if the user is only viewing the information of a specific DB then the mysql user account should have read-only access to that DB only. You can even specify permissions by tables IIRC.

Neil Watson
watson-wilson.ca

[Zaxo]

I'd prefer to set @question_marks like this:

my @question_marks = ('?') x @columns;

which populates the whole thing at once and eliminates an auxiliary variable.

You should localize $", moving it inside braces close to where it's needed.

        else {
            local $" = ', ';
            $sth = $dbh->prepare(
                       "INSERT INTO $table (@columns) VALUES (@questio
+n_marks)"
                   );
            $sth->execute(@csv_values);
        }

That will prevent surprises in case arrays are stringified elsewhere.

After Compline,
Zaxo