Need assistance checking code and possibly converting it to prepared statemetnts

PHP
,
1

Greetings! This is some old code I’m now working with. Nothing is being inserted into the DB, only read from to make sure someone doesn’t use an existing username/e-mail when signing up. Is it vulnerable? Any thoughts or suggestions would be most appreciated! Thanks!

if (isset($_POST['register'])) {
	$email = $_POST['email'];
	$username = $_POST['username'];
	$password = $_POST['password'];
	$password2 = $_POST['password2'];
	$who = $_POST['who'];
	$ip = $_POST['ip'];

	if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
	    $ip = $_SERVER['HTTP_CLIENT_IP'];
	} elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
	    $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
	} else {
	    $ip = $_SERVER['REMOTE_ADDR'];
	}

      $sql_w = "SELECT * FROM xf_user WHERE username='$who'";
      $res_w = mysqli_query($db, $sql_w); 	
	$sql_u = "SELECT * FROM xf_user WHERE username='$username'";
  	$sql_e = "SELECT * FROM xf_user WHERE email='$email'";
  	$res_u = mysqli_query($db, $sql_u);
  	$res_e = mysqli_query($db, $sql_e);
	
	if (empty($username)) {
    	  $name_error2 = "<br><span class='error_msg'>Username cannot be blank!</span>";
	}else if (empty($email)) {
    	  $email_error3 = "<br><span class='error_msg'>E-mail cannot be blank!</span>";
	}else if(mysqli_num_rows($res_u) > 0) {
  	  $name_error = "<br><span class='error_msg'>Opps! This username belongs to an existing member!</span>"; 	
  	}else if(mysqli_num_rows($res_e) > 0){
  	  $email_error = "<br><span class='error_msg'>Opps! This e-mail belongs to an existing member!</span>";
	}else if(!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL, FILTER_NULL_ON_FAILURE)) {
 	  $email_error2 = "<br><span class='error_msg'>Invalid e-mail!</span>";  
      }else if( mysqli_num_rows($res_w ) == 0){
 	  $who_error = "<br><span class='error_msg'>No matching member found!!</span>";
	}else if(strlen($password) < 8) {
	  $pass_error1 = "<br><span class='error_msg'>Password needs to be at least 8 characters!</span>";
	}else if($password != $password2) {
        $pass_error2 = "<br><span class='error_msg'>Passwords do not match!</span>";
  	}else{

	$date = date('m/d/Y');
      
	$headers = "From: zoldos <zoldos@xxxxx.net>";

	$subject = "access request";

	$message = "Your Submitted E-mail: $email
Choosen Username: $username
Password: $password
IP: $ip	
Referred By: $who
Date requested: $date\n
Thanks for your interest in my forum!  Thanks!";

	mail($email, $subject, $message, $headers);
	header('location: done.html');
	}  	
  }
2

execute exactly 1 query.
store your errors in an array; that way you can simplify your check for ‘was there an error’ down to a size check on the errors array.
code is 100% vulnerable to SQL injection. Replace query with prepared statement.

2 Likes
3

You’re not really going to send the users requested password in plain text in an email for a live forum, are you?

Does your use of else if mean that it doesn’t produce as many errors as you expect? I don’t like that construction myself so I try to avoid it, and I’m therefore not that familiar. In my head, I imagine that it only does the second error check if the first one did not return true, and so on down the list.

But, as in your title - convert it to prepared statements.

1 Like
4

Okay thanks! But would I still be able to display the specific errors, or is that redundant?

5

Yes, it basically checks for each specific error, so if one is false, it goes to the next, then if all are okay, it processes the rest of the script. It works, and I like the different error messages. :smiley:

Is there a better way to send the password?

6

Example:

$errors = array();
$errors['email'] = "Your email is bad.";
$errors['name'] = "Your name is bad";
//Later...
if(count($errors) > 0) {
  //There were 1 or more errrors. Do not adjust Database.
}
//Displaying errors:
foreach($errors as $error) { 
  echo "<li>".$error."</li>";
}
// Or something like...
<input type='text' name='email'>
<span class='error'><?php echo isset($errors['email']) ? $errors['email'] : ""; ?></span>
1 Like
7

Probably not, if you have to send the password, and you have to be able to read it when it arrives. It’s not very secure, though, and I wouldn’t imagine your users will be happy about it.

1 Like
8

No one has complained so far. :slight_smile: The purpose of the code portion in my OP is to collect user info which is then sent to my e-mail, and then I create the user’s account (my site is private).

My main goal is to make the query secure with prepared statements…

9

Cool, thanks! :smiley:

10

While you should, it is rather ironic that you care about that when you are using plain text passwords.

They will when their person information has been leaked from the poor practices you are implementing at which point you could be held legally liable. You cant expect users to know if what you are doing is right or not. It is your responsibility to know what is right and implement it.

1 Like
11

Thanks everyone. vexdarkness has abandoned the project and asked that this topic be closed.