Menu
▾
▴
Re: [WebERP-developers] New branch working for DB_query parameterisation work
|
From: Jon R <jr...@gm...> - 2014-03-31 02:28:55
|
Sorry about the repeating there, was on my phone and it does weird things. I figured out a work around for the DB_PreparedQuery issues, I made a class that has most of the functions of mysqli_result, and DB_PreparedQuery passes that back. That way, the DB functions can handle it as if it was a mysqli_result object. I've updated the working branch accordingly. I also moved $Parameters to the second position, so you don't have to specify values for everything (since DB_Query is now different $Parameters doesn't need to be last anymore), and reduced reliance on passing $db around, although legacy code will still work. I recommend removing $db from code outside of the connection includes wherever possible. *Sent from my iLaptop* On Sun, Mar 30, 2014 at 11:41 AM, Jon R <jr...@gm...> wrote: > Hi Phil, the main issue is icedlava's code returns an array and all the > support functions expect a mysqli_result object. If you have: a) DB_query > returns an array also, and the support functions expect an array (hard to > implement for the fetch functions) b) we take the array result into account > and handle DB_Preparedquery differently from DB_Query (i don't recommend > this) or, Hi Phil, the main issue is icedlava's code returns an array and > all the support functions expect a mysqli_result object. If you have: a) > DB_query returns an array also, and the support functions expect an array > (hard to implement for the fetch functions) b) we take the array result > into account and handle DB_Preparedquery differently from DB_Query (i don't > recommend this) or, Hi Phil, the main issue is icedlava's code returns an > array and all the support functions expect a mysqli_result object. If you > have: a) DB_query returns an array also, and the support functions expect > an array (hard to implement for the fetch functions) b) we take the array > result into account and handle DB_Preparedquery differently from DB_Query > (i don't recommend this) or, Hi Phil, the main issue is icedlava's code > returns an array and all the support functions expect a mysqli_result > object. If you have: a) DB_query returns an array also, and the support > functions expect an array (hard to implement for the fetch functions) b) we > take the array result into account and handle DB_Preparedquery differently > from DB_Query (i don't recommend this) or, c) we return the mysqli_stmt > back, which has issues with binding the return values, and that DB_Query > returns a mysqli_result object, which handles differently from mysqli_stmt > On Mar 30, 2014 6:29 AM, "Phil Daintree" <ph...@lo...> wrote: > >> I had a go at removing the $Conn parameter and renaming the function >> >> DB_PreparedQuery() >> >> leaving the DB_query() function where it is - no reason why we can't >> continue to use it where there are no parameters involved I guess - as >> you suggested Jo... >> >> I messed with the style to try to bring the new code into line with the >> conventions we use and make the variable names a bit more meaningful - I >> hate abbreviated variable names that give you no clue as to what is >> going on... it is obvious when you write the code but 3 or 4 sleeps >> later not so obvious ... curly braces on the same line as the function >> call/if test etc etc. >> >> and of course predictably I have messed it up - login and >> AccounGroups.php no longer work :-( >> >> Hopefully though the concept is clear. >> >> Phil >> >> Phil Daintree >> Logic Works Ltd - +64 (0)275 567890 >> http://www.logicworks.co.nz >> >> On 28/03/14 20:42, icedlava wrote: >> > Phil, >> > >> > Can I commit TWIG to the new branch and a simple integration - existing >> > code will work as normal but it would provide somewhere to add twig >> > templates and themes for proof of concept or comparison? >> > >> > If it is not required, or wanted, it could be removed later. >> > >> > What do you think? >> > >> > >> > >> > On 28 Mar 2014, at 17:34, Phil Daintree wrote: >> > >> >> Jo, >> >> >> >> I can find it now - but have you written your ideal new function - >> >> named >> >> other than DB_query - and where the parameter type is given explicitly >> >> rather than inferred? Would be interested to see that... perhaps since >> >> we are doing this we should jump right on in? >> >> >> >> I am also rethinking the TableView class idea to eliminate the >> >> <tr><td></td></tr> all over the place - and perhaps maybe conversion >> >> to >> >> <div> with optional classes? that might more easily accommodate phones >> >> etc. We are of course looking it almost a complete re-write here! >> >> >> >> Let's go :-) >> >> >> >> Phil >> >> >> >> Phil Daintree >> >> Logic Works Ltd - +64 (0)275 567890 >> >> http://www.logicworks.co.nz >> >> >> >> On 28/03/14 19:07, icedlava wrote: >> >>> Hi Phil, >> >>> >> >>>> As you say, my revised DB_query() just escapes the parameters as >> >>>> they >> >>>> were previously - just that we have now moved the escaping to the >> >>>> place >> >>>> where it needs to happen rather than blanket escaping every >> >>>> $_POST/$_GET >> >>>> - so we are equally secure as we were ... unless I am wrong and >> >>>> those >> >>>> variables which should not be escaped are not escape >> >>> Yes, exactly - we are equally secure as we were before for database >> >>> transactions. >> >>> As you say, the whole reason that this topic came up was due to >> >>> non-contextual escaping of variables and the issues that has and is >> >>> continuing to cause in the code/database. >> >>> We have the opportunity now to fix it, but also improve the security >> >>> of >> >>> webERP as we fix the original variable problem, which is what I've >> >>> tried to do with the version I committed. >> >>> >> >>> Thanks for your understanding! >> >>> >> >>> Cheers, >> >>> >> >>> >> >>> On 28 Mar 2014, at 16:27, Phil Daintree wrote: >> >>> >> >>>> Hi Jo, >> >>>> >> >>>> As you say, my revised DB_query() just escapes the parameters as >> >>>> they >> >>>> were previously - just that we have now moved the escaping to the >> >>>> place >> >>>> where it needs to happen rather than blanket escaping every >> >>>> $_POST/$_GET >> >>>> - so we are equally secure as we were ... unless I am wrong and >> >>>> those >> >>>> variables which should not be escaped are not escaped. >> >>>> >> >>>> That said, I will look into the full prepare execute method in more >> >>>> detail. Of course with my blinkered vision what I did is more >> >>>> readable >> >>>> :-) I am persuaded by the argument to do things the most secure way >> >>>> though given we are into a major re-write and I think this may be >> >>>> the >> >>>> full prepare/execute method you first suggested. >> >>>> >> >>>> Phil >> >>>> >> >>>> Phil Daintree >> >>>> Logic Works Ltd - +64 (0)275 567890 >> >>>> http://www.logicworks.co.nz >> >>>> >> >>>> On 28/03/14 16:45, icedlava wrote: >> >>>>> Hi Phil, >> >>>>> >> >>>>> For reference here on the list, I have committed the code as >> >>>>> requested. >> >>>>> You have updated the original function I committed which changes >> >>>>> and >> >>>>> I >> >>>>> did make a point to you in email that these changes will not work. >> >>>>> >> >>>>> As per your request I've forwarded your response to the mailing >> >>>>> list >> >>>>> but >> >>>>> posting here for completeness in this thread. >> >>>>> >> >>>>> Note that the latest changes will not work because it is not the >> >>>>> fact >> >>>>> that the query is prepared, it is the fact the query is >> >>>>> paramaterized >> >>>>> that makes it secure. >> >>>>> Parameterized queries, done in the way I did it work because the >> >>>>> query >> >>>>> string and the variables are sent separately to the database server >> >>>>> and >> >>>>> only there are put together. >> >>>>> >> >>>>> The changes you made are no different to the original DB_query that >> >>>>> mysql_db_escapes the data - except there is more code and separated >> >>>>> parameters. The key issue: >> >>>>> >> >>>>> In your new code: >> >>>>> $result=mysqli_query($Conn, $SQLString); is the problem. No >> >>>>> different >> >>>>> to >> >>>>> prior old DB_query. >> >>>>> >> >>>>> The query is already 'put together' with query and variables before >> >>>>> it >> >>>>> goes to the server. That is the insecure nature of it and why I >> >>>>> changed >> >>>>> it. >> >>>>> >> >>>>> Appreciate your work and thought on it so far. >> >>>>> >> >>>>> Cheers, >> >>>>> >> >>>>> >> >>>>> >> >>>>> >> >>>>> On 25 Mar 2014, at 17:10, Phil Daintree wrote: >> >>>>> >> >>>>>> I've made up a new branch so we can mess with this a bit >> >>>>>> >> >>>>>> svn checkout --username=XXXXX >> >>>>>> svn+ssh://XX...@sv.../p/web-erp/code/branches/working >> >>>>>> yourworkingcopy >> >>>>>> >> >>>>>> where XXXXX is your sourceforge login. >> >>>>>> >> >>>>>> Jo, if you commit the code you want for the new DB_query - and I >> >>>>>> propose >> >>>>>> we should keep it the same name - but with a new optional >> >>>>>> parameter >> >>>>>> for >> >>>>>> the array of parameters. >> >>>>>> >> >>>>>> >> >>>>>> -- >> >>>>>> Phil >> >>>>>> >> >>>>>> Phil Daintree >> >>>>>> Logic Works Ltd - +64 (0)275 567890 >> >>>>>> http://www.logicworks.co.nz >> >>>>>> >> >>>>>> >> >>>>>> >> ------------------------------------------------------------------------------ >> >>>>>> Learn Graph Databases - Download FREE O'Reilly Book >> >>>>>> "Graph Databases" is the definitive new guide to graph databases >> >>>>>> and >> >>>>>> their >> >>>>>> applications. Written by three acclaimed leaders in the field, >> >>>>>> this first edition is now available. Download your free book >> >>>>>> today! >> >>>>>> http://p.sf.net/sfu/13534_NeoTech >> >>>>>> _______________________________________________ >> >>>>>> Web-erp-developers mailing list >> >>>>>> Web...@li... >> >>>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> >>>>> >> ------------------------------------------------------------------------------ >> >>>>> _______________________________________________ >> >>>>> Web-erp-developers mailing list >> >>>>> Web...@li... >> >>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> >>>>> >> >>>> >> ------------------------------------------------------------------------------ >> >>>> _______________________________________________ >> >>>> Web-erp-developers mailing list >> >>>> Web...@li... >> >>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> >>> >> ------------------------------------------------------------------------------ >> >>> _______________________________________________ >> >>> Web-erp-developers mailing list >> >>> Web...@li... >> >>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> >>> >> >> >> >> >> ------------------------------------------------------------------------------ >> >> _______________________________________________ >> >> Web-erp-developers mailing list >> >> Web...@li... >> >> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> > >> ------------------------------------------------------------------------------ >> > _______________________________________________ >> > Web-erp-developers mailing list >> > Web...@li... >> > https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> > >> >> >> >> ------------------------------------------------------------------------------ >> _______________________________________________ >> Web-erp-developers mailing list >> Web...@li... >> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> > |
