Can someone check my registration script?

arbitter · April 26, 2010

Goodday,

I wrote my registration script from scratch, rechecked a lot of things twice, it seems to work quite well. Problem is that I'm not sure whether it's fully secure, and whether it's a good method, or perhaps I forgot to check something, who knows..

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 <[email protected]>';
				$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>";
	}
}

I know it's quite a lot of code, but it's primarily the first things that are the most important.

I have a database real_u1 and tmp_u1. WHen a user registers, his documentation goes to tmp_u1 and then he gets a confirmation mail with $random in it, with a link. When he goes there (still need to make this part though), the data from tmp_u1 should be transferred to real_u1. I wouldn't need to check if the email or nick is already in use because I already checked it here.

It is in dutch, but if anyone requests a translation of some part, I'll gladly transfer it to english.

arbitter · April 26, 2010

And forgot to mention;

The only thing I didn't write myself is the is_email.inc.php

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;
    }
}

Got this from www.web-garden.be, all the records go to that site

CaptainChainsaw · May 14, 2010

Definitely a lot of code there, you've certainly made an attempt to sanitize the data going into the DB by using mysql_real_escape_string() etc.

I prefer to using something like this:

http://pear.php.net/manual/en/package.html.html-quickform.tutorial.php

I think put all my error messages in a conf file so that if you build a new site the registration form can work exactly the same way but the error messages can be changed from another file without changing the registration script at all.

I've attached a registration script that I've used that uses the above pear package. It simply validates the form and if it validates the user is then inserted into the database and the user default preferences are stored too. That completes the registration process. I also use smarty in this example to separate out presentation from logic.

Hope you or someone else finds this helpful.

CaptainChainsaw

[attachment deleted by admin]

ignace · May 14, 2010

if($_GET['fx'] == 'registreer')

Never taught about using the MVC architecture instead of those many IF's?

arbitter · May 16, 2010

if($_GET['fx'] == 'registreer')
Never taught about using the MVC architecture instead of those many IF's?

That seems difficult.. I'd never heard of it though

awjudd · May 17, 2010

You have no protection against SQL injection anywhere in that script ... very risky business not accounting for that.

~juddster

arbitter · May 17, 2010

You have no protection against SQL injection anywhere in that script ... very risky business not accounting for that.

~juddster

doesn't mysql_real_escape_string() cover that?

ignace · May 17, 2010

You have no protection against SQL injection anywhere in that script ... very risky business not accounting for that.

~juddster

doesn't mysql_real_escape_string() cover that?

Yes but in order for it to work you will need to actually use it. In your entire code you only use it once on $nick while you perform queries using $email which is left unvalidated. And the MVC architecture isn't that hard, a sample (basic) OOP implementation:

class FrontController {
  public static function run() {
    $controllerName = isset($_REQUEST['controller']) ? $_REQUEST['controller'] : 'main';
    $actionName = isset($_REQUEST['action']) ? $_REQUEST['action'] : 'index';
    
    //validation and stuff
    $controller = new $controllerClass();
    //*snip*
    call_user_method($controller, $methodName);
  }
}

class SomeController extends BaseController {
  public function indexAction() {
    //*snip*
    $this->view->some = $someService->findSomeBySome($query);
    $this->view->render('path/to/script.phtml');
  }
}

If you want to add-in template support just use the two-step view pattern:

class View extends ViewAbstract {}
class TwoStepView extends ViewAbstract {}

class FrontController {
  public static function run() {
    //*snip*
    $controller = new $controllerClass(new TwoStepView());
    //*snip
  }
}

Your controllers still think they are using a normal view while in fact they are using a TwoStepView pattern that wraps a normal View.

abstract class TwoStepViewAbstract extends ViewAbstract {
  private $view;
  // overwrite methods and pass it to $view
  public function __set($key, $value) {
    $this->view->__set($key, $value);
  }
}

