create account script (could use improvements)

[spiritssight]

Hello All,

I have wrote the following script and I am looking for feed back and advice on improving it. I don't have form validation in it yet, I am slowly trying to work on a validation function (be posting that soon).

Any how the create account script does:
displays form, person enters data, it then displays there input but not there password, at that time it also encrpyted the password so if someone reads the source of the file they would not see it in plain text, once the person hits submit, it checks to see if the email address that has been entered is in the user table, if it is it tells them (future it will redirect) and if now it creates the account

<?php

if(isset($_POST['next']))
{
$salt = "123abc123abccba321cba321";
$_POST['user_password'] = md5($salt . $_POST['user_password'] . $salt);
$_POST['verify_user_password'] = md5($salt . $_POST['verify_user_password'] . $salt);

echo '
<form action ="create_account.php" method="post">
<input type="hidden" value="'. $_POST['f_name'] .'" id="f_name" name="f_name" />
<input type="hidden" value="'. $_POST['m_inital'] .'" id="m_inital" name="m_inital" />
<input type="hidden" value="'. $_POST['l_name'] .'" id="l_name" name="l_name" />
<input type="hidden" value="'. $_POST['email_address'] .'" id="email_address" name="email_address" />
<input type="hidden" value="'. $_POST['user_password'] .'" id="user_password" name="user_password" />
<input type="hidden" value="'. $_POST['verify_user_password'] .'" id="verify_user_password" name="verify_user_password" />

<p>PLease look at the information and make sure its correct!</p>
'. $_POST['f_name'] .' '. $_POST['m_inital'] .' '. $_POST['l_name'] .'<br />'. $_POST['email_address'].'<br /><br />
<input type="submit" name="create" value="Create Account" />
</form>
';
}
else if(isset($_POST['create']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

$query_s = "SELECT 'email_address' FROM user_info WHERE email_address = '{$_POST['email_address']}'";
$result_s = mysql_query($query_s) OR die("Sorry, unable to select record: " . mysql_error());
$count_s = mysql_num_rows($result_s);
	if($count_s)
	{echo 'Your email address is already in our system!';}
	else
	{
		do
		{
		$user_id = sprintf("%'09d", mt_rand(1, 999999999));
		$user_id = "P-".$user_id;
		$query_s = "SELECT user_id FROM access_credentials WHERE user_id = '$user_id'";
		$result_s = mysql_query($query_s) OR die("Sorry, unable to select record: " . mysql_error());
		echo $user_id .'<br />';
		}
		while(mysql_num_rows($result_s) != 0); // don't do again if = 0

$salt = "123abc123abccba321cba321";
$_POST['user_password'] = md5($salt . $_POST['user_password'] . $salt);

$query_i = "INSERT INTO user_info (user_id, f_name, m_inital, l_name, email_address) VALUES ('$user_id', '$_POST[f_name]','$_POST[m_inital]','$_POST[l_name]','$_POST[email_address]')";
 $result_i = mysql_query($query_i) OR die("Sorry was unable to create account (not able to insert into the database table! <br />" . mysql_error());
 $count = mysql_affected_rows();

 $query_i = "INSERT INTO access_credentials (user_id, user_password, user_access_level) VALUES ('$user_id', '$_POST[user_password]','0')";
 $result_i = mysql_query($query_i) OR die("Sorry was unable to insert ".$fin." into the database table allowed! <br />" . mysql_error());
 $count = mysql_affected_rows();
	if($count)
	{
	echo "Account Created";
	}
}
}
else
{
?>

<form action="create_account.php" method="post">

<label for="f_name">First Name: </label>
<input type="text" value="" id="f_name" name="f_name" /><br />

<label for="m_inital">Middle Inital: </label>
<input type="text" value="" id="m_inital" name="m_inital" /><br />

<label for="l_name">Last Name: </label>
<input type="text" value="" id="l_name" name="l_name" /><br />

<label for="email_address">E-Mail Address: </label>
<input type="text" value="" id="email_address" name="email_address" /><br />

<label for="user_password">Password: </label>
<input type="password" value="" id="user_password" name="user_password" /><br />

<label for="verify_user_password">Verify Password: </label>
<input type="password" value="" id="verify_user_password" name="verify_user_password" /><br />

<input type="submit" name="next" value="submit" /><input type="reset" name="clear" value="Clear" />
</form>
<?php
}
?>

I would really like to have this as three different files, one for form, preview, validating, and processing

the way I am hoping my validation works will be if the person entered some of the information right and others wrong the only stuff that would be displayed would be the wrong information, any ideas would be great on that?

thanks ahead of time for your advice and time for the help!

Sincerely,
Christopher

[bradgrafelman]

1. You might want to use [man]htmlentities/man on the data you output to fill the form fields, just to ensure that it doesn't break your HTML.

2. Your SELECT query selects the string 'email_address' - not the column; remove the quotes in the column list.

3. This script is also vulnerable to SQL injection attacks.

4. Is there an index on the email_address column? If you're simply trying to prevent duplicates, add another UNIQUE index on this column - that way you don't even need to run a SELECT statement to check for duplicates. Instead, just run the INSERT query and check if it fails. I don't remember the error code offhand for duplicate data for a key, but you can check for this and then display the error message.

5. As I've commented before, there's no need to count the rows SELECT'ed to see if data was returned - just try to fetch the first row.

6. Why are you trying to randomly generate a user_id and loop until you succeed? Just set user_id to be an AUTO_INCREMENT column.

7. Again, don't output MySQL error messages on failure.

8. Is there really a need to split the data into two separate tables?

[spiritssight]

Ok, I read in the man about the htmlentilies() I don't understand what this does for my data that fills the form fields, please explain so I can understand this better.

ok the '' around the email_address is fixed, I copy this from another thing I had done for a update set thing I think, but its fixed

I will take care of the vulnerable stuff also sometime soon (morning :-) )

