Show Me the Flaw(s) in this Login Script

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
drayarms
Forum Contributor
Posts: 134
Joined: Fri Dec 31, 2010 5:11 pm

Show Me the Flaw(s) in this Login Script

Post by drayarms »

I put together this login script, complete with a section to check for and report errors if the user failed to enter any one of the fields in the login form. The script works perfectly fine when I exclude the error section, but when I include the error section, nothing works. I get redirected to the login failed page (see script) both when I enter the the right username/password combination and when I deliberately mis spell them. I get the same results when I deliberately forget to fill out one of the fields, instead of the appropriate error messages. so who can tell me what I left out or didn't do right in this script?

Code: Select all


<?php


//address error handling

ini_set ('display_errors', 1);
error_reporting (E_ALL & ~E_NOTICE);

//Turn on output buffering. Allows for headers to be called anywhere on script. See pg228 Ulman.
ob_start();



if (isset($_POST['submitted'])) {
 
    $errors = array();


// Connect to the database.

        require_once ('config.php');


// Initialize a session:
session_start();


//Check for errors.


//Check to make sure they entered their first name.

       if (empty($_POST['username'])) {  

       $errors[] = '<font color="red">Please enter your user name.</font>';

    } else {

	$username = mysql_real_escape_string($_POST['username']);        

   }



//Check to make sure they entered their password.

       if (empty($_POST['password'])) {  

       $errors[] = '<font color="red">Please enter your password.</font>';

    } else {

	$username = mysql_real_escape_string($_POST['password']);        

   }



//Query the database. The variable assigned to the post username should match the named attribute of username of login form. same for the password.
		$sql="SELECT * FROM members WHERE username='$username' AND password='$password' AND activation_status ='1'"; 
	
		$result=mysql_query($sql);
		
		// Replace counting function based on database you are using.
		$count=mysql_num_rows($result);

		// If result matched $myusername and $mypassword, table row must be 1 
		if($count==1){

  
  		// Register username, firstname and redirect to file 
                        
			session_regenerate_id();
			$member = mysql_fetch_assoc($result); //Define a member array that holds result values.
			$_SESSION['id'] = $member['member_id'];
			$_SESSION['firstname'] = $member['firstname'];
			$_SESSION['lastname'] = $member['lastname'];
			session_write_close();
			
			header("location: member.php");
			exit();
		}else {
			//Login failed
			header("location: login_failed.php");
			exit();
                }






//Display error messages.


} else {// if errors array is not empty

        echo '<h3>Error!</h3>
        The following error(s) occured:<br />';
       
        foreach ($errors as $msg) {
            echo " - <font color=\"red\">$msg</font><br />\n";
        }
    
}




?>
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: Show Me the Flaw(s) in this Login Script

Post by Darhazer »

Code: Select all

 $username = mysql_real_escape_string($_POST['password']);     
You are overwritting the username variable.
Maybe you want to display error message if username or password is empty before quering the databse.
Other than this, I don't see any problems with the script
incubi
Forum Contributor
Posts: 119
Joined: Mon Dec 07, 2009 1:47 pm

Re: Show Me the Flaw(s) in this Login Script

Post by incubi »

Hi
I'm not sure this ( $password ) will work it depends on your Global var settings.
$sql="SELECT * FROM members WHERE username='$username' AND password='$password' AND activation_status ='1'";

Also I would use some encryption like md5($password); Then check the encrypted pass in the DB.


Lee
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Re: Show Me the Flaw(s) in this Login Script

Post by pickle »

- You should call session_start() only after you've verified the person logged in successfully. Otherwise you'll be (potentially) creating a new session for each login attempt.

- in addition to checking if $_POST['username'] is empty, you should check if it even exists with isset()
- Your error array shouldn't contain any formatting (ie the font tags). You're duplicating that formatting below when you output the errors anyway
- Don't use <font> tags. Update your code & use CSS
- you should potentially strip slashes if magic quotes are on:

Code: Select all

$username = (get_magic_quotes_gpc()) ? stripslashes($_POST['username']) : $_POST['username'];
$username = mysql_real_escape_string($username);
//same for the password 
- As mentioned, you're storing both the username and password in the $username variable
- Also mentioned, you're storing plain text passwords which is a HUGE security flaw. Hash it. Don't use MD5 though as it's easily breakable. Use SHA512.
- Good catch with the session_regenerate_id() - not a lot of people know to to do that. However, wouldn't be necessary if you only call session_start() there instead of higher up
- the Location: header should have the fully qualified URL, not just a filename. Most of the time it works, but some browsers may not like only having the filename.
- Your else() clause to display the errors will only be run if $_POST['submitted'] isn't set.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
drayarms
Forum Contributor
Posts: 134
Joined: Fri Dec 31, 2010 5:11 pm

Re: Show Me the Flaw(s) in this Login Script

Post by drayarms »

Thanks guys for pointing out the errors. As some of you pointed out, the main problem was the fact that I forgot to change the $username variable to $password in the error checking section. Also, I forgot to insert this statement if (empty($errors)) { just prior to querying the database. I know that it is advisable to encrypt the password, and to use CSS in setiting style. I was just trying to see if the login script works first before taking care of those little details. Thanks once again.
Post Reply