List Meny PREDEFINED Values validation..

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

List Meny PREDEFINED Values validation..

Post by NTGr »

Hello.
its the first time i m trying to secure what user ender in form field..so please be gentle with me :oops:
Lets say that i got a simple form...

Code: Select all

$test='<form  action="" method="post"  name="upl_form">
	<select name="categoryboth">
	<option value="">Pls Select One Option Of This Field</option>
			'.$photo_category_list.'
	</select>
	'.$error_cat.'
	<input  name="submit" type="submit"  value="Submit" />';
where $categoryboth...

Code: Select all

//Building Category
				 $photo_category_list  = "";
				 $allowed_category = array();
				 $result = mysql_query( "SELECT category_id,category_name FROM gallery_category ORDER BY category_name" );
				 while( $row = mysql_fetch_array( $result ) )
				 {
					  $photo_category_list .= '<option value="'.$row[0].'|'.$row[1].'" '.($row[0].'|'.$row[1]== $_POST["categoryboth"]? "selected=\"selected\"":"").'>'.$row[1].'</option>';
					  $allowed_category[]= $row[0].'|'.$row[1] ;
				 }
				 mysql_free_result( $result );
that create a list menu like this..

Code: Select all

<option value="">Pls Select One Option Of This Field</option>
<option value="2|EuroDance" >EuroDance</option>
<option value="7|Hip-Hop" >Hip-Hop</option>
<option value="1|Italo" >Italo</option>
Im trying also to create an allowed values array at the same time...seems that is working..but is secure???

OK now the validation part...

Code: Select all

