how can i improve this function?

[Ayon]

i have this line on the top of ever document to look for injection attempts...

<?php (safe_get()) ? exit() : false; ?>

 

but i'm sure there's some way to improve this to go even a little bit further

 

function safe_get()
{
if ($_GET) {
	foreach($_GET as $check) {
		$pattern = '/(DELETE|ALTER|SELECT|INSERT|UNION|CHAR|CONSTCHAR|INTO OUTFILE|DROP|TRUNCATE(.*))+/';
		if (!preg_match($pattern,$check)) { 
			$result = false;
		} else {
			$result = true;
			break;
		}
	}
	return $result;
}
}

 

Any suggestions are very much appreciated...

 

Thanks in advance

- Ayon

[Ayon]

anyone?

[ober]

So I assume you're not ever posting SQL statements, or if you are you are only doing it through POST?  You should also clean POST input, because someone can inject via POST just as easily as they can into GET.

[Ayon]

yeah i know it's just as easy to send injections trough $_POST as it is trough $_GET...

 

but the one i posted there is only for the $_GET, and no I never send any SQL statements trough the $_GET Is it secure enough or am I missing something?

 

as for the $_POST i only know about the htmlspecialchars() and mysql_real_escape_string()... is there anything else i should do besides those?

 

Thanks for the reply

[corbin]

Think about this:

 

SELECT * FROM users WHERE name = '{$name}';

 

 

Now, for $name to be dangerous, it would have to terminate the string encapsulation, yes?

 

The only way for that to happen is a ' character (over simplifying and lying, but not important here).

 

 

So, as long as ' is not present, unescaped, the variable is safe.

 

mysql_real_escape_string is the only thing needed to make a string safe (edit: for inserting into a MySQL database).  I don't know what this scanning for DELETE and what not non sense is.

 

 

 

 

But yes, if you're trying to detect, not prevent, then you should be fine.  You aren't doing anything when you do detect them though, so I don't see the point.

 

 

 

Also, that function would be easy to get around.  It would not get "delete" since only "DELETE" is matched.  (Perhaps use a i modifier?)

 

 

Also, the function does not recursively check the GET array.  What if there is a sub array element that is unsafe?

[ToonMariner]

I use a data cage class to sanitize any user submitted data...

 

However your situation is better solved using the pear DB package which does its magic to prevent sql with minimal effort.

 

it isn't efficient to run this on every page and on every variable passed to the page...

 

you will be far better off just using mysql_real_escape_string on the variables when you submit them to the query...

 

One point to note there is that utf-8 codes could be in the string and when those are submitted they are evaluated so they will allow an injection attack http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html

 

 

 

[corbin]

@ToonMariner:

 

Hrmmm, I thought only addslashes() was vulnerable to that.

 

Guess not ;p.

[Ayon]

Also, that function would be easy to get around.  It would not get "delete" since only "DELETE" is matched.  (Perhaps use a i modifier?)

 

Also, the function does not recursively check the GET array.  What if there is a sub array element that is unsafe?

 

Hmm, I thought only uppercase commands were used, but i guess i'm wrong then i modifier added.

 

But what you mean with sub array element? could you give a quick example?

 

I use a data cage class to sanitize any user submitted data...

 

However your situation is better solved using the pear DB package which does its magic to prevent sql with minimal effort.

 

it isn't efficient to run this on every page and on every variable passed to the page...

 

you will be far better off just using mysql_real_escape_string on the variables when you submit them to the query...

 

One point to note there is that utf-8 codes could be in the string and when those are submitted they are evaluated so they will allow an injection attack http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html

 

I guess you're talking about $_POST in your reply right? Atleast I can't see any $_GET relevant info might be me beeing stupid tho hehe

 

And thanks for the links.. Will read and see what I understand You might get a couple of questions tho hehe

 

 

 

@both of you..thanks for great replies! Very much appreciated!

[ToonMariner]

no not just post data but url encoded data too...

[Ayon]

could you explain the concept about a data cage?

 

and also another question... i have made a fully custom mysql class running baisicly everything trough my own object's and methods... now i just took a really quick look at mysqli... is that baisicly a mysql object only pre-made kinda like a framework?

 

probably a noobish question hehe

[corbin]

Also, that function would be easy to get around.  It would not get "delete" since only "DELETE" is matched.  (Perhaps use a i modifier?)

 

Also, the function does not recursively check the GET array.  What if there is a sub array element that is unsafe?

 

Hmm, I thought only uppercase commands were used, but i guess i'm wrong then i modifier added.

 

But what you mean with sub array element? could you give a quick example?

 

I use a data cage class to sanitize any user submitted data...

 

However your situation is better solved using the pear DB package which does its magic to prevent sql with minimal effort.

 

it isn't efficient to run this on every page and on every variable passed to the page...

 

you will be far better off just using mysql_real_escape_string on the variables when you submit them to the query...

 

One point to note there is that utf-8 codes could be in the string and when those are submitted they are evaluated so they will allow an injection attack http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html

 

I guess you're talking about $_POST in your reply right? Atleast I can't see any $_GET relevant info might be me beeing stupid tho hehe

 

And thanks for the links.. Will read and see what I understand You might get a couple of questions tho hehe

 

 

 

@both of you..thanks for great replies! Very much appreciated!

 

 

Traditionally and syntactically, only upper case commands are used, but every SQL dialect I've ever come across hasn't cared about case.

 

 

 

As the array thing:

 

somepage.php?something[]=DELETE FROM table;

[Ayon]

well i'm rewriting my mysql object now to work with prepared statements... haven't been doing this for to long. atleast not heavy stuff like this..

 

so my code is starting to get a little bit confusing now that i've passed 300 lines XD but i think i'm getting there very happy i'm solid on arrays