Page 1 of 2

Can you check my code - need your expert help

Posted: Mon Dec 10, 2007 4:18 pm
by newbie2php
Many thanks for responses to my other thread.

I have now created a registeration page, with a form. It posts the info to a new page, and I would like to check I have validated the

Code: Select all

$_POST['$varibles']
correctly.

I have placed all validation functions in thier own include file

Below is the functions:

Code: Select all

// check username format
// $string can be max. 10 chars, and must only contain letters and numbers, no spaces or other chars AT ALL
function check_username_format($string)
{

$pattern = "^[A-Za-z0-9]$";

if preg_match($pattern, $string)
	{
		if (strlen($string) <= 10 )
		{
		return true;
		}
		else
		{
		return false;
		}
	} 
	else
	{
	return false;
	}
}

// check string length to parameters min length and max length
// check $string is within $min and $max boundaries
function check_within_length($string, $min, $max) {
  $length = strlen ($string);
  if (($length < $min) || ($length > $max)) 
	{
    return false;
  } 
	else 
	{
    return true;
  }
}

// check password format
// $string can be between 6 and 12 chars long, and must only contain letters and numbers, no spaces or other chars AT ALL
function check_password_format($string)
{
$pattern = "^[A-Za-z0-9]$";

	if preg_match($pattern, $string)
	{
			if (check_within_length($string, '6', '12'))
			{
			return true;
			}
			else
			{
			return false;
			}
	} 
	else
	{
	return false;
	}

}


// check email format, taken off web
function check_email_format($email) { 
  // First, we check that there's one @ symbol, and that the lengths are right 
  if (!ereg("^[^@]{1,64}@[^@]{1,255}$", $email)) { 
    // Email invalid because wrong number of characters in one section, or wrong number of @ symbols. 
    return false; 
  } 
  // Split it into sections to make life easier 
  $email_array = explode("@", $email); 
  $local_array = explode(".", $email_array[0]); 
  for ($i = 0; $i < sizeof($local_array); $i++) { 
     if (!ereg("^(([A-Za-z0-9!#$%&'*+/=?^_`{|}~-][A-Za-z0-9!#$%&'*+/=?^_`{|}~\.-]{0,63})|(\"[^(\\|\")]{0,62}\"))$", $local_array[$i])) { 
      return false; 
    } 
  }   
  if (!ereg("^\[?[0-9\.]+\]?$", $email_array[1])) { // Check if domain is IP. If not, it should be valid domain name 
    $domain_array = explode(".", $email_array[1]); 
    if (sizeof($domain_array) < 2) { 
        return false; // Not enough parts to domain 
    } 
    for ($i = 0; $i < sizeof($domain_array); $i++) { 
      if (!ereg("^(([A-Za-z0-9][A-Za-z0-9-]{0,61}[A-Za-z0-9])|([A-Za-z0-9]+))$", $domain_array[$i])) { 
        return false; 
      } 
    } 
  } 
  return true; 
}

// check name format
// name can be max 22 chars, letters only but allow '-'
function check_name_format($string)
{

	$pattern = "^[A-Za-z]$ | ^[A-Za-z]-[A-Za-z]$";

	if preg_match($pattern, $string)
	{
			if (check_within_length($string, '1', '22'))
			{
			return true;
			}
			else
			{
			return false;
			}
	} 
	else
	{
	return false;
	}

}


// check gender
// very basic validation, check string is either 'male' or 'female'
function check_is_gender($string)
{
	if (($string != 'male') || ($string != 'female'))
	{
	return false;
	}
	else
	{
	return true;
	}
}

// check $input is only digits, return false if not
function check_is_numeric($input) {
  if(preg_match("^[0-9]$", $input))
	{
		return true;
	}
	else
	{
		return false;
	}
}


// check $input is only letter, return false if not
function check_is_alpha($input) {
	if(preg_match ("^[A-Za-z]$", $input))
	{
		return true;
	}
	else
	{
		return false;
	}
}

