SitePoint Sponsor |
|
User Tag List
Results 1 to 25 of 38
-
Jan 7, 2006, 09:38 #1
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Criticize and correct my login script please
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?Last edited by Xargo; Jan 20, 2006 at 13:41.
-
Jan 7, 2006, 10:48 #2
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.
-
Jan 7, 2006, 12:25 #3
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by duckax
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.
-
Jan 7, 2006, 12:46 #4
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
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:
PHP Code: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');
--
lv
-
Jan 7, 2006, 13:13 #5
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
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?
Originally Posted by lvismer
-
Jan 7, 2006, 13:38 #6
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
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
-
Jan 7, 2006, 14:01 #7
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
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?
-
Jan 7, 2006, 14:22 #8
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
PHP Code:$_SESSION['user'] = $filtered['user'];
$_SESSION['userhash'] = hmacMD5[$filtered['user']];
Originally Posted by Xargo
PHP Code:// 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
-
Jan 7, 2006, 14:35 #9
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
I better read your previous post a bit more concentrated.
I'll add it to the code of my first post.
Originally Posted by lvismer
I'll add this to the code of the first post too. ^^ Didn't know there was a ctype_alpha() function with PHP.
-
Jan 7, 2006, 14:42 #10
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
If not you might need to use preg_match or something similar.
--
lv
-
Jan 7, 2006, 15:01 #11
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
Omg I reread my replies and saw I sometimes really answered quite stupid, I apologise for thatI'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.
-
Jan 7, 2006, 15:29 #12
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
PHP Code:<?php
phpinfo();
?>Originally Posted by Xargo
PHP Code:// 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.";
}
Good luck
--
lv
-
Jan 7, 2006, 16:00 #13
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
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
-
Jan 8, 2006, 10:00 #14
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
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.
Originally Posted by lvismer
Originally Posted by lvismer
I will change it now.
Originally Posted by lvismer
Originally Posted by lvismer
Originally Posted by lvismer
Edit: Edited code in the first post.
-
Jan 8, 2006, 10:58 #15
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
PHP Code:php_flag register_globals off
Originally Posted by Xargo
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
-
Jan 8, 2006, 14:13 #16
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
Originally Posted by lvismer
Originally Posted by lvismer
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.Last edited by Xargo; Jan 9, 2006 at 13:30.
-
Jan 9, 2006, 13:45 #17
- Join Date
- Aug 2005
- Location
- South Africa
- Posts
- 185
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
Bite the bullet and add the following using the correct $_SERVER['PHP_SELF'] variable,
PHP Code:<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>, use the hidden field,
--
lv
-
Jan 11, 2006, 09:35 #18
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by lvismer
Sorry for my late reply by the way, but it's school again at the moment. :'(
-
Jan 11, 2006, 10:26 #19
Get a proper host!
Location: Alicante (Spain)... Hot and Sunny...
Texas Holdem Poker Probability Calculator | DNS test
Avatars | English Spanish Translation | CAPTCHA with audio
Email | PHP scripts | Cruft free domain names | MD5 Cracker
-
Jan 11, 2006, 11:44 #20
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by bokehman
-
Jan 11, 2006, 13:30 #21
- Join Date
- Sep 2005
- Location
- Illinois
- Posts
- 189
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by Xargo
-
Jan 11, 2006, 13:43 #22
Originally Posted by Xargo
Location: Alicante (Spain)... Hot and Sunny...
Texas Holdem Poker Probability Calculator | DNS test
Avatars | English Spanish Translation | CAPTCHA with audio
Email | PHP scripts | Cruft free domain names | MD5 Cracker
-
Jan 13, 2006, 09:55 #23
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
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?
Originally Posted by Xargo
-
Jan 13, 2006, 10:16 #24
Originally Posted by Xargo
Location: Alicante (Spain)... Hot and Sunny...
Texas Holdem Poker Probability Calculator | DNS test
Avatars | English Spanish Translation | CAPTCHA with audio
Email | PHP scripts | Cruft free domain names | MD5 Cracker
-
Jan 13, 2006, 10:42 #25
- Join Date
- Sep 2005
- Location
- Belgium
- Posts
- 20
- Mentioned
- 0 Post(s)
- Tagged
- 0 Thread(s)
Originally Posted by bokehman
What do you mean with the second statement? The only GET variable is the page var, defining which page the person visits.
Bookmarks