class TwoStepView extends TwoStepViewAbstract {
  public function render($script) {
    ob_start();
    $this->view->render($script);
    $this->content = ob_get_contents();
    ob_end_clean();
    include('template.phtml');
    //<div id="content"><?php print $this->content; ?></div>
  }
}

arbitter · May 20, 2010

You have no protection against SQL injection anywhere in that script ... very risky business not accounting for that.

~juddster

doesn't mysql_real_escape_string() cover that?

Yes but in order for it to work you will need to actually use it. In your entire code you only use it once on $nick while you perform queries using $email which is left unvalidated. And the MVC architecture isn't that hard, a sample (basic) OOP implementation:
class FrontController {
  public static function run() {
    $controllerName = isset($_REQUEST['controller']) ? $_REQUEST['controller'] : 'main';
    $actionName = isset($_REQUEST['action']) ? $_REQUEST['action'] : 'index';
    
    //validation and stuff
    $controller = new $controllerClass();
    //*snip*
    call_user_method($controller, $methodName);
  }
}

class SomeController extends BaseController {
  public function indexAction() {
    //*snip*
    $this->view->some = $someService->findSomeBySome($query);
    $this->view->render('path/to/script.phtml');
  }
}
If you want to add-in template support just use the two-step view pattern:
class View extends ViewAbstract {}
class TwoStepView extends ViewAbstract {}

class FrontController {
  public static function run() {
    //*snip*
    $controller = new $controllerClass(new TwoStepView());
    //*snip
  }
}
Your controllers still think they are using a normal view while in fact they are using a TwoStepView pattern that wraps a normal View.
abstract class TwoStepViewAbstract extends ViewAbstract {
  private $view;
  // overwrite methods and pass it to $view
  public function __set($key, $value) {
    $this->view->__set($key, $value);
  }
}

class TwoStepView extends TwoStepViewAbstract {
  public function render($script) {
    ob_start();
    $this->view->render($script);
    $this->content = ob_get_contents();
    ob_end_clean();
    include('template.phtml');
    //<div id="content"><?php print $this->content; ?></div>
  }
}

But I used a preg_match() (though in this sript it's still ereg()), so I thought I didn't have to... But I guess it won't do any harm, I'll fix that.

As for the MVC architecture, it does seem very difficult to me

But I'll do my best to understand it and try it out, see if it comes in handy.

Cardale · May 24, 2010

Whats the speed comparison with something like this? http://pear.php.net/manual/en/package.html.html-quickform.tutorial.php

Seems kind of outrageous to me.

You could use a function like this at the top of a header page instead of an MVC design pattern which is in my opinion over kill for a web application although the MVC pattern does make all the new graduates giddy with excitement because the first language they learned was Java and it is so object oriented.

	function SECURITY($debug){
	if((count($_POST)!=0) || (count($_GET)!=0)){
		if(count($_POST)!=0){
			foreach($_POST as $key => $val){
				$val = addslashes($val);
				$val = htmlspecialchars($val);
				$val = mysql_real_escape_string($val); 
				$_POST[$key] = $val;
				if($debug==1){
					echo "POST ARRAY - Field : $key Value: $val<br />";
				}
			}
			return $_POST;
		}elseif(count($_GET)!=0){
			foreach($_GET as $key => $val){
				$val = addslashes($val);
				$val = htmlspecialchars($val);
				$val = mysql_real_escape_string($val);
				$_GET[$key] = $val;
				if($debug==1){
					echo "GET ARRAY - Field : $key Value: $val<br />";
				}
			}
			return $_GET;
		}else{
			die("ERROR with the http phaser");
		}
	}
}

You would also want to make sure no one could access any script individually.

arbitter · May 24, 2010

So those three steps make everything safe?

$val = addslashes($val);
$val = htmlspecialchars($val);
$val = mysql_real_escape_string($val);

Also, for the user password; I use md5() on it, so I don't need to do anything else with it do i? that's wwhy I didn't mysql_real_escape_string() it

Cardale · May 24, 2010

No you should also use salt on your passwords.

Sign In

Can someone check my registration script?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information