is this registration script secure?

[arbitter]

Hello,

I wrote my own registration script, but I'm not sure whether it's fully safe or not. Also I'm not quite sure if my 'idea' is the best option. The idea is that when a person registers, a mail with a random code get's sent to that person. He then has to click the link with the random code in it and then his data gets transferred from a 'temporary' map (with the random code, the username, the email, the gender and the password) to a 'real' map (with all the previous but exchange the random code with the ID.)

I don't know whether it's the best Idea to do this, but with seperate databases it's easier to connect to all the registered users.

Anyhow, here's the script:

if($_GET['fx'] == 'registreer')
{
	if(isset($_POST['registreer']))
	{
		$nick = $_POST['nick'];
		$email = $_POST['email'];
		$email = strip_tags($email);
		$pas1nomd5 = $_POST['pas'];
		$pas1 = $_POST['pas'];
		$pas1 = trim($pas1);
		$pas1 = md5($pas1);
		$pas2 = $_POST['pas2'];
		$pas2 = trim($pas2);
		$pas2 = md5($pas2);
		$geslacht = $_POST['sex'];
		$exploded = explode(" ",$nick);
		require_once('is_email.inc.php');
		if(isset($_SESSION['c1']) || isset($_COOKIE['c1']))
		{
			$melding = 'U bent al ingelogd.';
		}
		elseif(empty($nick) || empty($email) || empty($pas1) || empty($pas2) || empty($geslacht))
		{
			$melding = 'U dient alle velden in te vullen.';
		}
		elseif($pas1 != $pas2)
		{
			$melding = 'De paswoorden komen niet overeen.';
		}
		elseif(strlen($pas1nomd5) < 6)
		{
			$melding = 'Uw paswoord moet minimum 6 tekens lang zijn.';
		}
		elseif(strlen($pas1nomd5) > 20)
		{
			$melding = 'Uw wachtwoord mag maximaal 20 tekens lang zijn.';
		}
		elseif(!is_email($email))
		{
			$melding = 'Gelieve uw email adres te controleren en opnieuw in te voeren.';
		}
		elseif(isset($exploded[1]))
		{
			$melding = 'Er mogen geen spaties voorkomen in je nick.';
		}
		else
		{
			$random = md5(microtime());
			$nick = strip_tags($nick);
			$nick = trim($nick);
			require_once('mysql_connect.inc.php');
			mysql_connect(MYSQL_SERVER, MYSQL_GEBRUIKERSNAAM, MYSQL_WACHTWOORD) or die("Verbinding mislukt: " . mysql_error());
			mysql_select_db('real_u1')or die(mysql_error());
			$ontvangr3 = mysql_query("SELECT * FROM real_u1 WHERE r3='$email'") or die(mysql_error());
			$rowr3 = mysql_fetch_array($ontvangr3);
			$ontvangr2 = mysql_query("SELECT * FROM real_u1 WHERE r2='$nick'") or die(mysql_error());
			$rowr2 = mysql_fetch_array($ontvangr2);
			mysql_select_db('tmp_u1');
			$ontvangt3 = mysql_query("SELECT * FROM tmp_u1 WHERE t3='$email'") or die(mysql_error());
			$rowt3 = mysql_fetch_array($ontvangt3);
			$ontvangt2 = mysql_query("SELECT * FROM tmp_u1 WHERE t2='$nick'") or die(mysql_error());
			$rowt2 = mysql_fetch_array($ontvangt2);
			if(!empty($rowr3))
			{
				$melding = 'Dit emailadres is reeds geregistreerd.';
			}
			elseif(!empty($rowr2))
			{
				$melding = 'Deze nick bestaat reeds.';
			}
			elseif(!empty($rowt3))
			{
				$melding = 'Dit emailadres staat reeds in de lijst om toegevoegd te worden.';
			}
			elseif(!empty($rowt2))
			{
				$melding = 'Deze nick staat reeds in de lijst om toegevoegd te worden.';
			}
			else
			{
				$to = $email;
				$subject = 'Uw account activeren';
				$header = 'From: FransDepypere <info@fransdepypere.be>';
				$message = "http://fun.fransdepypere.be/registreer?fx=bevestig&code=$random";
				$mail = mail($to,$subject,$message,$header);
				$nick = mysql_real_escape_string($nick);
				mysql_select_db('tmp_u1')or die (mysql_error());
				$ontvang2 = mysql_query("INSERT INTO tmp_u1 (t1,t2,t3,t4,t5) VALUES ('$random','$nick','$email','$pas1','$geslacht')")or die(mysql_error());
				$melding = 'U bent sucesvol aangemeld. Om uw aanmelding te voltooien, moet u uw email nakijken om uw account te verifiëren.';
			}
		}
		echo $melding;
	}
	else
	{
		echo "<div class='registreer'>
			<font color='#3870c4'>Vul in:</font><br />
			<form method='post' action='index.php&#63;fx&#61;registreer'><br />
			<center>
			<table>
				<tr>
					<td class='e'>
						Nick: 
					</td>
					<td>
						<input type='text' size='10' maxlength='20' name='nick'>
					</td>
				</tr>
				<tr>
					<td class='e'>
						E-mailadres:
					</td>
					<td>
						<input type='text' size='10' name='email'>
					</td>
				</tr>
				<tr>
					<td class='e'>
						Geslacht:
					</td>
					<td>
						m:<input type='radio' name='sex' value='man' />  v:<input type='radio' name='sex' value='vrouw' />
					</td>
				</tr>
				<tr>
					<td class='e'>
						Paswoord:
					</td>
					<td>
						<input type='password' size='10' maxlength='20' name='pas'>
					</td>
				</tr>
				<tr>
					<td class='e'>
						Bevestig Paswoord:
					</td>
					<td>
						<input type='password' size='10' maxlength='20' name='pas2'>
					</td>
				</tr>

			</table>
			</center>
			<input class='knop' name='registreer' type='submit' value='Registreer'>
			</form>
			</div>";
	}
}

 

