Proposal to secure $xoopsDB->query method

Remember a discussion about ->query method?

http://community.impresscms.org/modules/newbb/viewtopic.php?post_id=13353#forumpost13353

This was my proposal.

now this is the code that tries to execute this proposal

This is a test function, not related to iCMS, just wanted to try my method to detect SQL injections. This is the code:




if it works, we should change two or three lines to include it in iCMS.

nachenko - thank you

To get this to do anything, I had to escape most of the quotes.

I'm not sure what the section to remove the separators accomplishes, nor do I think you want to remove all of them, if that is the intent. They are valid in text areas - don't you think? They also are important to MySQL in properly casting the parameters. MySQL can and does convert them, but it takes additional processing time.

you should also add UN-ION to the forbidden array

Yes, union should not be allowed to be added to a query string, unless it is a valid portion of a field, like it is here.The same goes for all the forbidden words, not all occurrences of them are going to be malicious.

I suggest we look at GIJOE's Protector for his patterns, the PHP page on SQL injection and this class for safeSQL

would it not be ok to seperate db->query for types of query.

for example

add a flag to function dbquery & validate_query etc?

function validate_query($q, type='select')

so now if the type field is set, then the only allowed function in the query will be select

type='drop'

only a drop query allowed.

type='all' all methods

type='update' only update allowed

type='custom' a custom selection (can be defined in the module or core, allowing a specific complex query to be constructed).

I think we're going out of scope.

We all know we have to make DB queries safer for future developments.

The problem is WHAT THE HELL we do to MAKE OLD MODULES SAFER. How can we secure all these unsafe Xoops modules that are not being updated to use our improvements in security.

xoopsDB->query method syntax must stay unchanged, as all modules use it. So how can we fight against malitious queries given the fact that there are so many Xoops modules out there that are not going to be updated to be more secure?

This is what my code snippet is about. But this code snippet is just an idea expressed in code. What can we do to improve it?

db->query takes 3 parameters - a string that will be parsed as an SQL statement, a limit for the number of results to return and a starting point.

By the time the query method is called, all validation should have already happened, but we can't count on this, I understand that. But, arbitrarily forbidding delete, insert, update, drop, union, alter, or any other mysql reserved word or function would keep this post from being added here. In other injection attacks, the words 'and' and 'or' are also used to corrupt the query.

At this point, we also don't know much about the context of the query - what parameters were from user input and which were from the developer? What datatype should they be? What length should the parameters be? I've been going through all the possible ways to be sure the query string is valid using regex, stored procedures, prepare statements, but I keep coming up with exceptions to the rules, unless you know the context.

What we might be able to determine is if the string has been properly escaped, quoted and encoded. Maybe we look at it from this angle.

if the inputs were properly sanitized and the queries properly escaped, i don't think we would need to worry about the above too much.

what we need to find out more is why module developers create their own classes instead of using the cores class for sanitizing. if the core sanitizer is not upto the job then it needs to made to do it's job, so that module developers have no need to write their own classes in the 1st place, modules should always be using the core classes and functions for sanitizing and the likes, but in many cases they don't.

is it because module developers don't know about existing core functions/classes or is it because the existing classes etc are not up to scratch.

we should try to make it harder for query's to be executed if $variables etc have not been passed through the sanitizer or escaped properly using specific functions.

is this query clean? yes, then continue executing the query. no, then end query and return to start.

really we can only truly be safer when we go the full OO route, so that all queries, even from modules are all constructed from 1 place by means of the module passing the correct variables to the object class to construct the query, instead of the module doing the query construction itself.

then there'd be no need for a module to have such lines as

$result = $db->query(SELECT * FROM table WHERE id = 1);

in stead we'd pass * to the core along with the table name & other parameters such as id = 1.

and then the core would clean all the variables before it constructs the query itself and then executes it, then the output would then be returned to the module.

so essentially only the core is ever running queries on the DB and not the modules themselves.

In my case, it is because I don't fully understand how the built in sanitisers work - I haven't found any solid explanation anywhere, so I just feel more confident doing it myself.

Hi Vaughan, Nachenko, Skenow and all.


Unfortunately I think that it is impossible for you to read the original document. But the proposal should encapsular all recordings and recoveries in XoopsDB.

Just to remind again Here

[pt-br]
Infelizmente eu penso que é impossível para você ler este documento inicial. Mas a proposta deverá encapsular todas as gravações e recuperações no XoopsDB.

Apenas para relembrar novamente Aqui
[/pt-br]