$categoryboth=$_POST['categoryboth'];
	$error_msg='';
	$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
	$error_cat=(isset($categoryboth) && $categoryboth=="" && $categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
	$error_msg=(isset($categoryboth) && $categoryboth=="" && $categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
and...

Code: Select all

if ($error_msg!='')       ///<<<<<<<<<<<<<<<<<<< E r r o r s Display Form Again 
{
 redisplay form
}
 else
{
//       Insert values in my DB

  $query = "INSERT INTO database('categoryboth', ....etc....) VALUES ('my_quote($categoryboth)',  ....etc...) ";
	$result = mysql_query($query);
	if (!$result)
	{
		echo "<br>Error with query is ".mysql_error();
	}
	else
	{ 
		echo " CONGRATULATION !!!!!......etc........";
	} 
}
where my_quote()...

Code: Select all

function my_quote( $value )
{
    if( get_magic_quotes_gpc() )
    {
          $value = stripslashes( $value );
    }
    //check if this function exists
    if( function_exists( "mysql_real_escape_string" ) )
    {
          $value = mysql_real_escape_string( $value );
    }
    //for PHP version < 4.3.0 use addslashes
    else
    {
          $value = addslashes( $value );
    }
    return $value;
}
I m doing something wrong??
My validation is safe ??
Thanks.....
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

Your method of creating an "allowed categories" array is intuitive. There are other ways to do it, but most of them require querying the database later on to check if the choice was valid.

Everything looks solid aside from you not checking if $_POST['categoryboth'] exists before using it, but you've probably already done that beyond the code we see here.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

This won't work:

Code: Select all

" (snip) VALUES ('my_quote($categoryboth)', (snip) "
should be:

Code: Select all

" (snip) VALUES ('" . my_quote($categoryboth) ."', (snip) "
And anyway, my_quote() is an evil (and stupid) function, and you are advised not to use it, as it will introduce subtle bugs (not security threats though) under magic_quotes on. I know it's been in the PHP documentation, but there you go, it's still stupid. It is better to check for magic quotes in the beginning of your script, stripslashes all GPC variables if so, and then go on with a "normal" escaping routine, namely mysql_real_escape_string()

Code: Select all

if (!$result) 
        { 
                echo "<br>Error with query is ".mysql_error(); 
        }
This is unwise and unsecure. The same effect can be achieved with trigger_error, and in a production environment you should not display the error messages, but log them in a file.


As for your implementation, I suggest you use only the category_id, the category_name should be used only for display purposes.
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

Post by NTGr »

superdezign..thank you for the answer....
Your method of creating an "allowed categories" array is intuitive
what you mean..my dictionary is not giving me the right meaning of the word intuitive :)
aside from you not checking if $_POST['categoryboth'] exists before using it
You mean something like this???

Code: Select all

$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
Mordred thank you too ...
should be:
Code:
" (snip) VALUES ('" . my_quote($categoryboth) ."', (snip) "
yes you r right..Thank you
And anyway, my_quote() is an evil (and stupid) function, and you are advised not to use it
8O when i submited this post i was only sure that my_quote() was OK ..seems that i must reconsider this
This is unwise and unsecure. The same effect can be achieved with trigger_error,
yes i know that..but right now i want to learn how to validate correctly my forms...

Thank you both once again
:D
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

CORRECTION...

Post by NTGr »

seems that sould be...

Code: Select all

$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
$error_msg='';
$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
$error_cat=($categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
$error_msg=($categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
i think is BETTER NOW...
IS IT :?:
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

I meant for you to check if it's set before even trying to perform any operations on it. Why waste time processing nothing?


And your "my_quote" function isn't.. normal. Use mysql_real_escape_string instead and if magic quotes are on, clean them elsewhere or turn them off. (Basically what Mordred just said, but some repetition may keep it in mind).
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

better ??

Post by NTGr »

Code: Select all

if(!isset( $_POST['categoryboth']) || $_POST['categoryboth']=== '') 
{
	$error_cat='<font color="red"> Error in <strong>Category</strong></font>' ;
	$error_msg=$error_msg+1 ;
}
else
{
	$categoryboth=(isset($_POST['categoryboth']) ? $_POST['categoryboth'] : "" );
	$error_msg='';
	$categorybothA=(is_array($allowed_category) && !in_array($categoryboth,$allowed_category) ? true : false);
	$error_cat=($categorybothA==true ? '<font color="red"> Error in <strong>Category</strong></font>' : " "  );
	$error_msg=($categorybothA==true ?  $error_msg+1  : $error_msg  ) ;
}
and....

Code: Select all

function my_quote( $value )
{
	 if (get_magic_quotes_gpc()) 
	 {
		$value = stripslashes($value);
	 }
	else
	{
	 $value = " . mysql_real_escape_string($value) . ";
	 }
	return $value;
}
BETTER NOW??? :?
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Re: better ??

Post by superdezign »

Firstly, this:
NTGr wrote:

Code: Select all

if(!isset( $_POST['categoryboth']) || $_POST['categoryboth']=== '')
Can be rewritten as this:

Code: Select all

if(empty($_POST['categoryboth']))
However, if you're not checking at all before that, then even if people don't get a chance to make an entry, they'll get an error message.

And we've said multiple times... my_quote shouldn't be there at all. If you have magic_quotes enabled, clean them upon load (in all of GPC) instead of just going into your database, and use mysql_real_escape_string to escape data going into the database.
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

Post by NTGr »

thank you oce again superdezign !!!!!!!

Reading in
http://www.php.net/manual/en/security.m ... .php#55935
to turn of magic quotes put the following line into the .htaccess file:

php_flag magic_quotes_gpc off
So i think that is what you wanted to say ...
Can be rewritten as this:

if(empty($_POST['categoryboth']))
Thank you for the correction.
However, if you're not checking at all before that, then even if people don't get a chance to make an entry, they'll get an error message.
Sure ...

Code: Select all

if(isset($submit))
{
  validation goes here...etc...
}
I hope that his time i v made all things correctly !!!
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

I wrote:Looking for the submit button is not proper. Why? Because it's quite possible to submit the form (normally) without the submit button. Look for more specific data, that must be there or use $_SERVER['REQUEST_METHOD']. You still must use isset()/array_key_exists()/empty() however to detect which fields are there or not just to make sure you don't drop errors.
;)
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

NTGr wrote:Reading in
http://www.php.net/manual/en/security.m ... .php#55935
to turn of magic quotes put the following line into the .htaccess file:

php_flag magic_quotes_gpc off
So i think that is what you wanted to say ...
Not quite what I said, but that is a valid method.
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

Post by NTGr »

Thank you superdezign....once again!!!
Can you please be more specific ???
A sample code maybe ???
I dont want to give me the exact answer BUT at least give me a way to discover what you mean...
I know,you explain this several times ..but i dont get it....sorry :oops:
User avatar
superdezign
DevNet Master
Posts: 4135
Joined: Sat Jan 20, 2007 11:06 pm

Post by superdezign »

Code: Select all

if(get_magic_quotes_gpc())
{
	function strip_gpc_slashes(&$array)
	{
		if(!is_array($array))
		{
			return;
		}
		foreach($array as $key => $val)
		{
			is_array($array[$key]) ? strip_gpc_slashes($array[$key]) : ($array[$key] = stripslashes($array[$key]));
        }
	}
	
	$gpc = array(&$_GET, &$_POST, &$_COOKIE, &$_REQUEST, &$_FILES);
	strip_gpc_slashes($gpc);
}
I didn't write this, but I always use it. The .htaccess way is perfectly valid as well. I just prefer not to clog up mine, that's all. Yours is probably already clean.
NTGr
Forum Newbie
Posts: 14
Joined: Fri Mar 30, 2007 11:28 am

Post by NTGr »

Thank you superdezign :D
I didn't write this, but I always use it. The .htaccess way is perfectly valid as well. I just prefer not to clog up mine, that's all. Yours is probably already clean.
Yes its clean sicnce i havent touch it yet !!!!
Thank you once again 8)

feydwrote..
Looking for the submit button is not proper. Why? Because it's quite possible to submit the form (normally) without the submit button. Look for more specific data, that must be there or use $_SERVER['REQUEST_METHOD']. You still must use isset()/array_key_exists()/empty() however to detect which fields are there or not just to make sure you don't drop errors.
You mean that i must create an array with the submited values and check if all submited???
A hidden field is OK to check for , insted of $submit??
Thank you.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

NTGr wrote:You mean that i must create an array with the submited values and check if all submited???
A hidden field is OK to check for , insted of $submit??
Thank you.
Don't blindly use submission values. Check for their existence before use. There's no exceptions to this really.
Post Reply