// check input is a day (number i.e. '1-31') of the month
// check is numeric, and then within correct range 
function check_daynumber_dob($string)
{
	if check_is_numeric($string)
	{
		if (($string > 31) || ($string < 1))
		{
		return false;
		}
		else
		{
		return true;
		}
	}
	else
	{
	return false;
	}
}

//check month format - 3 letter abv's e.g jan, feb, mar etc...
// first check it only contains letters, then check the string length is only 3
function check_month_dob($string)
{
	if check_is_alpha($string)
	{
			if (check_within_length($string, '3', '3'))
			{
			return true;
			}
			else
			{
			return false;
			}
	}
	else
	{
	return false;
	}
}


// check year is correct format - 4 numbers such as 1956 or 2008
// first check it only contains numbers, and then make sure string length is only 4
function check_year_dob($string)
{
	if check_is_numeric($string)
	{
			if (check_within_length($string, '4', '4'))
			{
			return true;
			}
			else
			{
			return false;
			}
	}
	else
	{
	return false;
	}
}
Some are very anal - but this is good I guess

I just need to know if the functions look correct? Are the expressions correctly written?

Here is the page which the varibles get posted to....

Code: Select all

include_once('INCLUDE FILE WITH THE ABOVE FUNCTIONS');
session_start();

$username = check_username_format($_POST['username']);
$passwrd = check_password_format($_POST['psswrd']);
$conpasswrd = check_password_format($_POST['confirmpsswrd']);
$email = check_email_format($_POST['email']);
$fname = check_name_format($_POST['fname']);
$sname = check_name_format($_POST['sname']);
$gender = check_is_gender($_POST['gender']);
$day_dob = check_daynumber_dob($_POST['day']);
$month_dob = check_month_dob($_POST['month']);
$year_dob = check_year_dob($_POST['year']);
If someoen could have a decent look over my code I would be so thankful!

Thanks

Posted: Mon Dec 10, 2007 5:35 pm
by alex.barylski
Thats a lot of code to ask someone to look over in a forum thread. :P

I didn't read every line...and regex is difficult to comprehend without sitting down and trying to make sense of it...

But some general guidelines:

1) Follow the principle of least privilege. Don't allow anything more than you have to. If you have a regex which is cleaning a $name field.

Code: Select all

$name = preg_replace('[^0-9A-Za-z\' ]', '', $name);
I don't know how accurate that is syntactically...but notice how it uses preg_replace and just strips any characters which are not expected in typical names.

2) Depending on your design goals, you could carry out formatting on the client side. That way you only need filtering on the server side, not checking.

3) You want to make sure you call mysql_real_escape_string and htmlspecialchars/strip_tags/preg_replace on all of your data.

Posted: Mon Dec 10, 2007 5:50 pm
by newbie2php
Hi Hockey, many thanks for the reply.

Sorry if its alot of code, some of the functions are pretty similar though. I was just unsure about the patterns I created, I am new to that and wasn't sure if I had done them correctly.

So, you suggest using preg_replace instead of seeing if the string fits a pattern in the first place? Basically making the string follow a format instead of checking if it does? Sorry if this is not what it does, but if it does the reason I wanted to just check is because what if a user entered an invalid username, say with a '!' sign present, then the code just got rid of this and said the registration was successful? I would need to make sure the user was aware that the username had been changed because it didn't follow the rules. Would it not be easier to just display a message to reenter the user name as it was not correct? Sorry if I have got the wrong end of the stick, I am still trying to learn!

I want to be 100% with my security checking.

Reason for the functions instead of just applying some basic checks within the page itself was because I plan on using these validation functions, and some more, on other pages, such as log in, changing details etc... so I thought having an include file would be good to just reuse code and make functions speficic and airtight for their use

Yeah - I would use mysql_real_escape_string before entering into database :)

Thanks for the reply :D

Posted: Mon Dec 10, 2007 6:07 pm
by VladSun
Wrong answer ;) :

Code: Select all

// check gender
// very basic validation, check string is either 'male' or 'female'
function check_is_gender($string)
{
   if (($string != 'male') || ($string != 'female'))
   {
   return false;
   }
   else
   {
   return true;
   }
}
Use ctype_digit() instead this function:

