I do some moonlighting on the side. Recently, I was asked to look over a script and either rewrite it or upgrade it. I took one look and told him to bring the script down and have me rewrite it. There are at least 2 major security flaws, 2 bugs, and at least 3 others I could smell. How many can you find?

#!/usr/bin/perl ################################# Read Me ########################## +#### # + # # Program: opt_in.cgi + # # + # # This program will + # # + # ###################################################################### +#### ############################# Path Variables ####################### +#### # Base path to program domain. $public = "/someplace/far/away"; # Carmine's address. $other= 'joe@schmoe.net'; # Name of footer file. $foot = "/foot.html"; # Name of header file. $head = "/head.html"; ###################################################################### +#### use CGI qw(:standard :html3); use DBI(); use Net::SMTP; use Socket; # If input is ok, proceed. if (aok()) { # Add client to database and send notification with opt out link. opt_in(); # If there's a comment give success page. if ($comment) { print "\n<p><b>Hello $name</b></P>\n<p>Thank you for your commen +t, it reads as follows:<br>\n<p>$comment</P>"; piece($foot); } # If no comment no content. else { print "Content-Type: text/html\nStatus: 204 No Content\n\n"; + }} # If there's a comment or extra fields mail them to gabe. if ($comment || $xtra) { mail_stuf('comment@webzen.com', $email, $subject, "$xtra$comment" +); # If this form is from webzen mail one to moe also. # if ($ENV{HTTP_REFERER} =~ m<^http://www\.someplace.com/.*>) { mail +_stuf($other, $email, $subject, "$xtra$comment"); } } exit 0; ###################################################################### +#### ###################################################################### +#### # Check the input data. sub aok { # If direct hit redirect to main site. if (!param()) { print "Location: http://www.someplace.com\n\n"; ret +urn undef; } # Get the local variables. $email = param('email'); $site = param('site'); $type = param('type'); $in = param('function'); $subject = "$site.com"; if (param('subject')) { $comment = "\nRe: "; $comment .= param('sub +ject'); $comment .= "\n\n"; } if (param('name')) { $comment .= "\nName: "; $comment .= param(' +name'); $comment .= "\n\n"; } if (param('comment')) { $comment .= param('comment'); } # Check for good data. if (!$site || !$type || !good_address($email)) { return undef; } # Add any extra info put on the form. if (param('xtra') eq "xtra") { xtra_stuf(); } # Get the IP address. $ip = remote_addr(); # Set the date. $date = `date '+%D'`; # Get this far, everythings ok. return 1; } ###################################################################### +#### # The paths to the particular header is built. sub build_paths { if (!$site) { $site = 'generic'; } $foot = "$public$site$foot"; $head = "$public$site$head"; } ###################################################################### +#### # Check for the existance of address. sub found { (my $dbh) = @_; my $ret_val = undef; my $sth; # Print sql statement for active list member. $sth = $dbh->prepare("select 'x' from opt_in where email=\'$email\' + and type=\'$type\'"); $sth->execute(); # If a row exists for this surfer return 1 (success). if ($sth->rows gt 0) { $ret_val = 1; $active = 1; } # Else print sql statement for inactive list member. else { $sth = $dbh->prepare("select 'x' from opt_in where email=\'$email\' + and type=\'NULL\'"); $sth->execute(); # If a row does not exist for this surfer return 2 (success). if ($sth->rows gt 0) { $ret_val = 1; }} # Clean up $sth->finish(); # Else return 1 (success). return $ret_val; } ###################################################################### +#### # The e-mail address is checked for an "@", a "." and no spaces. sub get_id { my $dbh = shift; # Prepare and execute sql statement. my $sth = $dbh->prepare("select id from opt_in where email=\'$email +\' and type=\'$type\'"); $sth->execute(); # Build and return id number.. my $ref = $sth->fetchrow_hashref(); $temp = 776869 + $ref->{'id'}; $sth->finish(); my $id = "012"; $id .= $temp; $id .= "345"; return $id; } ###################################################################### +#### # The e-mail address is checked for an "@", a "." and no spaces. sub good_address { my $email = shift; ((index($email,"@") ne -1) && (index($email,".") ne -1) && (index($ +email," " ) eq -1))? return 1: return undef; } ###################################################################### +#### # The results are mailed if selected (disabled). sub mail_stuf { (my $to, my $from, my $subject, my $comment) = @_; $comment =~ s/([\!|\#|\;])//g; # Connect to SMTP and mail. $smtp = Net::SMTP->new('mail'); $smtp->mail($from); $smtp->to($to); $smtp->data(); $smtp->datasend("To: <$to>\nSubject: $subject\n$comment"); # Close $smtp->dataend(); $smtp->quit(); } ###################################################################### +#### # The page for a remote opt in. sub opt_in { my $stmt; ($name, undef) = split(/@/, $email); # Connect to the database. my $dbh = DBI->connect("DBI:mysql:database=my_db;host=localhost", " +root", "", {'RaiseError' => 1}); if (!found($dbh)) { if (!$in) { $type = "NULL"; } $stmt = "insert into opt_in (email, type, domain, date, ip) values +(\'$email\', \'$type\', \'$site\', \'$date\', \'$ip\')"; } elsif ($in && $active) { $stmt = "update opt_in set domain=\'$site\ +', date=\'$date\', ip=\'$ip\' where (email=\'$email\' and type=\'$typ +e\')"; } elsif ($in) { $stmt = "update opt_in set domain=\'$site\', date=\'$ +date\', ip=\'$ip\', type=\'$type\' where (email=\'$email\' and type=\ +'NULL\')"; } if ($stmt) { $dbh->do($stmt); } # If successful print sucess message and mail confirmation. if ($comment) { build_paths(); piece($head); if ($in) { print "\n<P>Welcome $name, you have been added to<br> + \Someplace's opt-in mail list, The Foobar'ed.</P>\n<P>You will recei +ve an email confirmation.</P>\n<P>Thank you.</P>\n\n<P><a href=\"http +://www.someplace.com/\">Someplace</a></P><hr>"; }} if ($in) { $id = get_id($dbh); my $welcome = "\nHello, $name\n\nYou have been added to Someplac +e's opt-in elist, The FooBar'ed. \n\nThe FooBar'ed is published occas +sionally.\n\nIf this subscription is an error click below and you wil +l be removed.\nhttp://www.someplace.com/cgi-bin/foobar/option.cgi?$id +\n \nThank you\n\n--\nGabriel Zappia, President\nWebZen, Inc.\n(207) +799-5221\n\nhttp://www.someplace.com"; mail_stuf($email, "joe@boo.com", "FooBar'ed confirmation", $welc +ome); } # Clean up $dbh->disconnect(); } ###################################################################### +#### # A piec of html is grabbed from file and sent to stdout. sub piece { my $segment = shift; # Open the file. if (open (text1, $segment)) { # Prime line and print out the file. my $line = <text1> ; while ($line ne "") { print ($line,"\n") ; $line = <text1>; }} # Close and exit. close text1; } ###################################################################### +#### # This will concatenate any extra fields into an email to gabe. sub xtra_stuf { $xtra = undef; my %supres = ( "comment" => 1, "email" => 1, "type" => 1, "site" => 1, "xtra" => 1); ($name, undef) = split(/@/, $email); # If all is selected ignore the other 4 checkboxes. if (param('all')) { $supres{looksmart} = 1; $supres{odp} = 1; $supr +es{snap} = 1; $supres{yahoo} = 1; } # Reset the subject. $subject = "Inquiry from $name regarding $site.com"; # Build the extra fields string. for (param()) { if (!exists($supres{$_})) { $xtra_field = param($_); $xtra = "$xtra\n$_: $xtra_field"; }} # Return the xtra string. $xtra = "\n *** Original Message Follows *** \n$xtra\n\n"; } ###################################################################### +#### ###################################################################### +####