It's in dutch, but I can translate it if you like.

 

The 'is_email.inc.php' file is following:

function is_email($emailadres)
{
    if (strlen($emailadres) < 7) {
        return FALSE;
    }
    if (ereg("^[_a-zA-Z0-9-]+(\.[_a-zA-Z0-9-]+)*@([a-zA-Z0-9-]+\.)+([a-zA-Z]{2,4})$", $emailadres)) {
        return TRUE;
    } else {
        return FALSE;
    }
}

 

The 'temporary' database is called 'temp_u1' and the registered users daztabase is called 'real_u1'.

t1 = random code

r1 = id

t2=r2 = nick

t3=r3 = email

t4=r4 = password

t5=r5 = gender

 

I have put this question here too:http://www.phpfreaks.com/forums/index.php/topic,295950.0.html , but coming to think about it it might be in the wrong section (also considering I received no replies).

[ChemicalBliss]

Your email part seems secure but your nick isnt.

 

always use mysql_real_escape_string($value) for each value used in a mysql query.

 

Also, google: PHP Security Tutorial

 

-cb-

[arbitter]

Thank you for your reply.

Right before inserting it all into the database I do have a mysql_real_escape_string() though; is that the same as mysql_real_escape()?

$nick = mysql_real_escape_string($nick);
mysql_select_db('tmp_u1')or die (mysql_error());
$ontvang2 = mysql_query("INSERT INTO tmp_u1 (t1,t2,t3,t4,t5) VALUES ('$random','$nick','$email','$pas1','$geslacht')")or die(mysql_error());

 

[Ken2k7]

There's no such pre-defined function as mysql_real_escape() so how can you compare them?

[arbitter]

There's no such pre-defined function as mysql_real_escape() so how can you compare them?

My bad

So I guess it's allright like this then?

[Ken2k7]

I would make some suggestions because the overall code is a bit over-complicated and a mess.

 

			$pas1 = $_POST['pas'];
		$pas1 = trim($pas1);
		$pas1 = md5($pas1);
		$pas2 = $_POST['pas2'];
		$pas2 = trim($pas2);
		$pas2 = md5($pas2);

 

I can't start or end my password with space? How come?

 

require_once('is_email.inc.php');

If I were to include something, I would include it at the top of the file so later on, you're not going line by line to find them. You have another require elsewhere. Same logic.

 

if(isset($_SESSION['c1']) || isset($_COOKIE['c1']))
		{
			$melding = 'U bent al ingelogd.';
		}

