Help clean up some code please :)

[sean04]

Hey! So I was wondering if anyone has a bit of time to take a look at the following and maybe fix it up a bit? I just feel like it's all over and maybe it needs some tidying

 

 

<?php
	include "config.php";

       // variable to check if form was sent
	$form_sent = $_POST['form_sent'];
	// assume form was completed properly
	$valid = true;
	//initialize an array to contain list of errors
	$errors = array();
	//capture form data if form has been sent
	if($form_sent == 1){

	$Email = $_POST['Email'];
	$Password = $_POST['Password'];
	$checkInfo = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
	$checkUser = mysql_num_rows($checkInfo);
	$checkUserInfo = mysql_fetch_array($checkInfo);

	//check to see if all fields are complete

	if($Email == ''){
		$errors[] = "Email";
	}
	if($Password == ''){
		$errors[] = "Password";
	}

	if($Email != '' && $Password != ''){


	if($checkUser == '0') {
            $errors[] = "Username or Password is incorrect";        
	}

	if($checkUserInfo[userlevel] == 1) {
            $errors[] = "This account has not yet been verified. Please check your email";
        }

	$Password = $Password;

	if ($Password != $checkUserInfo[Password]) {
		$errors[] = "Username or Password is incorrect";
	  }
	}

	//check to see if there were errors
	if(count($errors) > 0){
		$valid = false;
	}else{

                $query = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
                //fetchs the sql
                $user = mysql_fetch_array($query);
                //sets the logged session
                $_SESSION['ID'] = "$user[iD]";
                $_SESSION['Password'] = "$user[Password]";
                echo "<meta http-equiv='Refresh' content='0; URL=profile.php'/>";
            
	  }
	}

//output the form only if they have not submitted OR if there are errors they need to fix
if($form_sent != 1 || $valid == false){

if($valid == false){
	echo
	'
	<div class = "error1">
	<p class="error">Oops. There was a problem with your form submission. Please review the
	following:</p>';

	echo'<ul class="error">';
	$numerrors = count($errors);
	for($i = 0; $i<$numerrors; $i++){
	echo'<li>'.$errors[$i].'</li>';
	}
	echo'</ul><br/></div>';
 }
}
    ?>

 

 

Thanks in advance! Any tips would be great to!

 

Sean

 

[KevinM1]

1. Don't repeat queries.  You already obtain the user info at the beginning.  There's no reason to run the same query to get the same data you already have.

 

2. Don't use 'or die' to handle errors.  It's a sloppy, inelegant way of doing it.  Not only that, but dying with mysql_error() gives anyone with bad intent an idea of how your db is structured.  It's a security risk.

 

3. Why aren't you checking the user's password in your db query?  From the looks of it, you're not saving it as a hashed value.  Bad idea.

 

4. Why aren't you validating or escaping user input at all?

 

5. Why are you passing the user's password to a session?

[sean04]

Thanks for the reply!

 

I'm actually using "or die" for testing purposes only, and I am securing the password.

 

I will make sure to remove the password from the users session if I do not need it then.

 

As for obtaining user info, one query uses mysql_num_rows and the other is mysql_fetch_array. Are you suggesting that I just use one (mysql_fetch_array)?

 

Also, maybe you could explain number 3? I was pretty sure I am checking the password.

 

Thanks again for the help!!

[KevinM1]

Regarding the query, you have:

 

$checkInfo = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
// ...
$checkUserInfo = mysql_fetch_array($checkInfo);

 

Then, later on, you have:

 

$query = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
$user = mysql_fetch_array($query);

 

Same query, same data.  It's redundant, and unnecessary.

 

For the password, your code leads me to believe that you don't store passwords as a hashed value within your db, which is a security flaw.  It's far easier for someone with bad intent to brute force a non-hashed password than a hashed one.  What you should do is something like:

 

$password = validate($_POST['password']); // where validate() runs the password through regex so it's legit

if (sha1($password) == $user['password'])) // or md5($password) if you saved it in the db with md5
{
   // do whatever
}

