#!/usr/bin/perl

use strict;
use warnings;
use CGI::Session;

sub login {
    my ($dbh) = @_;

    my ($email)    = $dbh->quote( my $CGI->param('email') || undef );
    my ($password) = $dbh->quote( $CGI->param('password') || undef );
    my ($loginSessionID) = $CGI->param('loginSessionID') || undef;

    eval {
        do {
            my $tmp = "SELECT\n
            id\nFROM\n
            users\nWHERE\n
            LOWER(email) =
            LOWER($email)\n
            AND password = $password\n";

            my $sth = &return_query( $dbh, $tmp );
            my (@row) = $sth->fetchrow_array;
            my %result;
            $result{'userID'} = undef;
            if (@row) {
                $result{'userID'} = $row[0];
                CGI::Session->name(
                    my $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'n
+ame'}
                      || 'CGISESSID' );
                my $SESSION = CGI::Session->load(
                    'driver:MySQL',
                    $CGI || $loginSessionID,
                    {
                        DataSource => join( ':',
                            $CONFIG->{'mysql'}{'DSN'},
                            $CONFIG->{'mysql'}{'Collection'} ),
                        TableName   => 'sessions',
                        IdColName   => 'id',
                        DataColName => 'data',
                        Handle      => $dbh,
                    }
                ) || die( CGI::Session->errstr ) . "\n";

                if ( $SESSION->is_expired ) {
                    $SESSION->delete();
                    $SESSION->flush();
                }
                if ( $SESSION->is_empty ) {
                    $SESSION = CGI::Session->new();
                }
                $result{loginSessionID} = $SESSION->id();
                $SESSION->param( 'userID', $result{'userID'} );
                $SESSION->expire(
                    $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{expir
+e}
                      || '+30m' );
                my %COOKIES;
                $COOKIES{'loginSession'} = $CGI->cookie(
                    -name =>
                      $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'na
+me'},
                    -value => $SESSION->id(),
                    -expires =>
                      $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'ex
+pire'}
                      || "+30m",
                    -path =>
                      $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'pa
+th'}
                      || "/",
                    -domain =>
                      $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'do
+main'}
                      || "",
                    -secure =>
                      $CONFIG->{'userSession'}{'cookies'}{'LOGIN'}{'se
+cure'}
                      || 0,
                );
                $SESSION->flush();
            }
        };
        my %result;
        if ($@) {
            push @{ $result{'error'}; }, $@;
        }
        return encode_json( \%result );
    }
}
[download]

Some suboptimal parts in your code:

    my ($email)    = $dbh->quote($CGI->param('email')    || undef);
    my ($password) = $dbh->quote($CGI->param('password') || undef);
[download]

First, what is the purpose of $CGI->param('...') || undef? If the parameteter is missing, param() will return undef in scalar context, or an empty list in list context. But: If that parameter is empty or '0', you will replace it with undef. Why do you want to do that?

Second, calling DBIs quote() method is a sure sign that something is wrong with your code. No code outside DBDs should need to call the quote() method.

        my $tmp = <<EOF;
SELECT
    id
FROM
    users
WHERE
    LOWER(email) = LOWER($email)
        AND password = $password
EOF

        my $sth = &return_query($dbh, $tmp);
[download]

Here is what is wrong with your code. At least, you used $dbh->quote(), so you are not vulnerable to SQL injection here. But still, you construct a different SQL statement for each request, preventing any caching and forcing redundant work. See Re: Counting rows Sqlite and Re^2: Massive Memory Leak.

Re-think your return_query() function. I guess it is nothing more than a wrapper for $dbh->prepare($tmp) and $dbh->execute(). Drop that, or extend that wrapper to allow placeholder values being passed to execute(). Also, when you run your code from a persistant environment (FastCGI, mod_perl, Apache::Registry), switch to prepare_cached().

Also: Why do you use the ancient perl 4 syntax to call return_query()? Are you aware that that syntax is not only more typing, but also bypasses some error checks? Drop that ampersand, here and everywhere else.

Next problem: You seem to store passwords in plain text in your database. This is a really stupid idea. Use a salted hash instead, compare salt from the database + password from user input with the hash in the database.

        my @row = $sth->fetchrow_array;

        $result{userID} = undef;

        if (@row) {
            $result{userID}         = $row[0];
[download]

Once you get some non-empty result from the database, you assume that there is only one row with the result. How can you be that sure? You compare the email using the database's LOWER function. Does the database make sure that LOWER(email) is unique for the entire users table?

   if ($@) {
        push(@{ $result{error} }, $@);
    }

    return encode_json(\%result);
[download]

%result seems to be a global variable. Why isn't it a local variable (i.e. my %result?

Why do you return %result JSON-encoded? With a local variable %result (my %result), you could simply return a reference to %result: return \%result.

Not your fault: CGI::Session has a scary ID generator that may bite you one day.

Alexander

Hi Alexander,

Thanks for your comments. A few explanations:

The reason I do the $CGI->param(...) || undef is exactly for the case where the parameters are zero or empty. Those values are "replaced" by undef which the DBI "quote" turns into "NULL". Very few columns in my database tables allow NULL values.

Actually I do NOT store passwords in plain text. The password strings that pass through here were encoded in the browser's javascript before being sent to this app. I am comparing encoded strings, not plain text. The code I am writing will perhaps be used in other locations once I am done and I can not absolutely force future clients to use SSL connections so I encrypt what i can and assume a non-SSL'd channel.

The reason I do the $CGI->param(...) || undef is exactly for the case where the parameters are zero or empty. Those values are "replaced" by undef which the DBI "quote" turns into "NULL". Very few columns in my database tables allow NULL values.

Well, I would add a short comment to the code, explaining that trick (it really looks like a bug). Simply because my future self could get very angry spending hours to debug such tricks, invent a time machine, return back in time to now, and hurt me. ;-)

Actually I do NOT store passwords in plain text. The password strings that pass through here were encoded in the browser's javascript before being sent to this app. I am comparing encoded strings, not plain text. The code I am writing will perhaps be used in other locations once I am done and I can not absolutely force future clients to use SSL connections so I encrypt what i can and assume a non-SSL'd channel.

Sorry, but that won't help much. Your login form transmits an email address and an encrypted (that's what I read from your term "encoded") password back to the server. Together with the usual IP, TCP and HTTP headers, that should easily fit into a single package. Easy to grab, easy to decode. Essentially a no-brainer with Firesheep or Wireshark. I don't even have to break your Javascript-based encryption. It is sufficient to re-transmit the encrypted password (http://en.wikipedia.org/wiki/Replay_attack), because it is identical for every login until the user changes his password.

