Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-26-2014, 08:05 AM,
#51
RE: DB_escape_string
Hi Jo, it seems to me that using prepared statements for a query such as "SELECT name FROM debtorsmaster WHERE debtorno='" . $DebtorNo . "'" in your AddCustomerContacts.php seems like overkill. My understanding is that prepared statements are good for repeated queries, but for a simple one off SELECT query such as this is the extra code complexity, and speed penalties worth it?

Thanks
Tim
Reply
03-26-2014, 12:51 PM,
#52
RE: DB_escape_string
Hi Tim,

(03-26-2014, 08:05 AM)Uhuru Wrote: Hi Jo, it seems to me that using prepared statements for a query such as "SELECT name FROM debtorsmaster WHERE debtorno='" . $DebtorNo . "'" in your AddCustomerContacts.php seems like overkill. My understanding is that prepared statements are good for repeated queries, but for a simple one off SELECT query such as this is the extra code complexity, and speed penalties worth it?
I went through the whole add contact script late last night in an attempt to use it as a demonstration in use of the bind vars and did not really make exceptions on where to use it or not.

But, yes, it is not necessary generally for SELECT to use prepared statements, when they are normally ideal for repeated queries.

However the key reason we are using them and bind variables for webERP is for security reasons. As such any data that comes in from a potentially untrusted source should be treated this way. If the $DebtorNo is not passed in by the user via form or $_GET var then perhaps it is unnecessary in this case.
For consistency sake I'd probably still use bind vars and perhaps at some stage just cater for them in DB_query. I don't believe the speed penalty would be significant, if any penalty at all for webERP in this case. The statement in the DB_query function is also immediately closed after passing the $result.

Keep in mind draft code I pushed still has some requirements (eg fix the audit code) and hopefully we can continue to improve on all aspects of what is there now.

Cheers,

Reply
03-26-2014, 01:40 PM, (This post was last modified: 03-26-2014, 01:42 PM by phil.)
#53
RE: DB_escape_string
Yes, I thought perhaps naively that we could simply modify DB_query( ......, $Parameters) to do a foreach loop around the $Parameters and DB_escape_string() them - then just pop them in to the query string in sequence? What more do we really need?
Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
03-26-2014, 01:57 PM, (This post was last modified: 03-26-2014, 02:45 PM by icedlava.)
#54
RE: DB_escape_string
Hi Phil,
(03-26-2014, 01:40 PM)phil Wrote: Yes, I thought perhaps naively that we could simply modify DB_query( ......, $Parameters) to do a foreach loop around the $Parameters and DB_escape_string() them - then just pop them in to the query string in sequence? What more do we really need?
Well, just my 2 cents, but I think we need to use generally accepted, standard and up to date methods of handling database transactions in relation to security. I believe that will also ensure webERP has credibility as well as a secure code base.

Even now my feeling is we are falling behind in trying to remain compatible with php versions less than 5.3. The longer we put off upgrading our minimum php and coding practices, the bigger job it will be later. I hope I'm not the only one thinking like this, but I defer to your judgement on how you want to handle the code.

Cheers,
Jo

(03-26-2014, 01:40 PM)phil Wrote: Yes, I thought perhaps naively that we could simply modify DB_query( ......, $Parameters) to do a foreach loop around the $Parameters and DB_escape_string() them - then just pop them in to the query string in sequence? What more do we really need?

Hmm did i misinterpret you Phil? Were you talking about the audit section that needs to be fixed? If so, then as serakfalcon pointed out earlier any attempt to replace ? with variables will fail if their is a ? in any of the said strings.

If you did mean the DB_query function generally, the 'escaping' of the variables in a loop is not necessarily sufficient - it will not necessarily protect against mysql injection for example.
Reply
03-26-2014, 05:30 PM,
#55
RE: DB_escape_string
(03-26-2014, 01:57 PM)icedlava Wrote: Well, just my 2 cents, but I think we need to use generally accepted, standard and up to date methods of handling database transactions in relation to security. I believe that will also ensure webERP has credibility as well as a secure code base.

Hi Jo, I follow the PHP and PHP-DEV mailing lists and recently there has been a lot of debate about the best way to go for this. Paramaterised queries undoubtedly have their proponents, but I am surprised you call it an accepted standard.

By opening and closing the prepared statement within the same function do we not lose the main benefit where there are repeated queries?