[sean04]

Thanks!

 

I see what you mean. I'll get rid of that

 

And as for the password I do use md5 and I save it in the database with that as well.

 

Another thing, where I have

if($checkUser == '0') {
         $errors[] = "Username or Password is incorrect";        
}

if ($Password != $checkUserInfo[Password]) {
         $errors[] = "Username or Password is incorrect";
}

 

Those could be combined right? Just the last few times I tried to combine those it didn't really work correctly. Putting those together would make more sense correct?

[ignace]

if($checkUser == '0' || $Password != $checkUserInfo[Password]) {
         $errors[] = "Username or Password is incorrect";
}

 

Should do the trick

[TeddyKiller]

$Password = $Password;

I've never written something so pointless? D:

 

This should be better, it's not perfect.. but yeah.

<?php
include "config.php";

// variable to check if form was sent
$form_sent = $_POST['form_sent'];

// assume form was completed properly
$valid = true;

//initialize an array to contain list of errors
$errors = array();

//capture form data if form has been sent
if($form_sent == 1){
    $Email = $_POST['Email'];
   $Password = $_POST['Password'];
   $checkInfo = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
   $checkUser = mysql_num_rows($checkInfo);
   $checkUserInfo = mysql_fetch_array($checkInfo);
   
   //check to see if all fields are complete
   if(empty($Email) || empty($Password)){
      $errors[] = "Empty Fields!";
   } else {
       //If there is a row in the database
       if($checkUser == 1){
          if(sha1($Password) != $checkUserInfo['Password']){
              //Or md5 or any other hash that your password is saved in the database as.
                $errors[] = "Password is incorrect";        
          }
         //If account has not been verified
           if($checkUserInfo['userlevel'] == 1) {
                $errors[] = "This account has not yet been verified. Please check your email";
            }
       //If there is no row in the database
      } else {
          $errors[] = "Email is incorrect";
      }
    }  
   
   //check to see if there were errors
   if(count($errors) > 0){
       $valid = false;
   } else {
        //sets the logged session
        $_SESSION['ID'] = "$checkUserInfo['ID']";
        $_SESSION['Password'] = "$checkUserInfo['Password']";
        echo "<meta http-equiv='Refresh' content='0; URL=profile.php'/>";
    }
}    

//output the form only if they have not submitted OR if there are errors they need to fix
if($form_sent != 1 || $valid == false){
    if($valid == false){
       echo'<div class = "error1">
      <p class="error">Oops. There was a problem with your form submission. Please review the following:</p>
      <ul class="error">';
      $numerrors = count($errors);
      for($i = 0; $i<$numerrors; $i++){
          echo'<li>'.$errors[$i].'</li>';
      }   
       echo'</ul><br/></div>';
   }
}
?>

[sean04]

Thank you both for the help!

 

ignace I will give that a shot. I had that included before but it didn't seem to work right but most likely it was because something else was messed up

 

and TeddyKiller thank you as well for your constructive criticism   I may not be a professional but I do realize $Password = $Password is completely pointless.

 

I will for sure take a look at what you have provided me though!

 

Thanks guys,

Sean

[TeddyKiller]

The errors are combined, theres no useless bits in there really. It all works fine. Instead of distinguishing if there is an incorrect username OR password, its seperate so that it's easier to find out wheres wrong

[sean04]

Yes I agree. It would work perfectly. One thing though. Would it not be a little bit of a security issue if someone knew the email was correct but the password wasn't? I would think giving someone an exact error would be risky. Outputting an error such as "Username or Password is incorrect" would seem a bit more secure because your not giving out the exact problem.

 

Thanks for the help!

[TeddyKiller]

I understand your concern. You can have it this way if you like.

 

<?php
include "config.php";

// variable to check if form was sent
$form_sent = $_POST['form_sent'];

// assume form was completed properly
$valid = true;

//initialize an array to contain list of errors
$errors = array();

