are the Notice's relevant?

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

Post Reply
edhen
Forum Newbie
Posts: 21
Joined: Fri Jul 16, 2010 11:26 am
Location: Adelaide, Australia

are the Notice's relevant?

Post by edhen »

Hi, i had start developing my site with my web host and ftp'd changes then refreshing the page to debug and progress. while this was working it was just bad work flow so i decided to port everything over to wamp and develop on my machine before i decide to finalize my site. Now with my wamp compared to the web-host i noticed that notice's are turned off on the web-host yet my wamp is not (which is understandable).. now once my register form was processed through my web-host all went ok beause there were no notice about undefined variables and just ignored them... now on my wamp server obviously i get notices about undefined variables which causes my process script to display these notices instead of processing it all and showing a confirmation page... my main question is... would defining these variable's be 100% necessary? and would it cause any security risks if there not defined? if not would it be safe to turn of notices?
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: are the Notice's relevant?

Post by Benjamin »

Yes, it is best practice to write code with no notices. PHP allows you to use undefined variables, whereas most other languages wouldn't even parse correctly. To give you specific information we need to see your coding style and see what you've done. Feel free to post your code.
User avatar
pickle
Briney Mod
Posts: 6445
Joined: Mon Jan 19, 2004 6:11 pm
Location: 53.01N x 112.48W
Contact:

Re: are the Notice's relevant?

Post by pickle »

"necessary" is a tricky thing. It's not "necessary" to code HTML to web standards, but it's highly recommended, simplifies debugging, and is considered best practice. The same holds true for getting rid of notices. The PHP code will still run, but you should generally get rid of them. They're almost always trivial to fix anyway.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.
edhen
Forum Newbie
Posts: 21
Joined: Fri Jul 16, 2010 11:26 am
Location: Adelaide, Australia

Re: are the Notice's relevant?

Post by edhen »

Hey,

Thanks for the reply....

this part is the form to fill out to register

Code: Select all

require ('../admin/system/application-top.php');

if (!defined('ROOT_PATH')) {
  die("Security violation");
}

if ($_SESSION['level'] > GUEST) {
	$msg = 'Your already registered';
	header('Location: '.HTTP_PATH);
	exit($msg);
}

$page_name = 'register';

//template top
require_once (TEMPLATE_TOP);

$error = $_SESSION["error"];
$_POST['required'] = $_SESSION['required'];
$_POST['optional'] = $_SESSION['optional'];

