Sound safe?

Craig321 · 22 Jan 2006 at 13:11

OK, thanks Rob and Beansprout, I thought Rob was saying that I didn't need that big bit of code at the top of my script. I'm going to put that in a different PHP file and include it so everything is neater

OK, I have added that. Does this look right?? Or do I still need the code Beansprout posted as they are different, don't think I do as they have the same type of stuff in them if you know what I mean.

Code:

function remove_magic_quotes($array) {
    foreach ($array as $k => $v) {
        if (is_array($v)) {
            $array[$k] = remove_magic_quotes($v);
        } else {
            $array[$k] = stripslashes($v);
        }
    }
    return $array;
}
if (get_magic_quotes_gpc()) {
    $_GET    = remove_magic_quotes($_GET);
    $_POST   = remove_magic_quotes($_POST);
    $_COOKIE = remove_magic_quotes($_COOKIE);
}

	//Include the config file
	include "config.php";
	
	$username = $_POST['username'];
	$password = $_POST['password'];
	$rpassword = $_POST['rpassword'];
	
	$pass = md5($password);
	
	if ($username === htmlspecialchars($username)) {
	
	if (($username == "") || ($password == "") || ($password !== $rpassword)) {
		echo "Please go back and make sure you have filled everything in correctly<br>";
		
	} else {
	
	$sql = sprintf("INSERT INTO users (username, password) VALUES ('%s', '%s')",
		   mysql_real_escape_string($username),
           mysql_real_escape_string($pass));

[Edit]
Was reading the SQL escape_string page again looking for something I read yesterday.
"This function must always (with few exceptions) be used to make data safe before sending a query to MySQL." Does this mean I only need to have this coding in queries that post data to SQL or should I have it in ALL queries?

[Edit]
Just read through the whole of Robs security bit on roblog, very good thanks Rob

[Edit]
Should I use Robs code or the code Beansprout posted? They are different but will they do the same thing?

Craig.

robmiller · 22 Jan 2006 at 21:55

They both do the same thing, really. I'd rewrite yours as:

Code:

<?php

//////-------------------> MOVE THIS TO CONFIG.PHP: VVVVV <-------------------\\\\\\
function remove_magic_quotes($array) {
	foreach ($array as $k => $v) {
		if (is_array($v)) {
			$array[$k] = remove_magic_quotes($v);
		} else {
			$array[$k] = stripslashes($v);
		}
	}
	return $array;
}
if (get_magic_quotes_gpc()) {
	$_GET    = remove_magic_quotes($_GET);
	$_POST   = remove_magic_quotes($_POST);
	$_COOKIE = remove_magic_quotes($_COOKIE);
}
//////------------------->  MOVE THIS TO CONFIG.PHP ^^^^^ <-------------------\\\\\\

//Include the config file
include "config.php";

$username = $_POST['username'];
$password = $_POST['password'];
$rpassword = $_POST['rpassword'];

$pass = md5($password);

if (empty($username) || empty($password) || empty($rpassword)) {
	echo 'Sorry, but you didn\'t fill out all of the required fields. You must enter a choice of username and type your chosen password twice.';
}

elseif($password != $rpassword) {
	echo 'The two passwords you entered do not match. Please type them carefully!';
}

elseif($count['count'] > 0) {
	echo 'Sorry, but the username you have chosen is already taken. Please choose another one.';
}

else {
	$sql = sprintf("INSERT INTO users (username, password) VALUES ('%s', '%s')",
	mysql_real_escape_string($username),
	$pass);
	
	mysql_query($sql);
}

?>

Move the Magic Quotes code to config.php, so you don't have to paste it at the top of every single page.
Make it a little easier on the user - don't just tell them they've made a mistake, explain what went went wrong and how to fix it.
Don't allow users to sign up with the same username as an existing user.
You don't need to use mysql_real_escape_string on the password, since it's an MD5 hash and thus cannot possibly ever contain any special characters.

Beansprout · 22 Jan 2006 at 22:01

Craig321 said:
OK, thanks Rob and Beansprout, I thought Rob was saying that I didn't need that big bit of code at the top of my script. I'm going to put that in a different PHP file and include it so everything is neater

Ah, sorry, cool - 'tis confusing at first

Just different methods - mine was throwing everything into a function and calling the function for each variable whereas rob's was a block of code to run at the top of the script to fix the hell that magic_quotes produces on every POST/GET/COOKIE variable, letting you apply the MySQL escaping where necessary

Craig321 · 24 Jan 2006 at 01:01

robmiller said:

They both do the same thing, really. I'd rewrite yours as:

Code:

<?php

//////-------------------> MOVE THIS TO CONFIG.PHP: VVVVV <-------------------\\\\\\
function remove_magic_quotes($array) {
	foreach ($array as $k => $v) {
		if (is_array($v)) {
			$array[$k] = remove_magic_quotes($v);
		} else {
			$array[$k] = stripslashes($v);
		}
	}
	return $array;
}
if (get_magic_quotes_gpc()) {
	$_GET    = remove_magic_quotes($_GET);
	$_POST   = remove_magic_quotes($_POST);
	$_COOKIE = remove_magic_quotes($_COOKIE);
}
//////------------------->  MOVE THIS TO CONFIG.PHP ^^^^^ <-------------------\\\\\\

//Include the config file
include "config.php";

$username = $_POST['username'];
$password = $_POST['password'];
$rpassword = $_POST['rpassword'];

$pass = md5($password);

if (empty($username) || empty($password) || empty($rpassword)) {
	echo 'Sorry, but you didn\'t fill out all of the required fields. You must enter a choice of username and type your chosen password twice.';
}

elseif($password != $rpassword) {
	echo 'The two passwords you entered do not match. Please type them carefully!';
}

elseif($count['count'] > 0) {
	echo 'Sorry, but the username you have chosen is already taken. Please choose another one.';
}

else {
	$sql = sprintf("INSERT INTO users (username, password) VALUES ('%s', '%s')",
	mysql_real_escape_string($username),
	$pass);
	
	mysql_query($sql);
}

?>

Move the Magic Quotes code to config.php, so you don't have to paste it at the top of every single page.
Make it a little easier on the user - don't just tell them they've made a mistake, explain what went went wrong and how to fix it.
Don't allow users to sign up with the same username as an existing user.
You don't need to use mysql_real_escape_string on the password, since it's an MD5 hash and thus cannot possibly ever contain any special characters.

OK, cool, thanks Rob

I'll re-write it in my own words if you know what I mean, I wanna keep it my own work still

[Edit]
Hmm, I have looked and can't find anything wrong. When I try signup with an already existing username it gives the error after if (mysql_query($sql)) { for some reason instead of the proper username already exists error.

Also, I don't know why I have never looked at what elseif does, I'll be using it from now on though, very handy.

Thanks
Craig