in reply to Re: Re: Passing References to Subroutines
in thread Passing References to Subroutines

I don't have time right now to look through all your code. I'll spend some time on it later tonight, but for now a quick glance reveals what I consider a major problem. You have many SQL insert and update statements that use outside data, but you don't use placeholders. I haven't looked carefully enough to know for sure if you're vulnerable, but this is a prime candidate for SQL injection attacks.

Here's an example of one of your SQL inserts:

my $executeSQL = "INSERT INTO FDNMail (msgid,subject,sourceip, +sourcednsname,[from],[to],allheaders,preamble,body,receiveddate,attac +hmentdata, custnum,[X-AOLIP]) VALUES ('".$record->{"msgid"}."','" +.$record->{"subject"}."','".$record->{"sourceip"}."','".$record->{"so +urcednsname"}. "','".$record->{"from"}."','".$record-> +{"to"}."','".$record->{"allheaders"}."','".$record->{"preamble"}."',' +".$record->{"body"}. "','".$record->{"receiveddate"}." +','".$record->{"attachmentdata"}."','".$record->{"custnum"}."','".$re +cord->{"xaolip"}."')"; my $action = $tema1->prepare($executeSQL);

Besides being generally hard to read and somewhat messy, if any of the values from $record contain tainted data, you have a huge vulnerability. I would write that like this:

my %fields = ( msgid => $record->{msgid}, subject => $record->{subject}, sourceip => $record->{sourceip}, sourcednsname => $record->{sourcednsname}, '[from]' => $record->{from}, '[to]' => $record->{to}, allheaders => $record->{allheaders}, preamble => $record->{preamble}, body => $record->{body}, receiveddate => $record->{receiveddate}, attachmentdata => $record->{attachmentdata}, custnum => $record->{custnum}, '[X-AOLIP]' => $record->{xaolip}, ); my $sql = "INSERT" . " INTO FDNMail (" . join(",", keys %fields) . ")" . " VALUES (" . join(",", ('?') x keys %fields) . ")"; my $fdnmail_sth = $temal->prepare($sql); $fdnmail_sth->execute(values %fields);

This has several advantages.

  1. It's easier to maintain. The whole thing is controlled by the %fields hash, so new fields can be added, or fields can be removed, very easily. There won't be problems counting commas to make sure everything lines up.
  2. It's safer. Using placeholders eliminates the possiblity of SQL injection attacks, because proper escaping and quoting is done automatically.
  3. It's generic. The actual insertion code could be abstracted to a subroutine that takes in the DB handle and a hash of fields, making it easy to get these advantages all througout your code. Also, since the quoting and escaping is done by the DB driver, this is portable to any database that DBI supports.

Replies are listed 'Best First'.
Re: Re: Re: Re: Passing References to Subroutines
by ketema (Scribe) on May 19, 2004 at 13:23 UTC
    That is an excellent idea. As you can Tell Perl is a litte new to me, and I haven't gotten all its tricks learned yet, but i certaintly see the value of organizing the Insert statement in that manner. I also found that the recursive subroutive can't handle(at least on the machine I'm running this on) more than 5 attachments, depending on their size. I was able to upload 39768 out of 40321 messages successfully by limiting the subroutine in that manner. The only failures were due to bad character data from the attachements. Apparently I have missed a quotation mark somewhere, and I think your organized SQL builder will help with that. Much Appreciated.
    Ketema
Re: Re: Re: Re: Passing References to Subroutines
by ketema (Scribe) on May 19, 2004 at 14:34 UTC
    Well I tested your code, but unfortunately it looks like my DB doesn't support it:

    Panic: dynamic SQL (? placeholders) are not supported by the server you are connecting to

    I am using the DBI::Sybase driver combined with FreeTDS and unixODBC to connect to a Microsoft SQL Server. To be honest I'm not exactly sure which driver is being used. I found several articles on how to connect to a SQL 2000 Server from PERL and Linux, and it worked, so I didn't think much on it. I read the article on placeholders and the perils of SQL injection so I understand the benefits. However I'm not too worried about SQL injection because I am in a controlled environment so I'm not too worried about that. Any suggestions?
    Ketema
Re: Re: Re: Re: Passing References to Subroutines
by ketema (Scribe) on May 19, 2004 at 15:04 UTC
    Another general question. Any idea on how to convert a messgae size represented in octets to KB?
      Divide by 1024.