webERP Forum

Full Version: DB_escape_string
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
Pages: 1 2 3 4 5 6 7 8 9
(03-26-2014, 06:34 PM)Uhuru Wrote: [ -> ]This is already part of the coding standards (or certainly used to be). I know there are areas where variables are not enclosed in single quotes, these need to be corrected.

We are forgetting where this started.

We have in session.inc code that applies DB_escape_string to all session, post and get vars. This is wrong.

So, once we take this out, we need to adjust code elsewhere to compensate.

Going back to the original posts - this must be done contextually.

1. Datatabase transactions
2. Output data - to HTML, PDF, ajax or whatever

We are now supposed to be handling database transactions and thus we are trying to make it a little easier by using the DB_query function. We know we cannot just use mysqli_db_escape_string on the data - not good enough to prevent SQL injections for a start. We can use bind vars (not parameterized sql per se but it's the bind vars), PDO prepared statements, or we can escape and validate and quote, and ensure the character set is correct, for every set of data going into a database transaction - and to be absolutely correct we should adjust for string or integer for example.

We also have to yet find a consistent way of addressing point 2 - or ensure every person validates the data and then prepares it correctly prior to output otherwise we leave ourselves open to security issues and data integrity issues.

Just a quick summary so we know where we are.

Please correct if i've left anything out.

Thanks,
(03-26-2014, 06:27 PM)icedlava Wrote: [ -> ]Yes, agree except it is not parameterized queries per se that helps us - you could have parameterized queries without bind vars - but it's the bind vars we need to fix the issue.

We could as you say, escape all data, and be aware of data type when doing so and thereby get rid of the need for bind vars. This is a lot harder to do consistently taking into account data type, than it is using bind vars, and could leave us open to security incursions.

Hi Jo, It has always been a part of the philosophy of the project that as far as possible the code should easily define what is happening. It may be over familiarisation on my part but I find that something like:

$SQL = "SELECT foo from bar WHERE name ='Tim'";
$Result = DB_Query($SQL, $db); //Actually the $db is unnecessary as it is a global
while ($MyRow = DB_fetch_array($Result)) {
echo $MyRow['foo'];
}

nicely describes what is happening. I found the new code you did less obvious, but as I say it may be just over familiarisation.

This is why I have been urging Phil to look again at the code that serakfalkon has done for table views. IMHO that code better describes a table of data than the current mass of <td><th><th> etc tags. I think it makes the code more readable, not less.

Thanks
Tim
(03-26-2014, 06:42 PM)Uhuru Wrote: [ -> ]It may be over familiarisation on my part but I find that something like:

$SQL = "SELECT foo from bar WHERE name ='Tim'";
$Result = DB_Query($SQL, $db); //Actually the $db is unnecessary as it is a global
while ($MyRow = DB_fetch_array($Result)) {
echo $MyRow['foo'];
}

nicely describes what is happening. I found the new code you did less obvious, but as I say it may be just over familiarisation.
Tim I totally agree with you. Bind vars was never a hit for clarity compared to what the simple sql statement is. However what you have posted is insecure depending on what happens in DB_Query. Please see my last summary post if you will - i think it came in at about the same time you posted.

Cheers,


There are lots of references about on SQL injection and best practice. Perhaps one authoratative site has a specific section on SQL injection (and note this is just one type of security vulnerability ):

https://www.owasp.org/index.php/SQL_Inje...heat_Sheet

In particular I quote:
Quote:This third technique is to escape user input before putting it in a query. If you are concerned that rewriting your dynamic queries as prepared statements or stored procedures might break your application or adversely affect performance, then this might be the best approach for you. However, this methodology is frail compared to using parameterized queries and we cannot guarantee it will prevent all SQL Injection in all situations. This technique should only be used, with caution, to retrofit legacy code in a cost effective way. Applications built from scratch, or applications requiring low risk tolerance should be built or re-written using parameterized queries.
(03-26-2014, 06:46 PM)icedlava Wrote: [ -> ]
(03-26-2014, 06:42 PM)Uhuru Wrote: [ -> ]It may be over familiarisation on my part but I find that something like:

$SQL = "SELECT foo from bar WHERE name ='Tim'";
$Result = DB_Query($SQL, $db); //Actually the $db is unnecessary as it is a global
while ($MyRow = DB_fetch_array($Result)) {
echo $MyRow['foo'];
}

nicely describes what is happening. I found the new code you did less obvious, but as I say it may be just over familiarisation.
Tim I totally agree with you. Bind vars was never a hit for clarity compared to what the simple sql statement is. However what you have posted is insecure depending on what happens in DB_Query. Please see my last summary post if you will - i think it came in at about the same time you posted.
If the SQL string is constant like what Tim posted you don't need the overhead of a parametrized query, which is the point Tim was making I think.
If Tim is a variable and it is possible for the user to change the charset of the DB, it is possible but very difficult to write an injection.

The code
Code:
"...some query here '" . escapefunction($variable) . "' ...
should be safe no matter what, if the charset cannot be changed. note the single quotes. (escape functions generally won't escape if it thinks the input is a number, allowing for a certain class of injections, but SQL will still parse a number from a string as a number, so if you deliberately force all input to be strings it should be safe.) Yes, it says that on the page that you provided but the truth is more complicated, hence the current debate, even within the PHP dev lists themselves
(03-26-2014, 07:38 PM)serakfalcon Wrote: [ -> ]If the SQL string is constant like what Tim posted you don't need the overhead of a parametrized query, which is the point Tim was making I think.
If Tim is a variable and it is possible for the user to change the charset of the DB, it is possible but very difficult to write an injection.

Correct, I'd missed the 'constant' and was thinking of variables. However variables is what we have been talking of, and SQL injection protection, just one security issue we are trying to guard against, is possible with any variable although more so in some situations than others.

What it boils down to, is we could debate the issue for weeks as it happens in the lists.

We could look at other opensource projects etc drupal, wordpress, joomla, codeigniter that use one or other of the following (or a few):

1. PDO
2. Parameterized input in queries - using bound variables
3. Parameterized input in queries - using escaped variables where the escape function is context aware at least for string and integer

I've committed the code that contained bind vars as per request on this thread.

I used an adjustment to the existing DB_query as requested and as such needs to make provision for both existing parametrized and non-parametrized query input types (Personally not keen on that - prefer to use a totally new one with neater signature and code, and have them concurrent so the code still works until they are all updated).

If we want to use an escape function instead of bind vars, then that is fine by me as long as all things are considered, but everything seemed to have been eliminated except the bind var method which I was requested to commit.

We had I thought already also eliminated PDO - or is that also still in the running too?

The changes recently committed to the DB_query function replacing the one I originally put in the new branch, will not work to circumvent bad data and security issues (but I might have overlooked something).

Cheers,





PS. I am happy to help with the code refactor, but I don't have time to debate the pros and cons of every method as this is found in many threads and documents all over the web already debated vigourously - people have their favourites. I think one of the standard methods (list them such as the 3 in my last post) that are well documented, that have well documented code examples and that best fit with webERP objectives or can easily be modified for it, should be chosen- then go for it. No need to reinvent the wheel.

Cheers,
Guess I need to do some reading ... was hoping you guys could put me right...
I thought we could just use mysqli_real_escape_string() on all variables - hadn't realised that the content of the variable was a factor with the code I just committed.
Hi Phil,

There is a lot of reading on the topic but generally there are one or more of three methods used in the better known opensource projects. I listed these in my last post.

We have been just looking at SQL injection mostly in this thread and only at database data. We still need to address data output to HTML, ajax, PDF, etc.

Here is an interesing link cited by a few security related sites that reinforces on how important it is to have consistent tight security across the codebase.

http://stackoverflow.com/questions/13409...-injection

Cheers,
I think, in sum, the new query function is built to resemble the old query function, with the added benefit that, so long as users are instructed to only use bindvars for variables, the queries will be 99.9% injection safe.

However, the function does not take advantage of the fact that parametrized queries are more efficient when executed multiple times. A possible solution would be to pass the statement back and force the user to close it themselves. This is more to remember for coders but allows for parametrized queries to do what they are originally were intended for: executing the SQL statement with different variables multiple times. If we do that we'd also need a function to alter the bound parameters (since bind_param is mysqli specific) as well.

Also, for constant SQL queries, using parametrized queries comes at a slight performance hit. So, there is a case to be made for keeping a straight-up database query function for those cases, as long as it is very clear that it is only to be used when no variables are needed whatsoever.
(03-27-2014, 01:11 PM)serakfalcon Wrote: [ -> ]I think, in sum, the new query function is built to resemble the old query function, with the added benefit that, so long as users are instructed to only use bindvars for variables, the queries will be 99.9% injection safe.
That was the query, but it has been changed since in the subversion branch.
Quote:the function does not take advantage of the fact that parametrized queries are more efficient when executed multiple times.
Yes I thought of that - and perhaps we could get around it by passing in an option say, 'raw' for want of a better name, that is false.

This option would allow the statement to be passed back rather than the array as i had it.

This would therefore be only available as required, and hopefully to those that remember to close it.

Quote: If we do that we'd also need a function to alter the bound parameters (since bind_param is mysqli specific) as well.
Continuing on from the 'raw' idea - that would be handled and called by the developer's script after they receive the statement back.
Quote:Also, for constant SQL queries, using parametrized queries comes at a slight performance hit.
I don't believe it would be that great given the scope and purpose of most webERP scripts and think consistency would be better. Refactoring code somewhat for higher php versions later would I believe overcome it perhaps. But that's just my thought atm.


Cheers,
[Edit] - I would hope if consistency was chosen then traces of the 'old' DB_query would be removed at some point after all sql had been revised for new bind vars.
(03-27-2014, 01:39 PM)icedlava Wrote: [ -> ]perhaps we could get around it by passing in an option say, 'raw' for want of a better name, that is false.

This option would allow the statement to be passed back rather than the array as i had it.

This would therefore be only available as required, and hopefully to those that remember to close it.
Is it a good idea to have a boolean option that drastically alters the output of the function? I'd almost prefer to use a different function name in that case.


(03-27-2014, 01:39 PM)icedlava Wrote: [ -> ]Continuing on from the 'raw' idea - that would be handled and called by the developer's script after they receive the statement back.

Of course it has to be handled to a certain extent by the developer,
however you still need the wrapper function if you pass the statement back at all, mysqli statments use ->bind_param() to bind all values, PDO uses bindParam() for a single value, and I'm sure other database extensions use different syntax. If you don't have a wrapper the code won't work across database extensions. Of course, we could wrap all of this up in a class and just use that, which is exactly what Doctrine DBAL does.


Pages: 1 2 3 4 5 6 7 8 9