No, he teaches placeholders.

As noted everywhere and by everyone who knows YOU NEED TO USE PLACEHOLDERS. Don't make lame excuses. Just do it right the first time and save yourself untold grief.

I am trying to see if it works,

If it is not secure it does not! Period.

then I'll modify it to be secure.

Maybe you will, maybe you won't. The temptation to leave working code alone can be strong and pressures of time often see the 'proto' system in production years later. There is SFA overhead in doing it right the first time, in fact the total time consumed is far less than modifying poor code after the fact.

Plus, this is the administration area, the public cannot get into this area

Unless you are running this over https, and there are no other insecure CGIs on your site, and no-one except the admin users ever uses the access boxen, and you have removed page caching, and the browsers honour that, and there are no key loggers lurking around, and no one is sniffing packets on the network...... The only truly secure systems have no users.

So, is that code above still pretty bad?

I guess that depends on your definition but given what you have said about working now secure later I am not all that optimistic..... The $in{val} syntax is strongly suggestive of a hand rolled CGI param parser or cgi-lib.pl which have some significant issues. See use CGI or die;

If you want to write secure code with a login here is how I generally approach the problem.

1. If the login is not via https then anyone on the network can sniff the username/password as they go over the wire in plain text. Using https is no harder than using http - it just needs to be set up on the server.
2. OK so we show a login page, the user submits it and then we check the username/password against a DB. What next? Well if the username/password is invalid we throw the login page up again (don't bother to specify if the username or password is invalid - if you say invalid password I know I have a valid username). If not we want to maintain state through the session.
3. To protect against automated brute force password attacks just add a small timout so that the script takes say 1-2 seconds to check a password (sleep 2 works well) - this simple measure virually defeats brute forcing passwords and users will hardly notice the delay. This is generally much easier to code and far less fag to administer than X wrong attempts == lockout. First with that method you can lock someone out (if you know their username) just by hitting X wrong passwords. Next dumb users lock themselves out and then someone (?you) will have to unlock them.
4. There are a number of ways to maintain state. I prefer not to use cookies as they may be disabled in which case your script chokes.
5. So to maintain state I will pass around some hidden fields. Typically I would have 3 - one for the validated user_id/session_id, one for the session timeout and the other a hash value that ensures that these two can not be changed by a 'malicious' user.

   
   $hash = generate_hash( $user_id . $timeout );
   [blah]
   # in the form we have
   <input type="hidden" name="user_id" value="$user_id">
   <input type="hidden" name="timout" value="$timeout">
   <input type="hidden" name="hash" value="$hash">
   
   
   # now in the code we do
   use CGI;
   my $q = new CGI;
   my $user_id = $q->param('user_id');
   my $timout  = $q->param('timeout');
   my $hash    = $q->param('hash');
   
   unless( $hash ) {
       login('Login');
     exit 0;
   }
   
   if ( validate_hash( $hash, $user_id.$timeout ) ) {
       # we have a valid user id with an unchanged hash but have we timed
   + out?
       if ( time() > $timeout ) {
           login( 'Session timed out. Please Login' );
       }
       else {
           # update our timeout for the next iteration
           $timeout = time() + $TIMEOUT;  # 300 seconds is generally OK
           run_whatever();
       }
   }
   else {
       error( 'Invalid Checksum' );
   }
   
   ##### SUBS #####
   
   sub validate_hash {
       my ( $hash, $plain_text ) = @_;
     return $hash eq generate_hash($plain_text) ? 1 : 0;
   }
   
   sub generate_hash {
       my ( $plain_text ) = @_;
       require Digest::MD5;
       # note we append a random secret string so just concatenating
       # the hidden fields and trying an MD% hash will not ever give a va
   +lid checksum
     return Digest::MD5->new->add( $plain_text . 'GNUisnotunixandtheanswe
   +ris42' )->hexdigest;
   }
   [download]

   Note how we concat the hidden field values together with a secret string (our key) and make an MD5 hash so these can not be changed. Nor can the hash be guessed by MD5 hashing all the permutations of hidden field data.

   The timeout is also important. Typically I will set this 5 minutes in the future every time I deliver a new page. What this means if a user does nothing for 5 minutes the session expires automatically. So long as they hit the script once every 5 minutes they get another 5 minute window. This means that we don't really have to worry about caching of the page and its hidden fields as the info is useless after 5 minutes. This is quite nice as it means you don't have to do no_cache and expires now stuff which in turn means the user has a functional back button (instead of getting page expired).

   cheers

   tachyon

   s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print