Critique my php scripts | All criticism/sggestions/improvements appreciated

[Minimeallolla]

All criticism/suggestions/improvements appreciated

 

Registration.php

<?php
$con = mysql_connect("localhost","","") or die(mysql_error());
mysql_select_db('Users');

if(isset($_COOKIE['ID_my_site'])) {
$cookie_username = mysql_real_escape_string(filter_input(INPUT_COOKIE, 'ID_', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$cookie_password = sha1($_COOKIE['Key_']);
$cookie_check = mysql_query("SELECT * FROM Users WHERE username = '$cookie_username'") or die(mysql_error());
$cookie_results = mysql_fetch_array($cookie_check);

	if ($cookie_password == $cookie_results['Password']) {
		echo "<div id=\"login_msg\">You are already logged on. Redirecting...</div><br />" && header("location:/index.php");
	}
}

if(isset($_POST['submit']))	{
$Username = mysql_real_escape_string(filter_input(INPUT_POST, 'Username', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$Email = mysql_real_escape_string(filter_input(INPUT_POST, 'Email', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$Password = sha1($_POST['Password']);
$Password2 = sha1($_POST['Password2']);

if (!$Username | !$Email | !$Password | !$Passord2) {
echo "<div id=\"error_msg\">You did not complete all of the required fields, please try again.</div><br />";
}

if ($Password != $Password2) {
echo "<div id=\"error_msg\">Your passwords do not match, please try again.</div><br />";
}

$check_username = mysql_query("SELECT * FROM Users WHERE (Username = $Username)");
$result_username = mysql_fetch_row($check_username);
$check_email = mysql_query("SELECT * FROM Users WHERE (Email = $Email)");
$result_email = mysql_fetch_row($check_email);

if ($result_username == true) {
echo "<div id=\"error_msg\">The Username: '$Username', already exists. Please enter another username.</div><br />";
}

if ($result_email == true) {
echo "<div id=\"error_msg\">The Email Adress: '$Email', is already in our Database.</div><br />";
}

$sql = "INSERT INTO Users (Id, Username, Email, Password) VALUES ('', '$Username','$Email','$Password')";
$add_member = mysql_query($sql) or die(mysql_error());

if (mysql_query($add_member)) {
	$week = time() + 604800; 
	setcookie(ID_, $_POST['Username'], $week); 
	setcookie(Key_, $_POST['Password'], $week);
	echo "<div id=\"login_msg\">Successfully added to our Database.</div><br />"  &&  header ("location:/Login.php"); 
}

else {
	echo "<div id=\"error_msg\">Invalid input.</div><br />"; 
}
}
?>

 

Login.php

<?php
include("db.php");

if(isset($_COOKIE['ID_my_site'])) {
$cookie_username = mysql_real_escape_string(filter_input(INPUT_COOKIE, 'ID_', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$cookie_password = sha1($_COOKIE['Key_']);
$cookie_check = mysql_query("SELECT * FROM Users WHERE username = '$cookie_username'") or die(mysql_error());
$cookie_results = mysql_fetch_array($cookie_check);

	if ($cookie_password == $cookie_results['Password']) {
		echo "<div id=\"login_msg\">You are already logged on. Redirecting...</div><br />" && header("location:/index.php");
	}
}

if(isset($_POST['submit']))	{ 
$Username = mysql_real_escape_string(filter_input(INPUT_POST, 'Username', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$Password = sha1($_POST['Password']);

if (!$Username | !$Password) {
echo "<div id=\"error_msg\">You did not complete all of the required fields, please try again.</div><br />";
}

$sql = "SELECT * FROM Users WHERE (Username, Password) = ('$Username', '$Password')";
$db_check = mysql_num_rows($sql) or die(mysql_error());

if (mysql_query($db_check))	{
	$week = time() + 604800; 
	setcookie(ID_, $cookie_username, $week); 
	setcookie(Key_, $cookie_password, $week);
	echo "<div id=\"login_msg\">Successfully Logged In.</div><br />"  &&  header ("location:/index.php");
}

elseif (($Username | $Password) != $db_check) {
	echo "<div id=\"error_msg\">Invalid username or password, please try again.</div><br />";
}
}
?>

 

Logout.php

<?php
include("db.php");

if(isset($_COOKIE['ID_my_site'])) {
$cookie_username = mysql_real_escape_string(filter_input(INPUT_COOKIE, 'ID_', FILTER_SANITIZE_FULL_SPECIAL_CHARS));
$cookie_password = sha1($_COOKIE['Key_']);
$cookie_check = mysql_query("SELECT * FROM Users WHERE username = '$cookie_username'") or die(mysql_error());
$cookie_results = mysql_fetch_array($cookie_check);

	if ($cookie_password != $cookie_results['Password']) {
		header("location:/login.php");
	}
		else {
			$past = time() - 604800; 
			setcookie(ID_, gone, $past); 
			setcookie(Key_, gone, $past); 
			echo "<div id=\"error_msg\">Sucessfully logged out. Good Bye!</div><br />" && header ("location:/login.php");
		}
} 

?>

[Drummin]

Well right off the bat I see an echo and header in the same line.  For headers to work they must be done before anything is sent to the browser.  Rather than echo the line, set a variable, e.g. $message="whatever"; that you can then echo this down in your code below your <html> tags (which I don't see).  Really, processing should be done above <html> and forms messages etc, displayed within the <body> tags.

[Minimeallolla]

They are called when the form is submitted

 

<div id="regform"><form name="input" action="Registration.php" method="get">
    Username: <input type="text" name="Username" value="Username" onFocus="this.value=''"/><br />
    Password: <input type="password" name="Password"/><br />
    Email: <input type="text" name="Email" value="Enter Email"  onFocus="this.value=''"/><br /> 
    <input type="submit" name="Submit" value="Register" /><br />
</div></form>

 

you mean put:

<?php
if ($login_success == true) { print "$login_msg"; }
?>

in the html form if i want to display a message for a few seconds before they get redirected?

[TimeBomb]

Well, since you insist...

 

1) Create a function for filtering SQL data instead of copy pasting the code.

 

2) Or, better yet, use prepared statements with mysqli or PDO. See: mysqli prepared statements and PDO prepared statements. This will help further secure user input being sent to the database. Also, the PHP mysql library is not recommended for production use anymore.

 

3) Use the password() function in MySQL to further hash the password. You probably also want to salt the password as well.

 

4) Use sessions instead of cookies for storing the user's data. Though, for a 'remember me' function, you will need to use a cookie. Just make sure the data is heavily secured before it goes into the cookie.

[Minimeallolla]

(are we allowed bumps after a few days??)

[PFMaBiSmAd]

3) Use the password() function in MySQL to further hash the password. You probably also want to salt the password as well.

 

Do NOT use the mysql password function in your application, ever.

 

This is the function that is used for encrypting MySQL passwords for storage in the Password column of the user grant table.

 

...

 

The PASSWORD() function is used by the authentication system in MySQL Server; you should not use it in your own applications.

 

Also, by applying multiple hashes to a value, you increase the number of collisions (more different starting values produce the same ending value), because you end up hashing a fixed length string made up with a reduced set of characters (0-9,a-f only.)

 

 

[xyph]

Check out the article in my signature. It's a great read, let's you know the 'proper' way to hash a password (let a security expert do it ) and teaches you the basics about injection prevention, proper hashing and the principle of least privilege.

 

They use globals in the examples (bad boys) but it's more an example to learn from than something you should copy and paste