------
We are the carpenters and bricklayers of the Information Age.

The idea is a little like C++ templates, except not quite so brain-meltingly complicated. -- TheDamian, Exegesis 6

Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

Replies are listed 'Best First'.
Re: How many security holes can you find?
by Anonymous Monk on Sep 14, 2003 at 21:38 UTC
    Here is a brief list
    1. line 1 - no -T and no -w
    2. line 2 - no "use strict;" (and no "use warnings;")
    3. line x - variable scoping issues (very bad)
    4. line x - no style (perltidy should be run)
    5. line x - comments are either incomplete or useless (see line 142-146 for example, the comment is "# Clean up" followed by $sth->finish();)
    6. line 42 - he prints $name, no entities escaping is done -- he does this EVERYWHERE
    7. line x - no cleaning of any param values is done -- hello spam relay
    8. line 97 - use of `date '+%D'` -- what's next, he's gonna shell out for sort?
    9. line 125 - like with the spam relay, his database is open to corruption (this makes me think DBI ought to protect idiots from themselves like java does with executeQuery/executeUpdate)
    10. line 189 - needless repetition - if he doesn't know my can take a list, he doesn't know many other things
      7. line x - no cleaning of any param values is done -- hello spam relay

      While I agree that cleansing input is important, it seems unfair to label this script as a "spam relay".

      I haven't read every line of this script, but from what i can see the emailing functionality is used in 3 places. 2 of these are messages sent to hard coded email address, (yeah for common sense). THe third is in fact sent to an address specified via the form input -- but since the point of the script is to send an email confirming that the address is valid for registration, there's not a lot of ways arround that.

      The email sent will definitely contain information about where it came from, including why it was sent, and how to "Opt Out" -- including a phone number (which I have to applaud the inclusion of). In fact, the only means a malicious user has to infuence the body of this message is via "$name ... and that's not even a value they are allowed to submit directly, it must be the "username" portion of the email they want to send.

      This script may have a lot of issues, but being an open relay doesn't seem to be one of them.

      I almost forgot, he also does "use Socket;" (line 29), but doesn't directly use anything Socket exports.
        I get the impression the coder assumed checking HTTP_REFERER made the email address, et. al, safe, since they must have come from the right form.

        Learning to string together working Perl is one thing. In fact, it's pretty easy. Learning the ins and outs of HTTP and how to prevent easy exploitation is another thing.

