Are there any serious flaws in this?

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

User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

You could shorten the function to...

Code: Select all

<?php
function validate($string) {
    return preg_match("/^[\W]+[^,\.\'\"]*$/s", $string);
}
?>
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Aye, this one is shorter, but it's not any more correct than the first one. In fact both regexps happily allow quotes and other SQL syntactic characters, and if it weren't for the addslashes (wrong as it is), the script would be wide open for SQL injection.

dhrosti, are you actually testing those things you write? With both valid and invalid input?

Here, try

Code: Select all

'--
, it will pass both tests.

This is why "validation" shouldn't be thaught before "escaping"...
Tommy1402
Forum Newbie
Posts: 23
Joined: Tue Oct 03, 2006 4:33 am
Location: bandung
Contact:

Post by Tommy1402 »

Mordred wrote:Aye, this one is shorter, but it's not any more correct than the first one. In fact both regexps happily allow quotes and other SQL syntactic characters, and if it weren't for the addslashes (wrong as it is), the script would be wide open for SQL injection.

dhrosti, are you actually testing those things you write? With both valid and invalid input?

Here, try

Code: Select all

'--
, it will pass both tests.

This is why "validation" shouldn't be thaught before "escaping"...
How about this function?

Code: Select all

function noSlashQuote($value){
		$pattern = "/[^a-zA-Z0-9@_.-\s]/";
		$replacement = " ";
		return preg_replace($pattern, $replacement, $value);
	}
I use that function to filter all input(name, mail, phone) in my web form right before the date inserted into mysql.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

Tommy1402 wrote:How about this function?

Code: Select all

function noSlashQuote($value){
		$pattern = "/[^a-zA-Z0-9@_.-\s]/";
		$replacement = " ";
		return preg_replace($pattern, $replacement, $value);
	}
I use that function to filter all input(name, mail, phone) in my web form right before the date inserted into mysql.
It doesn't protect against all forms of SQL injection, if that's what you're asking. Where it notably fails to protect is injections that do not require quotes to taint your query.
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post by dhrosti »

I need to allow quotes though, which is why i have now used mysql_real_escape_string() before going anywhere near an sql query.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

As long as you write SQL code with quotes everywhere, and use mysql_real_escape_string() and casting to int for LIMIT values, you are safe from SQL injections. The preg_replace as a security measure is not required (it is not enough either, as it was already demonstrated). Now, as a part of your busyness logic you may want to disallow some characters in some fields, etc etc - THEN you may filter out some characters. Otherwise, from a security POV it's not needed.
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post by dhrosti »

Thanks, this post has been very helpful, cheers guys!
Post Reply