Page 2 of 2

Posted: Tue Feb 27, 2007 5:59 am
by jolinar
Mordred wrote:jolinar, your function is fine, but it is not enough. All data should be mysql_real_escape_string()-ed before putting it in a mysql_query(). If you have to stop and think if your check was enough, this means it's not enough.

Your function is about validation, which is a part of the code logic level. SQL Injection is an attack on the syntax level, and should be stopped by syntactical means. Proper escaping is one, writing SQL statements with proper syntax is another, and Kureigu did neither.

Btw, after $pword = md5($_POST['pword']); $pword is safe enough, on the code logic level it is fine. But remember what I told you - if you have to stop and think if it's enough, then it's not enough! Good coding practice dictates that you escape it as well.
Very good point, I've just started using that now. PHP 5 has a very nifty feature for this, the foreach iterator can be used to modify superglobals (probably not good practice)

Code: Select all

foreach($_GET as &$tmp) {
	$tmp = mysql_escape_string($tmp);
}
foreach($_POST as &$tmp) {
	$tmp = mysql_escape_string($tmp);
}
That, specifically, is the result of a conversation I had with a developer I know. My site uses a MySQL table to store some of the site's settings (in a similar vein to PHP Nuke, my coding style is horrible)

The problem suggested was that if I forgot to scan/escape the input then it would be vulnerable to a SQL injection attack. This approach scans EVERYTHING in $_GET and $_POST before the rest of the program even touches those variables.

Since this is a PHP5 specific feature, I've had to include a check at the start of the program that stops it and displays a version error if it detects anything<5 (In case I accidentally load it on to a machine running PHP4)

Posted: Tue Feb 27, 2007 6:16 am
by Mordred
jolinar, wash your hands with soap ;)

Don't ever do this kind of thing, it is plain wrong. The PHP team tried to do something similar (read on magic_quotes) and it was a disaster, security got no better, but writing code with magic_quotes on became a nuisance.

Escape for mysql ONLY if you're going to put the variable in mysql_query.

As for modifying arrays, PHP4 can do it like this:

Code: Select all

foreach ($array as $key=>$value)
   $array[$key] = $value . ' was changed';

Posted: Tue Feb 27, 2007 7:59 am
by jolinar
I stand corrected :(

Posted: Tue Feb 27, 2007 11:08 am
by Kureigu
Ok guys, I started over with the login page. There are some similarities, but little chance for SQL injection.

Oh, also. It works.
I also created a register script and a logout script.

I just have to now work out how to disallow certain username and password characters, and check that the email address is of a valid format.

Right now I'm working on a "create profile" section using text areas. I'll re-post when It's done.