Beefy Boxes and Bandwidth Generously Provided by pair Networks
Clear questions and runnable code
get the best and fastest answer
 
PerlMonks  

Sessions, Perl and MySQL

by powerhouse (Friar)
on Mar 30, 2003 at 06:54 UTC ( [id://246699]=perlquestion: print w/replies, xml ) Need Help??

powerhouse has asked for the wisdom of the Perl Monks concerning the following question:

Ok, I have a Module I wrote with the help of Paul DuBois' "Perl and MySQL for the web". Which has taught me a lot.

I have modified a few parts of it to fit what I'm doing.

I've created an "admin" interface, whereby, the individual I've created it for, and myself can manage this site, including Registered users.

In the Admin interface there is a link that is for "managing users", if you click that, it will show their status, if they are verified(email), if they are admin(yes or no). If they are NOT an administrator, their is an option to add them, if they are, their is an option to remove them.

In both, I want it to Open EVERY session_id, Check the "session's username" as in the session, if exists.

For Adding "admin" powers, I want it to check if it is their session, and they are logged in, then if both of those are true, then it ADDS the variable the system checks to see if they are an admin user.

If it's to remove them, then it checks to find All THEIR sessions that are not deleted, and if they contain the variable that says they are admin users, then it deletes those variables. This way, if they are currently logged in, and I remove them, it's real time, not only after they log out and back in. and the other way around too. Where it will add them, real time, not only after they logout and back in.

So to do this, I added this to the code, in the appropriate place. Do you see a problem with this?
$cust_username = $dbh->selectrow_array(qq{SELECT username FROM + users WHERE id = "$in{id}"}); $sth1 = $dbh->prepare (qq{ SELECT * FROM session_db }); $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();
I did it this way in the Remove them code:
$cust_username = $dbh->selectrow_array(qq{SELECT username FROM + users WHERE id = "$in{id}"}); $sth1 = $dbh->prepare (qq{ SELECT * FROM session_db }); $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("is_admin") == 1) { $sess_ref1->clear_attr("is_admin"); # Delete that +variable from sess. } else { next; } } else { next; } } $sth1->finish();


It seems like that would work. I use $sess_ref for the regular sessions, so I added 1 behind it to open each session.
This is not working. 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.

Do you see a reason why this would not work?

Do you need to see more? If so, what part, module or the actual code surrounding the above extracts?

Thanks,
Richard

Replies are listed 'Best First'.
Re: Sessions, Perl and MySQL
by tachyon (Chancellor) on Mar 30, 2003 at 08:09 UTC

    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] }

    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();
      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; }

          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

Re: Sessions, Perl and MySQL
by runrig (Abbot) on Mar 30, 2003 at 07:19 UTC
    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,
    If you don't do what?
    ...then it just adds them or removes them depending on the action, but it don't work until they Next login.
    I would find out exactly where this script is getting hung up (you say it times out, you mean it gets hung up somewhere?). And CWT::Site_DB is not a CPAN module (I assume maybe you got it from the book?), so its hard to tell what exactly is happening there. Is this vanilla MySQL with no transactions? Otherwise, I might say there's a deadlock, but I assume its not the new fangled MySQL with extra added transaction flavor. 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? Do the 'attr' and 'clear_attr' methods update the database immediately?

    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).

Re: Sessions, Perl and MySQL
by dws (Chancellor) on Mar 30, 2003 at 17:28 UTC
    Do you see a reason why this would not work?

    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.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://246699]
Approved by Enlil
Front-paged by Courage
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others chanting in the Monastery: (6)
As of 2024-03-19 03:07 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found