Code: Select all

// check $input is only digits, return false if not
function check_is_numeric($input) {
  if(preg_match("^[0-9]$", $input))
   {
      return true;
   }
   else
   {
      return false;
   }
}
Use ctype_alpha() instead this function:

Code: Select all

// check $input is only letter, return false if not
function check_is_alpha($input) {
   if(preg_match ("^[A-Za-z]$", $input))
   {
      return true;
   }
   else
   {
      return false;
   }
}
You can use this structrure:

Code: Select all

return (a==b);
insetad of:

Code: Select all

if (a==b)
{
  return true;
}
else
{
  return false;
}

Posted: Mon Dec 10, 2007 6:15 pm
by newbie2php
Hi VladSun, cheers.

Wrong answer - is it? If the $string is not equal to "male", or "female" return false? I thought that was correct?

Thanks fot them functions, never seen them before, do they do the same thing? Will look them up.

(a==b) - can you explain this a little more if possible? Where do I use that structure?

Thanks

Posted: Mon Dec 10, 2007 6:27 pm
by VladSun
newbie2php wrote: Wrong answer - is it? If the $string is not equal to "male", or "female" return false? I thought that was correct?
Suppose we have $string="male", then $string != "female" is true so you return false because of the "OR" ;)
newbie2php wrote: (a==b) - can you explain this a little more if possible? Where do I use that structure?

Thanks
E.g.

Code: Select all

function check_is_numeric($input) {
  return (preg_match("^[0-9]$", $input));
}

Posted: Mon Dec 10, 2007 6:33 pm
by Benjamin
Regex is preferred and should be used in place of ctype_digit for get/post/cookie data.

Posted: Mon Dec 10, 2007 6:38 pm
by VladSun
astions wrote:Regex is preferred and should be used in place of ctype_digit for get/post/cookie data.
Reasons? I do also prefer regex, but for more complicated cases, not so trivial...

Posted: Mon Dec 10, 2007 6:43 pm
by Benjamin
Because it can return false in certain circumstances with valid integers depending on what the programmer or PHP decided to type cast it as. Unless the programmer wants to type cast all data to a known state before testing it with ctype_digit, they are better off just using regex.

Posted: Mon Dec 10, 2007 6:48 pm
by VladSun
astions wrote:Because it can return false in certain circumstances with valid integers depending on what the programmer or PHP decided to type cast it as. Unless the programmer wants to type cast all data to a known state before testing it with ctype_digit, they are better off just using regex.
astions wrote:Regex is preferred and should be used in place of ctype_digit for get/post/cookie data.
GET/POST/COOKIE values are always string type as far as I know ...
It is quicker to typecast the variable than using regex ...

Code: Select all

ctype_digit((string)$value)
PS: Scalar values are string types ...

Posted: Mon Dec 10, 2007 6:58 pm
by Benjamin
Not for me preg_match('#^[0-9]{1,12}$#', $var)

Probably took me less time to type that than to type ctype_digit((string)$var), but that's besides the point as I have seen posted data that is of type INT. This can lead to some hard to track, obscure and intermittent bugs. From experience, I am recommending regex.

Posted: Mon Dec 10, 2007 7:07 pm
by VladSun

Posted: Mon Dec 10, 2007 7:17 pm
by Benjamin
I'm not going to argue with you. I have seen it happen.

Posted: Mon Dec 10, 2007 7:18 pm
by Weirdan
btw, it's better to use \z instead of $ when validating input. regexps with $ would allow one trailing newline to be put at the end of the input field, which may or may not be harmful depending on how you use them later (for example if you allow trailing newline in email field your mail submissions to your users could be vulnerable to header injection). See Stefan Esser's article on this issue: http://blog.php-security.org/archives/7 ... lters.html

Posted: Mon Dec 10, 2007 7:20 pm
by Weirdan
astions wrote:I'm not going to argue with you. I have seen it happen.
That is, ctype_* are locale-dependent.