Criticize and correct my login script please

[Xargo]

Edited code [7/1/2006 - evening]
Edited code [8/1/2006 - 17:04]
Edited code [9/1/2006 - 20:29]

Good day,

some time ago it seemed interesting to learn a dynamic web application language, because I was still making all my sites in HTML. I first tried ASP, but when I almost got the hang of it I suddenly noticed I could have been better learning ASP.NET (with Microsoft's NET framework). Anyway, I choosed PHP then, mainly because it looked more simple, and you didn't have to choose another language like c# or vb to program the application in.

Fortunately I'm already experienced with some other programming languages, and it was quite simple. In a weekend I learnt php and rewrote the site of my mmorpg (from 70+ files to one index.php (MySQL)).

But that was quite easy, it was just some MySQL managing, that's it. Anyway, now I have to make a login script with sessions for a site of a game clan. I'm capable of doing that, but the problem is the security, I'm not experienced with that. That's why I want you guys to check and correct my script, it can be possible it have to be rewritten totally .

Anyway, here it is:

[Note]: Unfortunately there is no other 'encryption' used except from md5 (that's actually a hashing algorithm). I noticed PHP only has the crypt function as standard, and unfortunately I found out that's also just a hashing script. As you can see I tried some with mcrypt, a seperated library, but unfortunately that lib is not standard and also not supported by my host.

[Note II]: You will also notice there is no DOCTYPE at the beginning of the html pages. Anyway, when I got my php login script ready I will rewrite it to xhtml and add a DOCTYPE.

[Note III]: Some html code is not used correctly, for example I've written <br> instead of <br /> a few times. Well, I just want to say I'm aware of that, I'll improve my code when I got my final login script.

[Note IV, I made this red because I think I really did this wrong.]: Notice my way of securing. What I do is this: when he logs in and it's correct I store three session variables. One md5 hash of the username + hashed password, and two variables containing the username (not hashed because I will need it later) and the password hashed. When he enters another page he takes the two session variables with the username and hashed password, makes a new hash of it and compares it to the hash that was already stored when he logged in.

Offline

Now you've read this script you will probably say, "What the hell is that for a way of securing", and I know, it's a stupid way and certainly not secure. Anyway, that's why I asked you guys to help me.

I also read some about the mysql_real_escape_string() function, about html or mysql injection. Anyway, it was all quite unclear for me so I didn't add it to my script yet. Is that needed? What does it do exactly? It replaces some certain characters so injection is not possible? Something like that?

[duckax]

You might want to check out session_regenerate_id(). Gives the user a new session id before login to make sure that the current one is not planted by some hacker.
http://www.php.net/session_regenerate_id

mysql_real_escape_string() is usually not needed in a login script. However, it will be needed when you let your user enter their password, username, etc into your database. Simply escape everything that you put into your DB.

You will also need to know about the evil magic qoutes.
http://www.webmasterstop.com/63.html

Hope this helps.

[Xargo]

You might want to check out session_regenerate_id(). Gives the user a new session id before login to make sure that the current one is not planted by some hacker.
http://www.php.net/session_regenerate_id

mysql_real_escape_string() is usually not needed in a login script. However, it will be needed when you let your user enter their password, username, etc into your database. Simply escape everything that you put into your DB.

You will also need to know about the evil magic qoutes.
http://www.webmasterstop.com/63.html

Hope this helps.

Great, I will add session_regenerate_id() to the code of my first post. Please say it if I add it wrong.

I don't know if I need something with the magic quotes. I always read much about it, but actually I never ever got problems with those errors by not using these functions with the quotes etc. And I only allow the users to use letters and ciphers anyway. Or isn't that a good idea for a password?

I also added another note to the first post.

[lvismer]

I am not sure if you have magic quotes on or off but I do notice that $user is not validated,

An extreme example, please bear with me

if I make $user in the form

0";delete from go_admins where 1="1

the resulting query becomes

SELECT pass FROM go_admins WHERE user="0";delete from go_admins where 1="1"

not really what you wanted.

You might also want to consider creating a Hashed Message Authentication Code for the password using your own key aswell. Something like the following would be be better and more secure:

function hmacMD5($data, $key)
    {
        // HMAC(Data) = Hash(SecretKey, Hash(SecretKey, Data))
        $block_size = 64; // byte length for md5
        if ( strlen($key) > $block_size )
            $key = pack("H*", md5($key));
        // Fill the rest of the key with NULL if shorter than block_size
        $key  = str_pad($key, $block_size, chr(0x00));
        // Create some padded vars to Xor with our key
        $ipad = str_pad('', $block_size, chr(0x36));
        $opad = str_pad('', $block_size, chr(0x5C));
        $k_ipad = $ipad ^ $key;
        $k_opad = $opad ^ $key;
        return md5($k_opad  . pack("H*", md5($k_ipad . $data)));
    }

$encryptedPassword = hmacMD5('mypassword', 'mySeCret'); 

You might also consider this to be a little over the top

--
lv

[Xargo]

I am not sure if you have magic quotes on or off but I do notice that $user is not validated,

An extreme example, please bear with me

if I make $user in the form

0";delete from go_admins where 1="1

the resulting query becomes

SELECT pass FROM go_admins WHERE user="0";delete from go_admins where 1="1"

not really what you wanted.

Omg damn, I'm so glad you saw that. Never thought about that. How can I avoid that? Make a string length limit? Or not allow any characters except from numbers and letters? Or maybe better, just put the username between to "'s in the MySQL query?

You might also want to consider creating a Hashed Message Authentication Code for the password using your own key aswell. Something like the following would be be better and more secure:

function hmacMD5($data, $key)
    {
        // HMAC(Data) = Hash(SecretKey, Hash(SecretKey, Data))
        $block_size = 64; // byte length for md5
        if ( strlen($key) > $block_size )
            $key = pack("H*", md5($key));
        // Fill the rest of the key with NULL if shorter than block_size
        $key  = str_pad($key, $block_size, chr(0x00));
        // Create some padded vars to Xor with our key
        $ipad = str_pad('', $block_size, chr(0x36));
        $opad = str_pad('', $block_size, chr(0x5C));
        $k_ipad = $ipad ^ $key;
        $k_opad = $opad ^ $key;
        return md5($k_opad  . pack("H*", md5($k_ipad . $data)));
    }

$encryptedPassword = hmacMD5('mypassword', 'mySeCret'); 

You might also consider this to be a little over the top

--
lv

Hmmmm.... Has it any use to make a function that hashes using md5 with a key? Md5 is not reversable anyway.

[lvismer]

Hmmmm.... Has it any use to make a function that hashes using md5 with a key? Md5 is not reversable anyway.

If for some reason the session is highjacked one can use brute force to hack away at the password as with MD5 alone only data integrity is assured.

A MAC verifies the authenticity and the integrity of the data. Basically the hmacMD5 function creates a hashed message authentication code (HMAC) which is more than just using md5 with a key. A digest algorithm (like md5) only guarentees the integrity. If someone manages to alter the current MD5 hash with their own md5 version you would not be able to catch that unless you check the username and password combination every time.

As an example, storing the username and an HMAC of the username in a session one would not need to store the password in the session variable at all.

--
lv

[Xargo]

If for some reason the session is highjacked one can use brute force to hack away at the password as with MD5 alone only data integrity is assured.

A MAC verifies the authenticity and the integrity of the data. Basically the hmacMD5 function creates a hashed message authentication code (HMAC) which is more than just using md5 with a key. A digest algorithm (like md5) only guarentees the integrity. If someone manages to alter the current MD5 hash with their own md5 version you would not be able to catch that unless you check the username and password combination every time.

As an example, storing the username and an HMAC of the username in a session one would not need to store the password in the session variable at all.

--
lv

So if I understand it right I only have to store the username and the HMAC of the password in the session? No other md5 hashes anymore? And what do I have to do to validate the user after he logged in then?

Oh btw, you missed my other question. How do I validate the username so it doesn't get such a query to ruin my database? Is it right to put them between "'s when I query the database? Or can I stop this with that magic quotes thing?

[lvismer]

So if I understand it right I only have to store the username and the HMAC of the password in the session? No other md5 hashes anymore? And what do I have to do to validate the user after he logged in then?

Well a simpler way would be to store the username and the hmac of the username. This way one would not need to store sensitive info like a password. To validate a user session one would basically just recompute the username hmac and check it with the session version. In other words your session can contain the following after an initial valid login with the username filtered,

$_SESSION['user'] = $filtered['user'];
$_SESSION['userhash'] = hmacMD5[$filtered['user']]; 

Oh btw, you missed my other question. How do I validate the username so it doesn't get such a query to ruin my database? Is it right to put them between "'s when I query the database? Or can I stop this with that magic quotes thing?

One way is to filter the username and use the filtered value in your script.

// Usernames only contain alpha's

$filtered = array();

if ( ctype_alpha($_POST['user']) ) {
    $filtered['user'] = $_POST['user'];
}

// then later on ..

$result = mysql_query("SELECT pass FROM go_admins WHERE user=\"" . $filtered['user'] . "\"", $link); 

--
lv

[Xargo]

Well a simpler way would be to store the username and the hmac of the username. This way one would not need to store sensitive info like a password. To validate a user session one would basically just recompute the username hmac and check it with the session version. In other words your session can contain the following after an initial valid login with the username filtered,

$_SESSION['user'] = $filtered['user'];
$_SESSION['userhash'] = hmacMD5[$filtered['user']]; 

I get it now! I better read your previous post a bit more concentrated. I'll add it to the code of my first post.

One way is to filter the username and use the filtered value in your script.

// Usernames only contain alpha's

$filtered = array();

if ( ctype_alpha($_POST['user']) ) {
    $filtered['user'] = $_POST['user'];
}

// then later on ..

$result = mysql_query("SELECT pass FROM go_admins WHERE user=\"" . $filtered['user'] . "\"", $link); 

--
lv

Great great! I'll add this to the code of the first post too. ^^ Didn't know there was a ctype_alpha() function with PHP.

[lvismer]

Didn't know there was a ctype_alpha() function with PHP.

Should work with php 4.0.4 and up, check your host support. Obviously I presumed that your username only contains alpha characters If not you might need to use preg_match or something similar.

--
lv

[Xargo]

Should work with php 4.0.4 and up, check your host support. Obviously I presumed that your username only contains alpha characters If not you might need to use preg_match or something similar.

--
lv

To be honest I got a rather irritating host. Just after I registered they disabled the register feature and upgraded from 111mb to 777mb and 1111mb bandwidth to 7777mb bandwidth. That's nice, but their site is terrible. Only four pages work: their homepage, the control panel, the mysql panel and the file manager. The links to the contact, information, faq and such don't work at all. I use 777mb.com. So I also don't know which version of php they use. I'm currently still using it yet, someone knows a better host?

Omg I reread my replies and saw I sometimes really answered quite stupid, I apologise for that I'm also playing a mmorpg at the moment, I get a bit distracted by that.

I updated my code now btw, is it a nice login script now? Or are there still measures to be taken.

[lvismer]

To be honest I got a rather irritating host. Just after I registered they disabled the register feature and upgraded from 111mb to 777mb and 1111mb bandwidth to 7777mb bandwidth. That's nice, but their site is terrible. Only four pages work: their homepage, the control panel, the mysql panel and the file manager. The links to the contact, information, faq and such don't work at all. I use 777mb.com. So I also don't know which version of php they use. I'm currently still using it yet, someone knows a better host?

What does a script with phpinfo() in tell you?

<?php
phpinfo();
?>

I updated my code now btw, is it a nice login script now? Or are there still measures to be taken.

Ultimately only you will be able to say if the login script does what you want it to, two small things I notice,

// 1. $user is never set so you will not reach
    if ($user!="") 
    { 
        //COMMENT: New code, start 
        if (ctype_alpha($_POST['user'])) 
        { 
            $user = $_POST['user']; 
        } 
        else 
        { 
            die("Incorrect username or password."); 
        }

// you can change it to this after the login page is detected
    $user = '';
    if (ctype_alpha($_POST['user']))
    {
        $user = $_POST['user'];
    }
    else
    {
        die("Incorrect username or password.");
    }
    if ( $user != '' )
    {
        ....

// 2. to be consistent should you not use die here aswell?
        if (mysql_num_rows($result) < 1)
        { 
            echo "Incorrect username or password."; 
        } 

PS: I'm signing off now, rather late on my side

Good luck

--
lv

[lvismer]

Some other comments that jump to mind after the initial things,

- you would need to set a hidden field in the login form, to carry the page variable if you choose to use this single script.
- you should change $page = $_GET['page'] to something like $page = $_REQUEST['page'] to support GET and POST
- you also might want to group functionality into functions, perhaps a login function, a function to check if a valid login session exists and some seperate functions for all the pages would be a start

--
lv

[Xargo]

What does a script with phpinfo() in tell you?

Oh yes of course, why didn't I think of that. PHP Version 4.4.1, so it supports both ctype_alpha() and session_regenerate_id(). I'm wondering why they didn't upgrade to PHP 5.x yet.

// 1. $user is never set so you will not reach

Omg yes of course, I had to know that. It works anyway btw, because PHP automatically redirects the POST and GET variables to normal variables. Still I will change it because people could be able to use the URL to init a username otherwise. I will change it now.

// 2. to be consistent should you not use die here aswell?
        if (mysql_num_rows($result) < 1)
        { 
            echo "Incorrect username or password."; 
        } 

Yes indeed, I actually already changed that in my code on my PC, but forgot to adjust it here too. I will change it now.

- you would need to set a hidden field in the login form, to carry the page variable if you choose to use this single script.

Why should I do that? I always find it weird people do that because you can also check if the page is submitted just by checking if the first variable is empty or not. Or is there another reason?

- you should change $page = $_GET['page'] to something like $page = $_REQUEST['page'] to support GET and POST

I don't think that's needed because I will never drag a page variable using the POST functions because I want my users to have the whole url. Otherwise they can only eg. bookmark the first page and when refreshing they go to the first page again.

- you also might want to group functionality into functions, perhaps a login function, a function to check if a valid login session exists and some seperate functions for all the pages would be a start

I could do that yes, but that hasn't a high priority at the moment.

Edit: Edited code in the first post.

[lvismer]

Omg yes of course, I had to know that. It works anyway btw, because PHP automatically redirects the POST and GET variables to normal variables. Still I will change it because people could be able to use the URL to init a username otherwise. I will change it now.

Register_globals are evil and one should be careful of this. You should actually code with it switched off using a .htaccess if the host company has it turned on.

php_flag register_globals off 

Why should I do that? I always find it weird people do that because you can also check if the page is submitted just by checking if the first variable is empty or not. Or is there another reason?

In the form PHP_SELF is only available if the register_globals are turned on which I consider bad practise. Rather use $_SERVER['PHP_SELF'].

The way I see it, the single script does the login, testing and displaying of the initial form. If the form now calls itself and the method is post, once the scripts gets submitted you will always get the message "This is the start page." as you are referencing the _GET variable but the form was submitted using the POST method. It will work by chance if the action in the form is empty btw, which will by chance result in the _GET and _POST arrays being populated with info.

--
lv

[Xargo]

Register_globals are evil and one should be careful of this. You should actually code with it switched off using a .htaccess if the host company has it turned on.

php_flag register_globals off 

I know register_globals are evil, but I never worked with a .htaccess before. I'll try it now and hope my host supports it.

In the form PHP_SELF is only available if the register_globals are turned on which I consider bad practise. Rather use $_SERVER['PHP_SELF'].

Didn't know that also included the register_globals. I'll change that too when I use the .htaccess.

The way I see it, the single script does the login, testing and displaying of the initial form. If the form now calls itself and the method is post, once the scripts gets submitted you will always get the message "This is the start page." as you are referencing the _GET variable but the form was submitted using the POST method. It will work by chance if the action in the form is empty btw, which will by chance result in the _GET and _POST arrays being populated with info.

Ah no, I make the form as action to $PHP_SELF, what seems to include the current GET variables also. I first thought that wasn't going to work, but if you are currently at the url login.php?page=test and you make a form going to $PHP_SELF it goes to login.php?page=test, not to login.php.

Edit: It seems my host doesn't support .htaccess. I first tried to upload .htaccess but he refused that, then I tried htaccess.txt but he also refused that. Then I tried a.htaccess.txt, he allowed that, but then I tried to rename it to .htaccess on the server itself, but he also refused that. It seems he doesn't like .htaccess.

Edit II: I edited the code of the first post by changing the variable $PHP_SELF to $_SERVER['PHP_SELF'], not that the register_globals are disabled (I don't even got the possibility, my host doesn't support it), but I just like that more.

Edit III: It seems you are right. It will always return to the start page again. But guess what I noticed? If you use the variable $PHP_SELF it works perfect! Only if you use the non-register_globals variable $SERVER['PHP_SELF'] it doesn't work and goes to the start page. It seems the php_self variable of the register_globals on and off are not the same... Anyway, can't I just keep $PHP_SELF? I don't see any security danger as long as I use all other code with proper non-register_globals variable except from that one.

Also edited some other common mistakes like forgetting a semicolon.

[lvismer]

It seems you are right. It will always return to the start page again. But guess what I noticed? If you use the variable $PHP_SELF it works perfect! Only if you use the non-register_globals variable $SERVER['PHP_SELF'] it doesn't work and goes to the start page. It seems the php_self variable of the register_globals on and off are not the same... Anyway, can't I just keep $PHP_SELF? I don't see any security danger as long as I use all other code with proper non-register_globals variable except from that one.

I did mention this in one of the previous posts, using $PHP_SELF I am almost certain that if you view the source (html source in the browser) of your form the action tag will be empty. What happens is the form action will actually be used from the original referer, which includes the _GET variable page=login, because the action in the form is empty. So after the form gets submitted you will have $_GET and $_POST variables set, which in my opinion is messy.

Bite the bullet and add the following using the correct $_SERVER['PHP_SELF'] variable,

<html> 
    <head> 
    <title>Login test</title> 
    <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> 
    </head> 
    <body> 
    <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
    <input type="hidden" name="page" value="login"><br>
    <input type="text" name="user"><br> 
    <input type="password" name="pass"><br> 
    <input type="submit" value="Login"> 
    </form> 
    </body> 
    </html>

Come on, I know you can do it ... , use the hidden field,

--
lv

[Xargo]

I did mention this in one of the previous posts, using $PHP_SELF I am almost certain that if you view the source (html source in the browser) of your form the action tag will be empty. What happens is the form action will actually be used from the original referer, which includes the _GET variable page=login, because the action in the form is empty. So after the form gets submitted you will have $_GET and $_POST variables set, which in my opinion is messy.

I don't see why you find POST and GET variables set messy. The point is that I use MySQL for the content of my sites, my site is only one page: index.php. I always have to use GET variables. And if I use a login form I certainly need to use POST too, so I can't avoid using both at the same time.

Sorry for my late reply by the way, but it's school again at the moment. :'(

[bokehman]

Get a proper host!

[Xargo]

Get a proper host!

Recommend me one then.

[DewChugr]

Recommend me one then.

mediatemple

[bokehman]

Recommend me one then.

The truth is if they are good they are usually expensive. Personally I pay for a 24 hour connection and have my own server. I have my own DNS, mail and web server. Every time I add another domain it costs nothing extra.

[Xargo]

I'm sorry, I won't ever pay for a web hosting if there are so many free web hosts out there. I'm already paying for a com domain and in the future I'll have to pay for an mmorpg server too, that's more than enough.

Btw, lvismer, can you please check my answer?

I don't see why you find POST and GET variables set messy. The point is that I use MySQL for the content of my sites, my site is only one page: index.php. I always have to use GET variables. And if I use a login form I certainly need to use POST too, so I can't avoid using both at the same time.

[bokehman]

The point is that I use MySQL for the content of my sites, my site is only one page: index.php

Why don't you use the file system? Also you can still have proper URLs without a query string even if you do use a DB.

[Xargo]

Why don't you use the file system? Also you can still have proper URLs without a query string even if you do use a DB.

Do you mean just different files for every page or files containing the content that get displayed by the index.php file? Anyway, I won't use both of them, first of all I really like the nice way of putting a whole site in one file and a database and I also want to make a dynamic site so the people don't have to ask me to update the site all the time, and secondly I'm not experienced with file reading with PHP, although it's probably not hard.

What do you mean with the second statement? The only GET variable is the page var, defining which page the person visits.