Even if you would protect the remainder of the login mechanism against replay attacks, all I need to to with a grabbed pair of email address plus encrypted password is to fill in both into the login form I get when I want to log in into your service. Firebug can easily be used to modify the form on-the-fly in a way that it transmits the encrypted password as pasted into the form.

This "solution" offers only a tiny bit more security than a plain-text password. Your site is vulnerable, but until the attacker can decrypt the password, the attacker can not use it on other sites.

Unfortunately, your encryption function was transmitted before, when the user's browser loaded the login form and the associated Javascript code. So if you really encrypt the password (instead of processing it through a hash function), you have handed out the encryption algorithm and the encryption key to the attacker. And that's a very bad idea, because now the attacker has all components he need to calculate the plaintext password.

If you really just encoded the password (instead of encrypting or hashing it), like for example by transfering it to its base64, base36, hex, or URL representation, things become dramatically easier for the attacker, because he just has to change the encoding back, no crypto involved at all.

If you can't use SSL, think about Challenge-response authentication, but be aware that you need to protect the shared secret (password or password hash) with that.

A Zero-knowledge password proof looks really promising, but I don't know if it can be easily implemented in a HTTP context.

Perhaps it is easier to rely on SSL to protect the plain text password transfered through the SSL layer, and to use salted hashes in the database.

Alexander

--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)