Re: How many security holes can you find?
by pzbagel (Chaplain) on Sep 15, 2003 at 07:02 UTC

    Not to be a party-pooper but did you have permission to post that code? If someone originally paid to have that written and are paying you to write a new version, they may own the copyrights to this version and the one you are writing. Clearly posting the code in it's entirety (whether it is full of holes or not) is bound to infringe their copyright.

    I only bring it up because the OSS community and the GPL itself is being tested now by this whole SCO debacle and while I am all for free (as in speech) software, I feel that the community as a whole still needs to respect the rights of those who own and maintain "closed-source" software.

    Peace

Re: How many security holes can you find?
by Juerd (Abbot) on Sep 14, 2003 at 21:59 UTC

    With code like this, it's easier to rewrite everything than to find anything.

    Juerd # { site => 'juerd.nl', plp_site => 'plp.juerd.nl', do_not_use => 'spamtrap' }

      And, that's what I suggested and am being paid to do. (Actually, I was supposed to re-implement the emailing functionality with a few new gizmos, but once alerted to security holes, I have been asked to rewrite the whole thing from scratch, adding the new features along the way.)

      ------
      We are the carpenters and bricklayers of the Information Age.

      The idea is a little like C++ templates, except not quite so brain-meltingly complicated. -- TheDamian, Exegesis 6

      Please remember that I'm crufty and crochety. All opinions are purely mine and all code is untested, unless otherwise specified.

Re: How many security holes can you find?
by hardburn (Abbot) on Sep 15, 2003 at 14:01 UTC

    Two related problems: Checks for e-mail address validity by looking for an '@' and a '.' and parses the user portion of the e-mail address by splitting at the first '@'.

    The first is wrong because parsing an e-mail address is a lot harder than most people think. Now, because this is Perl, somebody mercifully wrapped up the hard part inside Mail::Valid (and other modules as well, but that's the one I usually use). Using that module will take you two lines, including the use statement.

    For the second part, it is perfectly valid to have more than 1 '@' in an e-mail address (which goes back to the problems in parsing e-mail addresses). This is used to specify relays to put a message through. Admittedly, this is really a mis-feature that most MTAs no longer allow because it is too easy to use for spam.

    The way you want to get the user portion of the address is to use Mail::Address's user() method. This will take a whopping three lines of Perl (gasp!).

    ----
    I wanted to explore how Perl's closures can be manipulated, and ended up creating an object system by accident.
    -- Schemer

    Note: All code is untested, unless otherwise stated

      A big security hole is not using placeholders. Always use placeholders or a dbi->quote(), or else untainted values can really mess up the database.
Re: How many security holes can you find?
by Anonymous Monk on Sep 14, 2003 at 21:59 UTC

    Just a few ones:

    • line 218: connection with user "root" and no password!
    • line 228: do($smtp) without error checking
    • lines 126, 136: execute without error checking
    • No placeholders
    • "where ... type=\'NULL\'" smells of being dead wrong.
      • line 228: do($smtp) without error checking
      • lines 126, 136: execute without error checking
      What do you think RaiseError does?