?>
<font size="-2">* fields are required</font>
<br />
<br />
<form action="process.php" method="post" enctype="multipart/form-data" name="register">
<table style="margin:0 auto;" width="90%" border="0" cellpadding="2">
  <tr>
    <td><label>First Name</label></td>
    <td><input name="required[fname]" <?php if(!$error[0] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="text" size="32" maxlength="34" value="<?php echo $_POST['required']['fname'];  ?>" >*</td>
    <td><h6><?php echo $error[0] ?></h6></td>
  </tr>
  <tr>
    <td><label>Last Name</label></td>
    <td><input name="required[lname]" <?php if(!$error[1] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="text" size="32" maxlength="34" value="<?php echo $_POST['required']['lname'];  ?>" >*</td>
    <td><h6><?php echo $error[1] ?></h6></td>
  </tr>
  <tr>
    <td><label>Artist/Band/Nick Name</label></td>
    <td><input name="required[aname]" <?php if(!$error[2] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="text" size="32" maxlength="255" value="<?php echo $_POST['required']['aname'];  ?>" >*</td>
    <td><h6><?php echo $error[2] ?></h6></td>
  </tr>
  <tr>
    <td style="border-bottom: solid 1px #666;" colspan="3"></td>
  </tr>
  <tr>
    <td><label>Password</label></td>
    <td><input name="required[pass]" <?php if(!$error[3] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="password" size="32" maxlength="16" value="<?php echo $_POST['required']['pass']; ?>" >*</td>
    <td><h6><?php echo $error[3] ?></h6></td>
  </tr>
  <tr>
    <td><label>Confirm Password</label></td>
    <td><input name="required[confirmpass]" <?php if(!$error[4] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="password" size="32" maxlength="16" value="<?php echo $_POST['required']['confirmpass'];  ?>" >*</td>
    <td><h6><?php echo $error[4] ?></h6></td>
  </tr>
  <tr>
    <td style="border-bottom: solid 1px #666;" colspan="3"></td>
  </tr>
  <tr>
    <td><label>Email Address</label></td>
    <td><input name="required[email]" <?php if(!$error[5] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> type="text" size="32" maxlength="32" value="<?php echo $_POST['required']['email']; ?>" >*</td>
    <td><h6><?php echo $error[5] ?></h6> </td>
  </tr>
  <tr>
    <td style="border-bottom: solid 1px #666;" colspan="3"></td>
  </tr>
  <tr>
    <td><label>Country</label></td>
    <td>
    <select <?php if(!$error[6] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> id='countrySelect' name="required[country]" onchange='populateState()' >
    <option selected="selected" value="<? echo $_POST['required']['country']; ?>"><? echo $_POST['required']['confirmpass']; ?></option>
    </select>*
    </td>
    <td><h6><?php echo $error[6] ?></h6></td>
  </tr>
  <tr>
    <td><label>State</label></td>
    <td><select <?php if(!$error[7] =="") { echo 'style="background-color:#FFBFBF;"'; } ?> id='stateSelect' name="required[state]">
        </select>
        <script type="text/javascript">initCountry('AU'); </script>*</td>
    <td><h6><?php echo $error[7] ?></h6></td>
  </tr>
  <tr>
    <td style="border-bottom: solid 1px #666;" colspan="3"></td>
  </tr>
  <tr>
    <td><label>Date of Birth</label></td>
    <td>
    <select style="width: 45px;" name="required[DOB_day]"><? $DOB_day = $_POST['required']['DOB_day']; ?>
        <?php 
		$DOB_day = array('1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19', '20', '21', '22', '23', '24', '25', '26', '27', '28', '29', '30', '31'); //List all months
    $selected = '1';
    if (!empty($_POST)) {
        $selected = $_POST['required']['DOB_day'];
    }

    foreach($DOB_day as $key) {
        echo '<option ' . ($selected == $key ? 'selected="selected"' : '') . ' value="' . $key . '">' . $key . '</option>';
    }
	?>
    	
    </select> 
        /
	<select name="required[DOB_month]">
	<?php
    $DOB_month = array('1' => 'JAN', '2' => 'FEB', '3' => 'MAR', '4' => 'APR', '5' => 'MAY', '6' => 'JUN', '7' => 'JUL', '8' => 'AUG', '9' => 'SEP', '10' => 'OCT', '11' => 'NOV', '12' => 'DEC'); //List all months
    $selected = '1';
    if (!empty($_POST)) {
        $selected = $_POST['required']['DOB_month'];
    }

    foreach($DOB_month as $key => $val) {
        echo '<option ' . ($selected == $key ? 'selected="selected"' : '') . ' value="' . $key . '">' . $val . '</option>';
    }
	?>
	</select>
    	/  
    <select style="width: 60px;" name="required[DOB_year]" >
    <?php
	$selected = '1';
	if (!empty($_POST)) {
    $selected = $_POST['required']['DOB_year'];
    }
	for ($y = 2005; ; $y--){
		echo '<option ' . ($selected == $y ? 'selected="selected"' : '') . ' value="'.$y.'">'.$y.'</option><br/>';
		
		if ($y == 1920){
		break;
		}
	}
?>
    </select>* 
        </td>
    <td>&nbsp;</td>
  </tr>
  <tr>
    <td><label>Gender</label></td>
    <td>
    <select name="required[gender]"> 
    	<option <? if ($_POST['required']['gender'] == 'M') { echo 'selected="selected"'; } ?>value="M">Male</option>
        <option <? if ($_POST['required']['gender'] == 'F') { echo 'selected="selected"'; } ?>value="F">Female</option>
    </select>*
    </td>
    <td><h6><?php echo $error[11] ?></h6></td>
  </tr>
  <tr>
    <td><label>Are You An Artist/Singer?</label></td>
    <td><input name="optional[artist]" <? if ($_POST['optional']['artist'] == '1') { echo 'checked="checked"'; } ?> type="checkbox"  value="1" ></td>
    <td>&nbsp;</td>
  </tr>
  <tr>
    <td><label>Are You A Producer?</label></td>
    <td><input name="optional[producer]" <? if ($_POST['optional']['producer'] == '1') { echo 'checked="checked"'; } ?> type="checkbox" value="1" ></td>
    <td>&nbsp;</td>
  </tr>
  <tr>
    <td><label>Label</label></td>
    <td><input name="optional[label]" type="text" size="32" maxlength="255" value="<?php echo $_POST['optional']['label'];  ?>" ></td>
    <td><h6><?php echo $error[14] ?></h6> </td>
  </tr>
  <tr>
    <td style="border-bottom: solid 1px #666;" colspan="3"></td>
  </tr>
  <tr>
    <td><label>Newsletter</label></td>
    <td><input name="optional[newsletter]" <? if ($_POST['optional']['newsletter'] == '1') { echo 'checked="checked"'; } ?> type="checkbox" value="1" ></td>
    <td>&nbsp;</td>
  </tr>
  <tr>
    <td><label>Avatar <font size="-2" >(JPG, GIF, PNG, BMP) *2MB max</font></label></td>
    <td><input size="45" name="optional[avatar]" type="file"></td>
    <td><h6><?php echo $error[16] ?></h6></td>
  </tr>
  <tr>
    <td><label><font size="1">You Agree To Terms & Conditions? <br />
				(click here to read)</font></label></td>
    <td><input name="required[terms]" <? if ($_POST['required']['terms'] == '1') { echo 'checked="checked"'; } ?> type="checkbox" value="1" ></td>
    <td><h6><?php echo $error[17] ?></h6></td>
  </tr>
  <input type="hidden" name="sent_r" value="1" />
  <input type="hidden" name="MAX_FILE_SIZE" value="2097152" />
</table>
<div style="text-align:right;">
<input style="padding:5px 10px 5px 10px;" name="submit" type="submit" value="submit">
</div>

</form>
 
<?php

unset($_POST['submit']);

session_destroy ();

//template bottom
require_once (TEMPLATE_BOTTOM);

require (SYSTEM_PATH . 'application-bottom.php');

?> 

Now my process script is...

Code: Select all

<?php
require ('../admin/system/application-top.php');


//set vars
if(isset($_POST['sent_r'])) {
	$register = $_POST['sent_r'];
} else {
	$register = 0;
}

if(isset($_POST['sent_c'])) {
	$crop = $_POST['sent_c'];
} else {
	$crop = 0;
}

//check to see whether the forms were sent
if (!isset($register) && !isset($crop)) {
	die ('security violation');
}

//process cropping
if ($crop == 1) {
	include(FUNCTIONS_PATH . 'image/process.php');
}

//Process registration
if ($register == 1) { 	
	

	if(isset($_POST['required']['fname'])) { $fname = $_POST['required']['fname']; } else { $fname = false; }
	if(isset($_POST['required']['lname'])) { $lname = $_POST['required']['lname']; } else { $lname = false; }
	if(isset($_POST['required']['aname'])) { $aname = $_POST['required']['aname']; } else { $aname = false; }
	if(isset($_POST['required']['pass'])) { $pass = $_POST['required']['pass']; } else { $pass = false; }
	if(isset($_POST['required']['confirmpass'])) { $confirmpass = $_POST['required']['confirmpass']; } else { $confirmpass = false; }
	if(isset($_POST['required']['email'])) { $email = $_POST['required']['email']; } else { $email = false; }
	if(isset($_POST['required']['country'])) { $country = $_POST['required']['country']; } else { $country = false; }
	if(isset($_POST['required']['state'])) { $state = $_POST['required']['state']; } else { $state = false; }
	if(isset($_POST['required']['DOB_day'])) { $DOB_day = $_POST['required']['DOB_day']; } else { $DOB_day = false; }
	if(isset($_POST['required']['DOB_mont'])) { $DOB_month = $_POST['required']['DOB_month']; } else { $DOB_month = false; }
	if(isset($_POST['required']['DOB_year'])) { $DOB_year = $_POST['required']['DOB_year']; } else { $DOB_year = false; }
	if(isset($_POST['required']['gender'])) { $gender = $_POST['required']['gender']; } else { $gender = false; }
	if(isset($_POST['optional']['artist'])) { $artist = $_POST['optional']['artist']; } else { $artist = false; }
	if(isset($_POST['optional']['producer'])) { $producer = $_POST['optional']['producer']; } else {$producer = false; }
	if(isset($_POST['optional']['label'])) { $label = $_POST['optional']['label'];	} else { $label = false; }
	if(isset($_POST['optional']['newsletter'])) { $newsletter = $_POST['optional']['newsletter']; } else { $newsletter = 0; }
	if(isset($_POST['optional']['avatar'])) { $avatar = $_FILES['optional']['tmp_name']['avatar']; } else { $avatar = false; }
	if ($artist == '') {$artist = '0';}
	if ($producer == '') {$producer = '0';}
	if ($newsletter == '') {$newsletter = '0';}
	
	$_SESSION['fname'] = $fname;
	
  	//fname validation
	
	if($fname !== "") {
		if(strlen($fname) > 34) {
		$error[0] = 'Your first name must be equal to or below 34 characters';
		die ('violation has occured 1');
		}
		
		if(preg_match("/[^A-Za-z']/", $fname)) {
		$error[0] = 'First name may only contain letters';
		}
		
	} else {
		$error[0] = 'Please enter your first name';
	}
	
	//lname validaion
	if ($lname !=="") { 
		if(strlen($lname) > 34) {
		$error[1] = 'Your last name must be equal to or below 34 characters';
		die ('violation has occured 2');
		}
		
		if(preg_match("/[^A-Za-z']/", $lname)) {
		$error[1] = 'Last name may only contain letters';
		}
		
	} else {
		$error[1] = 'Please enter your last name';
	}
	
	//aname validation
	if ($aname !=="") { 
		if(strlen($aname) > 255) {
			$error[2] = 'Your artist/band/nick name cannot be longer than 255 characters';
			die ('violation has occured 3');
		}
	} else {
		$error[2] = 'Please enter your artist/band/nick name';
	}
	
	//password validation
	if($pass !== "") {
		if (strlen($pass) < 6 || strlen($pass) > 14) { 
			$error[3] = 'Please make sure your password is between 6 - 14 characters long';
		}
		
	} else {
		$error[3] = 'Please enter a password';
	}
	
	//password confirm validation
	if ($confirmpass !=="") {
		if(!strcmp($pass, $confirmpass) == 0) {
			$error[4] = 'Your passwords do not match';
		}
	} else {
		$error[4] = 'Please confirm your password';
	}
	
	//email validation
	$checkemail = mysqli_query($mysqli, "SELECT email FROM users WHERE email='$email'");
	$email_exists = mysqli_num_rows($checkemail);
	
	if($email !== "") { 
    	if(preg_match("/^[-A-Za-z0-9_]+[-A-Za-z0-9_.]*[@]{1}[-A-Za-z0-9_]+[-A-Za-z0-9_.]*[.]{1}[A-Za-z]{2,5}$/", $email)) { 
			if ($email_exists > 0) {
		   $error[5] = 'Email address already exists';
	   		}
    	} else { 
		$error[5] = 'Not a valid email address';
		}
  	
	} else { 
    $error[5] = 'Please enter your email address'; 
  	}
	
	//country validation
	if($country !== "") {
		if(strlen($country) > 2) {
		$error[6] = '!ERROR Invalid country code';
		die ('violation has occured 4');
		}
	} else {
		$error[6] = 'A country has not been selected';
	}
	
	//state
	if($state !== "") {
		if(strlen($state) > 64) {
			$error[7] = 'Invalid state (exceeds maximum characters)';
		} 
	} else {
		$error[7] = 'Please select your state';
	}
	
	//Gender validation
	if($gender !=='M' && $gender !=='F') {
		$error[11] = 'Invalid gender format';
		die ('violation has occured 5');
	}
	
	//Avatar validation
	if($avatar) {
	$avatar_t = $_FILES['optional']['tmp_name']['avatar'];
	$avatar_n = $_FILES['optional']['name']['avatar'];
	$info = pathinfo($avatar_n);
	$ext = $info['extension'];
	
		if(filesize($avatar_t) > 2097152) {
			$error[16] = 'File is too large, MAX file size is 2MB';
		}
		if(($ext !=='gif') && ($ext!=='jpg') && ($ext!=='png') && ($ext!=='bmp')) {
			$error[16] = 'Invalid file format. Only JPG, GIF, PNG or BMP allowed';
		}
	}
	
	//if not errors then format cleanly and enter into the database
	if($error > 0){
		
		$_SESSION["error"] = $error;
		$_SESSION['required'] = $_POST['required'];
		$_SESSION['optional'] = $_POST['optional'];
		
		//include (ROOT_PATH.'/register/index.php');
		header('Location: index.php');

	} else {
		//check if id is available if not repeat till it finds 1
		
		$id =  mt_rand(100000000, 999999999);
		$checkid = mysqli_query($mysqli, "SELECT id FROM users WHERE id='$id'");
		$idexists = mysqli_num_rows($checkid);
		if ($idexists > 0) {
			for ($id; ; mt_rand(1, 5)) {
				if ($idexists == 0)
				break;
			}
		}
		

		//Ready everything prior to entry in database
		
		$fname = ucwords($fname);
		$lname = ucwords($lname);
		$aname = strtoupper($aname);
		
		$pass = md5($pass);
		$email = strtolower($email);
		
		$DOB = $DOB_day . '-' . $DOB_month . '-' . $DOB_year;
		
		$label = strtoupper($label);
		
		$register_date = date('d-m-Y - H');
		
		$validation_code = md5($id);
		
		$level = PROCESSING;
        
		$q = "INSERT INTO users (id, fname, lname, aname, pass, email, country, state, DOB, gender, newsletter, artist, producer, label, registered_date, validation_code, level)
				VALUES('$id', '$fname', '$lname', '$aname', '$pass', '$email', '$country', '$state', '$DOB', '$gender', '$newsletter', '$artist', '$producer', '$label',  '$register_date', '$validation_code', '$level')";
				
		mysqli_query($mysqli, $q) or die();
				
		//create users directories
		mkdir(USER_PATH . $id. '/', 0711);
		mkdir(USER_PATH . $id. '/tmp', 0711);
		mkdir(USER_PATH . $id. '/pics', 0744);
		mkdir(USER_PATH . $id. '/pics/bg', 0744);
		mkdir(USER_PATH . $id. '/audio', 0744);
		
		
				if ($avatar) {
					
					include (INCLUDE_PATH . 'mail_validation.php');
					
					$des = USER_PATH . $id . '/tmp/avatar.' . $ext;
					$des_jpg = USER_PATH . $id . '/tmp/avatar.jpg';
					
					move_uploaded_file($avatar_t, $des);
					
					if ($ext=='png' || $ext=='gif' || $ext=='bmp') {
						$mw = NewMagickWand();
						MagickReadImage($mw,$des);
						$w = MagickGetImageWidth($mw);
						$h = MagickGetImageHeight($mw);
						
						MagickSetFormat($mw, "jpeg");
						
						MagickWriteImage($mw, $des_jpg);
						unlink($des);
						$des = $des_jpg;
					}	
					
						
					$_SESSION['id'] = $id;
					$_SESSION['file'] = $des;
					$_SESSION['name'] = $aname;
					
					header( "Location: crop.php");
					
					exit;
					
				} else {
					
				$_SESSION['name'] = $aname;
				$_SESSION['id'] = $id;
				include (INCLUDE_PATH . 'mail_validation.php');
				header ( "Location: confirmed.php" );
			
				}
				
		}
}

?>
Mind you this is my first php script Ive made and going by what Ive learnt from tutorials etc. so please be kind on my coding.. At the top you will notice the if(isset($_post.......... this was to overcome those notices and i didn't change it back.

ok so in the registration form i get a whole bunch of notices because nothing was defined... but thats obviously the case because the form hasn't even been processed... it seemed to work by isset everything and go through each bit by bit but seemed like a nightmare just to do that.... this was why i'm if i should take this notice seriously or if its ok to skip some...

thanks in advance...
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: are the Notice's relevant?

Post by Mordred »

There are multiple SQL injection opportunities, read up on mysql_real_escape_string() and SQL injection.
edhen
Forum Newbie
Posts: 21
Joined: Fri Jul 16, 2010 11:26 am
Location: Adelaide, Australia

Re: are the Notice's relevant?

Post by edhen »

Oh yh i forgot to mention that i took out some security functions temporarily like mysql_real_escape_string() & addslashes etc only for formatting purposes (it was intentional and this code is not accessed by the web as of yet).. i actually had the mysql_real_escape_string() up top to realize that the effect was also taken place back on my form so i was going to move it down just before db entry... but i just want the scripts to work without problems before i get stuck into security coz if i just start adding this and that then it could just give me more problems to deal with... i like to work at 1 thing at a time kind of way... thanks though sql injections is still something i need to learn more about though and will implement them once the basics are out the way and that the script behaves through all processes..
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: are the Notice's relevant?

Post by Mordred »

Unless you properly escape, it will not behave correctly, fact of life :/ Sometimes you may not have exploitable security holes, but not escaping properly will still make your data incorrect.
Old rant: Escaping is NOT a Security Measure
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: are the Notice's relevant?

Post by Benjamin »

I'm not sure why you have named your post field names in such a way that they are posted as arrays. Change them to strings and you will be able to simplify your code.

You can write a wrapper function for the post data, so that you do not need to check if they are set before using them:

Code: Select all

function post($k) {
    return empty($_POST[$k]) ? null : trim($_POST[$k]);
}
Then in your form:

Code: Select all

<input type="text" name="foo" value="<?php echo post('foo'); ?>" />
Also, you are generating your own user ID's. It would be better to simply use the auto_increment field type in your database table. You can easily get the ID after an insert if you need it.

Short tags are bad, don't use <?.

You may want to start looking into frameworks such as cakePHP.
edhen
Forum Newbie
Posts: 21
Joined: Fri Jul 16, 2010 11:26 am
Location: Adelaide, Australia

Re: are the Notice's relevant?

Post by edhen »

Thanks Benjamin for your input a lot has been taken into account.. Regarding the Post's as arrays happened when i was first writing the form and was thinking about the best way to go about it (this was 2 weeks ago) as I was going to do a loop for the arrays as a 1 hit wonder... but eventually i really haven't found it useful and realized it really didn't do much to help, But I pretty much kept it as it is as the scripts are already written out for them. but because you mentioned it and caught your attention then im going to go through and change it now :). also regarding the <? short tags, i read php in a nutshell by O'reilly and learnt not to use <? short tags because of compatibility issues and ever since i have been using the standard <?php tags i just haven't backtracked to fix them all up... Oh and the main reason for ID is to have profile?id= pages looking the same and not really like incremented system eg. a person having id=5 then 1 having id=107 just doesn't appeal to me... I will also have a look into cakephp and see how it turns out
Post Reply