What does that do? I can't read your language.

 

				$ontvangr3 = mysql_query("SELECT * FROM real_u1 WHERE r3='$email'") or die(mysql_error());
			$rowr3 = mysql_fetch_array($ontvangr3);
			$ontvangr2 = mysql_query("SELECT * FROM real_u1 WHERE r2='$nick'") or die(mysql_error());
			$rowr2 = mysql_fetch_array($ontvangr2);
			mysql_select_db('tmp_u1');
			$ontvangt3 = mysql_query("SELECT * FROM tmp_u1 WHERE t3='$email'") or die(mysql_error());
			$rowt3 = mysql_fetch_array($ontvangt3);
			$ontvangt2 = mysql_query("SELECT * FROM tmp_u1 WHERE t2='$nick'") or die(mysql_error());
			$rowt2 = mysql_fetch_array($ontvangt2);

Rather than running 4 queries, I would suggest you run 2, but 1 if possible. Also, or die() is bad practice.

 

					$ontvang2 = mysql_query("INSERT INTO tmp_u1 (t1,t2,t3,t4,t5) VALUES ('$random','$nick','$email','$pas1','$geslacht')")or die(mysql_error());

You'll want to run mysql_real_escape_string on $geslacht.

[arbitter]

Good point there. I figured it was possible that people would 'accidentaly' enter a space or something.. But now that you mention it, it could be part of the code... I'll change that.

 

I'll put the 2 require's in the top of the page.

 

if(isset($_SESSION['c1']) || isset($_COOKIE['c1']))
		{
			$melding = 'U bent al ingelogd.';
		}

 

means

if(isset($_SESSION['c1']) || isset($_COOKIE['c1']))
		{
			$melding = 'You are already logged in.';
		}

(The person has a cookie (if the cookies are enabled) with it's emailadress , or if they aren't enabled a session containing their email. Ofcourse if they are enabled, both are set)

 

about the queries, I'm not good with Mysql at all, and since I wanted a different warning for each problem, I figured it'd be better that I'd do 4 queries..

 

And $geslacht ($gender) is gotten with $_POST[] from a form with 'radio'-type. So the value is OR man OR woman. So I thought I wouldn't need to mysql_real_escape() that.. But perhaps it's safer to do that too indeed.

 

Also, what should I use instead 'or die()'?

 

Thanks!

[Daniel0]

Just quickly looking at your code, these things come to mind (though they're not related to security):

 

1) Stick to one language for identifiers. I would even say, always choose English.

2) Why are you redeclaring so many things? I'm thinking of all the POST variables.

3) Get some real error handling instead of ending script execution on database errors.

4) The ereg function is deprecated. It will likely be removed in future versions.

5) You should utilize table locking when you're first checking if something is available and then proceeding to insert if it is. This way you ensure that you won't run into concurrency problems.

[arbitter]

Just quickly looking at your code, these things come to mind (though they're not related to security):

 

1) Stick to one language for identifiers. I would even say, always choose English.

2) Why are you redeclaring so many things? I'm thinking of all the POST variables.

3) Get some real error handling instead of ending script execution on database errors.

4) The ereg function is deprecated. It will likely be removed in future versions.

5) You should utilize table locking when you're first checking if something is available and then proceeding to insert if it is. This way you ensure that you won't run into concurrency problems.

What do you mean with table locking?

[Daniel0]

http://dev.mysql.com/doc/refman/5.1/en/lock-tables.html

[arbitter]

And what should I use instead of ereg()? preg_match()?

[newbtophp]

And what should I use instead of ereg()? preg_match()?

 

Yes but make sure to add delimiters around the expression. (look it up via google for more info)

[Daniel0]

And what should I use instead of ereg()? preg_match()?

 

Yes but make sure to add delimiters around the expression. (look it up via google for more info)

 

Also see: http://dk.php.net/manual/en/reference.pcre.pattern.posix.php

[arbitter]

And what should I use instead of ereg()? preg_match()?

 

Yes but make sure to add delimiters around the expression. (look it up via google for more info)

 

Also see: http://dk.php.net/manual/en/reference.pcre.pattern.posix.php

I find this all so difficult! But I'll do my best, thanks for all the help!

[Ken2k7]

How so? Just stick with preg_match and preg_replace.

[arbitter]

I've never worked with preg_replace() or preg_match(), neither did I use preg() (I got that part of the code from a site)

 

And the only decent explanation I can find is: http://www.webcheatsheet.com/php/regular_expressions.php but still I can't figure it out :')