//capture form data if form has been sent
if($form_sent == 1){
    $Email = $_POST['Email'];
   $Password = $_POST['Password'];
   $checkInfo = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());
   $checkUserInfo = mysql_fetch_array($checkInfo);
   
   //check to see if all fields are complete
   if(empty($Email) || empty($Password)){
      $errors[] = "Empty Fields!";
   } else {
        if(sha1($Password) != $checkUserInfo['Password'] || $Email != $checkUserInfo['Email']){
            //Or md5 or any other hash that your password is saved in the database as.
            $errors[] = "Username or Password is incorrect";        
        }
        //If account has not been verified
        if($checkUserInfo['userlevel'] == 1) {
            $errors[] = "This account has not yet been verified. Please check your email";
        }
    }  
   
   //check to see if there were errors
   if(count($errors) > 0){
       $valid = false;
   } else {
        //sets the logged session
        $_SESSION['ID'] = "$checkUserInfo['ID']";
        $_SESSION['Password'] = "$checkUserInfo['Password']";
        echo "<meta http-equiv='Refresh' content='0; URL=profile.php'/>";
    }
}    

//output the form only if they have not submitted OR if there are errors they need to fix
if($form_sent != 1 || $valid == false){
    if($valid == false){
       echo'<div class = "error1">
      <p class="error">Oops. There was a problem with your form submission. Please review the following:</p>
      <ul class="error">';
      $numerrors = count($errors);
      for($i = 0; $i<$numerrors; $i++){
          echo'<li>'.$errors[$i].'</li>';
      }   
       echo'</ul><br/></div>';
   }
}
?>

[sean04]

Yes exactly what I would do. ignace wrote the same thing. Thanks for clearing it up a bit though! looks great!

 

Thanks for the help!

[ignace]

I may not be a professional

 

Keep coming here and you soon will be My motto: learn from the mistakes of others as you can't make them all yourself Visiting these forums made me a wise man

[sean04]

Thanks a lot ignace  I appreciate that! I will definitely be back here again soon! 

[the182guy]

Your code is vulnerable to an SQL Injection attack, because you're putting raw user input directly into your SQL query.

 

Here

    $Email = $_POST['Email'];
   $Password = $_POST['Password'];
   $checkInfo = mysql_query("SELECT * FROM `user` WHERE `Email` = '$Email'") or die(mysql_error());

 

If I typed in this string into the email address textbox and submitted your form I could inject any SQL of my choosing, for example:

 

';DROP TABLE user;SELECT * FROM user where email <> '

 

Could potentially delete your users table, or simple select any user from the table.

 

The other problem is that you show the mysql error if there is one, which means the attacker could run a failed query and he would be shown the full original query in the mysql error, for example he would then know the name of your table, and that you're selecting all.

[sean04]

Thanks for the reply!

 

I have noted that issue and encrypted the password. As for the "or die", I'm just using that for testing purposes only.

 

Thanks for the information!

[the182guy]

Thanks for the reply!

 

I have noted that issue and encrypted the password. As for the "or die", I'm just using that for testing purposes only.

 

Thanks for the information!

 

I think you missed my point, the problem is the $Email variable goes directly into your SQL query without being escaped. See mysql_real_escape_string()

[sean04]

Ah ok I see sorry about that. I checked it out. As of now I have this:

 

$Email = htmlspecialchars(addslashes($_POST['Email']));

 

Would that be the same as what you recommended? What I mean is, are they used for the same purpose? Seems like it is.

 

Thanks again,

Sean

[KevinM1]

Instead of addslashes, I'd use mysql_real_escape_string.

[sean04]

Thanks! Could you explain the difference?

 

Thanks for the help!

[KevinM1]

Thanks! Could you explain the difference?

 

Thanks for the help!

 

Addslashes doesn't escape everything like mysql_real_escape_string does.  Any time you want to escape data, you should use the escape function related to the kind of database your using, or prepared statements, which automatically escape data inserted into a query (not available with MySQL, instead use the MySQLi extension.  See: mysqli