Yes I have a index on the email_address and I have also now taken out the select query (leaves me a little confused of how I can check if account exist before creating a user id) I will read the mysql man and find out wha the error code is

I no longer have that select statement so thats fixed :-) or less I missed one I have a mysql_num_rows for the while loop, should I change that to the $results_s var?

I want the user_id to have P- (person) in them and E- (entitie) W- (web) any ideas of how to do this a better way would be great :-) .

I will delete all mysql_error() things once I know the script is safe and also ready to be put in live mode

are you talking about the activation and access_... tables? if so I did not think about that and the three fields thats in the activation table get deleted after the activation process as this is at that time no longer useful activation code, activation expire date and then the user_id that says what account the activation code for

wow lots of things, I am learning through!

below is the new script:

Sincerely,
Christopher

Could I do what I did with the escaping in the area with the hidden form or should I do it in the querys?

<?php

if(isset($_POST['next']))
{
$salt = "123abc123abccba321cba321";
$_POST['user_password'] = md5($salt . $_POST['user_password'] . $salt);
$_POST['verify_user_password'] = md5($salt . $_POST['verify_user_password'] . $salt);

echo '
<form action ="create_account.php" method="post">
<input type="hidden" value="\". mysql_real_escape_string($_POST['f_name']) .\"" id="f_name" name="f_name" />
<input type="hidden" value="\". mysql_real_escape_string($_POST['m_inital']) .\"" id="m_inital" name="m_inital" />
<input type="hidden" value="\". mysql_real_escape_string($_POST['l_name']) .\"" id="l_name" name="l_name" />
<input type="hidden" value="\". mysql_real_escape_string($_POST['email_address']) .\"" id="email_address" name="email_address" />
<input type="hidden" value="\". mysql_real_escape_string($_POST['user_password']) .\"" id="user_password" name="user_password" />
<input type="hidden" value="\". mysql_real_escape_string($_POST['verify_user_password']) .\"" id="verify_user_password" name="verify_user_password" />

<p>PLease look at the information and make sure its correct!</p>
'. $_POST['f_name'] .' '. $_POST['m_inital'] .' '. $_POST['l_name'] .'<br />'. $_POST['email_address'].'<br /><br />
<input type="submit" name="create" value="Create Account" />
</form>
';
}
else if(isset($_POST['create']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

do		{
$user_id = sprintf("%'09d", mt_rand(1, 999999999));
$user_id = "P-".$user_id;
$query_s = "SELECT user_id FROM access_credentials WHERE user_id = '$user_id'";
$result_s = mysql_query($query_s) OR die("Sorry, unable to select record: " . mysql_error());
echo $user_id .'<br />';
}
while(mysql_num_rows($result_s) != 0); // don't do again if = 0

$salt = "123abc123abccba321cba321";
$_POST['user_password'] = md5($salt . $_POST['user_password'] . $salt);

$query_i = "INSERT INTO user_info (user_id, f_name, m_inital, l_name, email_address) VALUES ('$user_id', '$_POST[f_name]','$_POST[m_inital]','$_POST[l_name]','$_POST[email_address]')";
 $result_i = mysql_query($query_i) OR die("Sorry was unable to create account (not able to insert into the database table! <br />" . mysql_error());
 $count = mysql_affected_rows();
	if($count)
	{
	 $query_i = "INSERT INTO access_credentials (user_id, user_password, user_access_level) VALUES ('$user_id', '$_POST[user_password]','0')";
	 $result_i = mysql_query($query_i) OR die("Sorry was unable to insert ".$fin." into the database table allowed! <br />" . mysql_error());
	 $count = mysql_affected_rows();
		if($count)
		{
		include "/home/dev/www/lib/random.php";
		$activation_code = strtoupper(random(10));
		$expire_date = strtotime("+3 days");
		$query_i = "INSERT INTO activation (user_id, activation_code, expire_date) VALUES ('$user_id', '$activation_code','$expire_date')";
		$result_i = mysql_query($query_i) OR die("Sorry was unable to insert ".$fin." into the database table allowed! <br />" . mysql_error());
		$count = mysql_affected_rows();
			if($count)
			{
			echo "Account Created";
			$expire_date = date( "Y-m-d", $expire_date);
			include '/home/dev/www/user-sys/create_account_activation_email.php';
			header("location: activate.php");
			}
			else
			{
			echo "We had a problem adding your account ERROR: 1012";
			}
		}
		else
		{
		echo "We had a problem adding your account ERROR: 1011";
		}
	}
	else
	{
	echo "We had a problem adding your account ERROR: 1010";
	}
}
else
{
include '/home/dev/www/user-sys/create_account_form.php';
}
?>

[laserlight]

Ok, I read in the man about the htmlentilies() I don't understand what this does for my data that fills the form fields, please explain so I can understand this better.

Basically, it escapes special characters for HTML.

[spiritssight]

ok, I have gone through the code and I believe I have made all the changes as recommended.

Here is the files for the create account part of the user-sys:

create_account_verify.php:

<?php
// covert _POST var to shorter var for the form
$f_name = $_POST['f_name'];
$m_inital = $_POST['m_inital'];
$l_name = $_POST['l_name'];
$email_address = $_POST['email_address'];
$salt = "123abc123abccba321cba321";
$user_password = md5($salt . $_POST['user_password'] . $salt);
$verify_user_password = md5($salt . $_POST['verify_user_password'] . $salt);

echo '
<form action ="create_account.php" method="post">
<input type="hidden" value="'. $f_name .'" id="f_name" name="f_name" />
<input type="hidden" value="'. $m_inital .'" id="m_inital" name="m_inital" />
<input type="hidden" value="'. $l_name .'" id="l_name" name="l_name" />
<input type="hidden" value="'. $email_address .'" id="email_address" name="email_address" />
<input type="hidden" value="'. $user_password .'" id="user_password" name="user_password" />
<input type="hidden" value="'. $verify_user_password .'" id="verify_user_password" name="verify_user_password" />

<p>PLease look at the information and make sure its correct!</p>
'. $f_name .' '. $m_inital .' '. $l_name .'<br />'. $email_address .'<br /><br />
<input type="submit" name="create" value="Create Account" />
</form>
';
?>

create_account_process.php:

<?php

if(isset($_POST['next']))
{
include '/home/dev/www/user-sys/create_account_verify.php';
}
else if(isset($_POST['create']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

// covert _POST var to shorter var for the form
$f_name = $_POST['f_name'];
$m_inital = $_POST['m_inital'];
$l_name = $_POST['l_name'];
$email_address = $_POST['email_address'];
$salt = "123abc123abccba321cba321";
$user_password = md5($salt . $_POST['user_password'] . $salt);

do
{
$user_id = sprintf("%'09d", mt_rand(1, 999999999));
$user_id = "P-".$user_id;
$sql_s = "SELECT user_id FROM access_credentials WHERE user_id = '$user_id'";
$result_s = mysql_query($sql_s) OR die("Sorry, unable to select record: " . mysql_error());
}
while(mysql_num_rows($result_s) != 0); // don't do again if = 0

$sql_i = sprintf(
"INSERT INTO user_info (user_id, f_name, m_inital, l_name, email_address) VALUES ('%s', '%s', '%s', '%s', '%s')",
	$user_id,
	mysql_real_escape_string($f_name),
	mysql_real_escape_string($m_inital),
	mysql_real_escape_string($l_name),
	mysql_real_escape_string($email_address)
);
$result_i = mysql_query($sql_i) OR die("Sorry was unable to create account (not able to insert into the database table! <br />" . mysql_error());
$count = mysql_affected_rows();
	if($count)
	{
	include "/home/dev/www/lib/random.php";
	$user_activation_code = strtoupper(random(10));
	$activation_expire_date = strtotime("+3 days");

	$sql_i = sprintf( 
	"INSERT INTO access_credentials (user_id, user_password, user_access_level, user_activation_code, user_activation_expire_date) VALUES ('%s', '%s', '%s','%s', '%s')",
		$user_id,
		mysql_real_escape_string($user_password),
		0,
		$user_activation_code,
		$activation_expire_date
	);
	$result_i = mysql_query($sql_i) OR die("Sorry was unable to insert into the database table! <br />" . mysql_error());
	$count = mysql_affected_rows();
		if($count)
		{
		include '/home/dev/www/user-sys/create_account_activation_email.php';
		header("location: activate.php");
		}
		else
		{
		echo "We had a problem adding your account ERROR: 1012";
		}
	}
	else
	{
	echo "We had a problem adding your account ERROR: 1010";
	}
}
else
{
include '/home/dev/www/user-sys/create_account_form.php';
}
?>

I did not put the create_account_form.php in here as its just html, if you would like to see it then I will include it.

I am looking for feedback on improving this script even more, I am also looking for advice on validating user input, and ways to make this script simple and better.

Thanks for your assistance with this project!

Sincerely,
Christopher