Tim
Reply
03-26-2014, 06:04 PM, (This post was last modified: 03-26-2014, 06:19 PM by icedlava.)
#56
RE: DB_escape_string
(03-26-2014, 05:30 PM)Uhuru Wrote: Hi Jo, I follow the PHP and PHP-DEV mailing lists and recently there has been a lot of debate about the best way to go for this. Paramaterised queries undoubtedly have their proponents, but I am surprised you call it an accepted standard.

By opening and closing the prepared statement within the same function do we not lose the main benefit where there are repeated queries?

Tim

Hi Tim,

Yes I follow the mailing lists too.

As I said previously, the main benefit we have and why this all started is for security of code, not for performance or other reasons that may be one of the many pros or cons of prepared statements. You will not get protection just using mysqli_db_escape_string. As you'd know, SQL injection works by including malicious string data when it creates SQL to send to the database. Bind vars work is because the sql string with the placeholders and the parameter array are sent to the mysql server separately and it is only there they are combined for execution.

Security and data integrity - that is why we have bindvars which use the prepared statements. Feel free to change it as you and phil see fit.

Would you like to go to PDO instead?

Cheers,
[Edited - added point about SQL injection]


Reply
03-26-2014, 06:20 PM,
#57
RE: DB_escape_string
Quote:I looked at doctrine DBAL some time ago(and propel) when working on another open source project and looking for an ORM. While it is good, it was not used. It just added another layer of code and syntax to learn, and was rather slow although the latest Doctrine is much faster. Do we really need this for webERP?
You can use it pretty much the same way as PDO, but with a broader range of supported databases and a consistent interface. In that case it doesn't really make things more complicated. The ORM is a separate layer that is complicated and not worth it for webERP but the DBAL is worth it I think.

(03-26-2014, 05:30 PM)Uhuru Wrote:
(03-26-2014, 01:57 PM)icedlava Wrote: Well, just my 2 cents, but I think we need to use generally accepted, standard and up to date methods of handling database transactions in relation to security. I believe that will also ensure webERP has credibility as well as a secure code base.

Hi Jo, I follow the PHP and PHP-DEV mailing lists and recently there has been a lot of debate about the best way to go for this. Paramaterised queries undoubtedly have their proponents, but I am surprised you call it an accepted standard.

By opening and closing the prepared statement within the same function do we not lose the main benefit where there are repeated queries?

Tim

If we wanted to take advantage of parametrized query re-use, or not use parameterized queries at all for simple queries (like select statements that require no variables) but still have better escaping of complicated variables then we'd need to have a completely different database interface from what we have now. One of the advantages of parametrized queries is that users could create new queries without worrying about injection, whereas now they have to remember about BOTH using DB_Escape_String() AND enclosing all variables within single quotes. There are a number of cases in existing code where variables are not enclosed in single quotes (because the data is set to a hidden control) which open up the door for SQL injections if the page was accessed via curl to custom-set the POST variables. I haven't tried to manipulate this but it might be possible to do things like alter the administrator's password in an update statement or return a list of all the users in the DB & their passwords as a UNION to an existing SELECT statement.

If we make a point to say that all variables must be escaped, AND enclosed in single quotes the need for parametrized queries disappears, so that could be added as a new coding standard, or just use parameters & the parametrized queries.


Reply
03-26-2014, 06:27 PM,
#58
RE: DB_escape_string
(03-26-2014, 06:20 PM)serakfalcon Wrote: If we make a point to say that all variables must be escaped, AND enclosed in single quotes the need for parametrized queries disappears, so that could be added as a new coding standard, or just use parameters & the parametrized queries.
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.


Reply
03-26-2014, 06:34 PM,
#59
RE: DB_escape_string
(03-26-2014, 06:20 PM)serakfalcon Wrote: If we make a point to say that all variables must be escaped, AND enclosed in single quotes the need for parametrized queries disappears, so that could be added as a new coding standard, or just use parameters & the parametrized queries.

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.

Thanks
Tim
Reply
03-26-2014, 06:40 PM,
#60
RE: DB_escape_string
(03-26-2014, 06:34 PM)Uhuru Wrote:
(03-26-2014, 06:20 PM)serakfalcon Wrote: If we make a point to say that all variables must be escaped, AND enclosed in single quotes the need for parametrized queries disappears, so that could be added as a new coding standard, or just use parameters & the parametrized queries.

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.

Thanks
Tim

Yeah I just checked the coding conventions page, it is there. I can send the results of the grep of the the SQL functions that don't follow this convention to the developers mailing list if that would be helpful.
Reply


Forum Jump:


Users browsing this thread: 2 Guest(s)