Do you see a problem with this?

There are lots of issues with your code. The first major one is that you have no error handling (as far I can see) and thus no idea what the problem is - checking the logs is always a good start. The next is that you are passing raw user input to your DB and are not using place holders to quote (automatically escape) your values. As your code stands if I passed it a param 'id' with a value like ' "; DROP TABLE users;.......' it would probably execute any SQL I wanted. Adding yourself as a root user is always a good trick when this happens (with remote access rights of course ;-)

There is loads of material on how to use databases well (safely and reliably) on this site. Start in Tutorials I am not familiar with the book you quote but if it taught you to generate code like you posted I'm afraid to say it is not much chop. This is a very good, short and free reference that covers things in more detail. A short guide to DBI by Dominus. I really suggest you read it. As a starter here is a useful template:

my $dbh = do_dbi_connect( $db_type, $db_name, $db_username, $db_passwo
+rd, @_ );

my $sql =<<SQL;
SELECT *
FROM some_table
WHERE
    some_val  = ? OR
    other_val = ?
SQL

# now exec our sql using bind values to get our sth
# these replace our ? placeholders and will be quoted correctly
my $sth = do_sql_sth( $dbh, $sql, $bind_val1, $bind_val2 );
while( my $res = $sth->fetchrow_array() ) {
    # blah
}
$sth->finish

##### STANDARD DATABASE FUNCTIONS #####

# do_dbi_connect( DB_TYPE, DB_NAME, DB_USERNAME, DB_PASSWORD, @OPTIONS
+ )
# connect to the specified DB_TYPE and DB_NAME using the DB_USERNAME a
+nd
# DB_PASSOWRD as well as passing any other @OPTIONS
# returns the DBH
sub do_dbi_connect {
    my ( $db_type, $db_name, $db_username, $db_password ) = ( shift, s
+hift, shift, shift );
    my $dbh = DBI->connect("dbi:$db_type:$db_name", $db_username, $db_
+password ) or die_nice( $DBI::errstr );
  return $dbh;
}

# do_sql( DBH, SQL, BIND_VAL1.....BIND_VALx )
# prepares, executes and closes sth in one call
# typical use for create tables, update tables
sub do_sql {
    my $dbh = shift;
    my $sql = shift;
    my $sth = get_sth( $dbh, $sql );
    $sth->execute(@_) or 
        die_nice( "Could not execute SQL statement\n\n$sql\n" . $sth->
+errstr() );
    $sth->finish;
}

# do_sql_sth( DBH, SQL, BIND_VAL1.....BIND_VALx )
# as for do_sql but returns the STH
# obviously does not call finish on the STH (so you have to remember)
# typical use for fetchrow_blah
sub do_sql_sth {
    my $dbh = shift;
    my $sql = shift;
    my $sth = get_sth( $dbh, $sql );
    $sth->execute(@_) or 
        die_nice( "Could not execute SQL statement\n\n$sql\n" . $sth->
+errstr() );
  return $sth;
}

# get_sth( DBH, SQL )
# returns a (cached) STH for the supplied DBH and SQL
sub get_sth {
    my ( $dbh, $sql ) = @_;
    my $sth = $dbh->prepare_cached($sql) or 
        die_nice( "Could not prepare SQL statement\n\n$sql\n" . $dbh->
+errstr() );
  return $sth;
}

# die nice( ERROR )
# dies with ERROR after printing a valid header and error message to b
+rowser
# you may choose to modify this to produce a BS message to the user li
+ke
# 'the server can not respond to your request at this time due to rout
+ine  maintenance'
# or some such rubbish. just make sure you get to see the ERROR!

sub die_nice { 
    print "Content-type: text/html\n\n$_[0]"; 
    die $_[0] 
}
[download]

cheers

tachyon

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

you are passing raw user input to your DB...

A serious concern here is that you can turn "where id = $something" into "where id = <anything> or 1=1" possibly letting anyone have admin access.

{Sorry, I did this hours ago, I thought I submitted it after I previewed it, but obviously I did not ;(}

No, he teaches placeholders. I'm only in testing mode, I am trying to see if it works, then I'll modify it to be secure. Plus, this is the administration area, the public cannot get into this area, and I just don't know that it would work.

I was very tired when I did that, and forgot that right above that, $row contained the record for that user, so I could get rid of the database lookup of the username, and just do it like this: $cust_username = $row->{username}; So that fixed all that part.

Here is the code now:

        $sth1 = $dbh->prepare (qq{ SELECT * FROM `sessions` });
        $sth1->execute();
        while($row1 = $sth1->fetchrow_hashref()) {
            next if !$row1->{id};
            $sess_ref1 = CWT::Site_DB->open_with_expiration(undef, $ro
+w1->{id});
            if($sess_ref1->attr("username") eq "$cust_username") {
                if ($sess_ref1->attr("logged_in") == 1) {
                    $sess_ref1->attr("is_admin",1);
                } else {
                    next;
                }
            } else {
                next;
            }
        }
        $sth1->finish();
[download]

Yes, CWT::Site_DB is a module that I created.
And if you clear the admin attribute, but it doesn't take effect until their next login, then are you checking for the attribute every time they request an admin page?
Yes, it's checking the Session, EVERY time the page loads, if the session does not contain that line, then it don't give them any access to that page.

Do the 'attr' and 'clear_attr' methods update the database immediately? YES, attr is used to retrieve and set data, and clear_attr is used to delete the session variable that I pass. I'm thinking of making it where I can pass multiple variables to it to delete, but right now, it only accepts one at a time to delete.

So, is that code above still pretty bad?

thx,
Richard

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

When I load the page, either adding or removing the admin "powers", it times out, then the dang site won't load for me for at least an hour. If I don't do that,

...then it just adds them or removes them depending on the action, but it don't work until they Next login.

Update: Putting raw user input into SQL statements is in general a bad idea, use placeholders or at least the DBI quote() method. (Ah, tachyon mentions this below, but its worth reiterating).

Without seeing what's going on down in CWT::Site_DB, it's hard to say.

A flag goes up when I see code that mixes direct queries with queries that are abstracted away behind classes. It's hard to get a grip on what your connections and statement handles are up to unless they're either all exposed, or all abstracted away behind well unit-tested classes (or at least that's been my experience).

Scanning the code, it shows that you're starting off with an id, querying first to get the corresponding username, then querying and iterating over a session table to look for (a non-expired session?) with the same username

Is that something you could arrange to do in a single query, using JOINs or OUTER JOINs?

Also, it's generally bad form to "SELECT *" in production code unless you're going to use all of the fields. Better to name the field explicitly.