Thread: [Phpsurveyor-developers] BUG Escaping Forms input in database.php
The leading Open Source survey tool
Brought to you by:
c_schmitz
From: Thibault Le M. <Thi...@su...> - 2007-01-24 17:51:30
|
Hi, When I tried to insert fields with special chars such as simple quotes, = I got an SQL error. I tracked down this issue in the database.php file. No adslashes is done in the INSERT queries which use directly $_POST. So, in theory, if I want input-escaping I need to turn on = 'magic_quotes_gpc' in php.ini.=20 But this fails since the follofing lines can be found at the beginning = of the database.php file: if (get_magic_quotes_gpc()) $_POST =3D array_map('stripslashes', $_POST); Of course commenting the above 2 lines solves the issue (with magic_quotes_gpc enabled), but then we would need a stripslashes = somewhere to remove \. What do you propose ? +------------------------------------------------------------------------= + | Thibault LE MEUR | http://www.supelec.fr = | | Sup=E9lec | e-mail: = Thi...@su... | | Centre de Ressources Informatiques| tel: +33 [0]1 69 85 17 89 = | | Plateau de Moulon | = | | 3 rue Joliot-Curie | fax: +33 [0]1 69 85 12 34 = | | 91192 Gif-sur-Yvette CEDEX, France| Supelec: +33 [0]1 69 85 12 12 = | +------------------------------------------------------------------------= +=20 |
From: Carsten S. <car...@gm...> - 2007-01-24 17:59:30
|
Hi! We should find an magic_quotes_gpc independent solution. The only way can be to properly escape the simple quote before inserting into mysql. Maybe adodb provides a function to do this properly - i remember to have seen something like that - please look into the adodb docs since the solution should be db independent too. Best regards Carsten Thibault Le Meur wrote: > Hi, > > When I tried to insert fields with special chars such as simple quotes, I > got an SQL error. > > I tracked down this issue in the database.php file. > > No adslashes is done in the INSERT queries which use directly $_POST. > > So, in theory, if I want input-escaping I need to turn on 'magic_quotes_gpc' > in php.ini. > But this fails since the follofing lines can be found at the beginning of > the database.php file: > if (get_magic_quotes_gpc()) > $_POST = array_map('stripslashes', $_POST); > > Of course commenting the above 2 lines solves the issue (with > magic_quotes_gpc enabled), but then we would need a stripslashes somewhere > to remove \. > > What do you propose ? > > +------------------------------------------------------------------------+ > | Thibault LE MEUR | http://www.supelec.fr | > | Supélec | e-mail: Thi...@su... | > | Centre de Ressources Informatiques| tel: +33 [0]1 69 85 17 89 | > | Plateau de Moulon | | > | 3 rue Joliot-Curie | fax: +33 [0]1 69 85 12 34 | > | 91192 Gif-sur-Yvette CEDEX, France| Supelec: +33 [0]1 69 85 12 12 | > +------------------------------------------------------------------------+ > > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys - and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > PHPSurveyor-Developers mailing list > PHP...@li... > https://lists.sourceforge.net/lists/listinfo/phpsurveyor-developers > > |
From: Thibault Le M. <Thi...@su...> - 2007-01-25 11:07:45
|
> -----Message d'origine----- > De : php...@li...=20 > [mailto:php...@li...]=20 > De la part de Carsten Schmitz > Envoy=E9 : mercredi 24 janvier 2007 19:01 > =C0 : php...@li... > Objet : Re: [Phpsurveyor-developers] BUG Escaping Forms input=20 > indatabase.php >=20 >=20 > Hi! >=20 > We should find an magic_quotes_gpc independent solution.=20 > The only way can be to properly escape the simple quote=20 > before inserting into mysql. Sure. > Maybe adodb provides a function=20 > to do this properly - i remember to have seen something like=20 > that - please look into the adodb docs since the solution=20 > should be db independent too. Yes there is the $DB->qstr function, but this requires rewriting the DB calls in all phpsurveyor code ! Is it something to be done for next alpha release ? I my opinion YES, since, as is, phpsurveyor is not usable (simple quotes = are too much used in current languages ). Thibault |
From: Carsten S. <car...@gm...> - 2007-01-25 13:33:31
|
Hi! Well.. then we should start right now with it... Can you post a small code snippet example old/new? We can share the work if you tell me what files you do and what files you want me to do. Best regards Carsten -------- Original-Nachricht -------- Datum: Thu, 25 Jan 2007 12:07:33 +0100 Von: "Thibault Le Meur" <Thi...@su...> An: php...@li... Betreff: [Phpsurveyor-developers] RE : BUG Escaping Forms input indatabase.php > > > > -----Message d'origine----- > > De : php...@li... > > [mailto:php...@li...] > > De la part de Carsten Schmitz > > Envoyé : mercredi 24 janvier 2007 19:01 > > À : php...@li... > > Objet : Re: [Phpsurveyor-developers] BUG Escaping Forms input > > indatabase.php > > > > > > Hi! > > > > We should find an magic_quotes_gpc independent solution. > > The only way can be to properly escape the simple quote > > before inserting into mysql. > > Sure. > > > Maybe adodb provides a function > > to do this properly - i remember to have seen something like > > that - please look into the adodb docs since the solution > > should be db independent too. > > Yes there is the $DB->qstr function, but this requires rewriting the DB > calls in all phpsurveyor code ! > > Is it something to be done for next alpha release ? > I my opinion YES, since, as is, phpsurveyor is not usable (simple quotes > are > too much used in current languages ). > > Thibault > > > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share > your > opinions on IT & business topics through brief surveys - and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > PHPSurveyor-Developers mailing list > PHP...@li... > https://lists.sourceforge.net/lists/listinfo/phpsurveyor-developers -- "Feel free" - 10 GB Mailbox, 100 FreeSMS/Monat ... Jetzt GMX TopMail testen: http://www.gmx.net/de/go/topmail |
From: Thibault Le M. <Thi...@su...> - 2007-01-25 16:20:33
|
Hi Carsten, >=20 > Well.. then we should start right now with it... Can you post=20 > a small code snippet example old/new? We can share the work=20 > if you tell me what files you do and what files you want me to do.=20 Good news: I had a deeper look at the code and it seems that Adodb = quoting functions are already used in the code (but some calls are missing). Here is my analysis: In database.php: * At the beginning of the file: ----- if (get_magic_quotes_gpc()) $_POST =3D array_map('stripslashes', $_POST); ----- This reverse any quoting automatically done by magic_quotes_gpc if it is enabled in php.ini. Then the following function is defined: ----- function db_quote($str) { global $connect; return $connect->escape($str); } ----- Note that since we have undone any magic_quote_gpc first, we do not need = to give the second boolean argument to the escape method, the value of the magic_quote_gpc, which is assumed to be false. Then, before 'some' INSERT queries (the some word means there are some missing calls), the db_quote is applied: For instance in the '$action =3D=3D "insertnewgroup"' section: ----- $_POST =3D array_map('db_quote', $_POST); ----- That is why there is no issue with survey name, group names and descr, question names. The issue seems to be that there are some missing db_quote calls before = some INSERTs (for instance for answers). So we just have to track INSERTS and UDPATE queries and see if the posts have been previously quoted. By the way I had a look at common.php and there is a = 'mysql_escape_string' remaining. I think (but I'm not sure) that it should be replaced by a = adodb quoting function such as $connect->escape. What do you think ? Am I right or wrong in this analysis ? Regards, Thibault |
From: Thibault Le M. <Thi...@su...> - 2007-01-25 16:49:51
Attachments:
dbquote1.diff
|
> So we just have to track INSERTS and UDPATE queries and see=20 > if the posts have been previously quoted. The attached patch is my first review of the code and it prove to be = working for quoting attributes and answers. Can you have a quick look at the code and see if other db_quote calls = are missing ? The code isn't very readable and I could have missed some = calls. Do you want me to submit the patch to SVN ? > By the way I had a look at common.php and there is a=20 > 'mysql_escape_string' remaining. I think (but I'm not sure)=20 > that it should be replaced by a adodb quoting function such=20 > as $connect->escape. >=20 > What do you think ? Am I right or wrong in this analysis ? I'm still not sure what to do with this ? Thibault |
From: Carsten S. <car...@gm...> - 2007-01-25 17:43:29
|
Hi Thibault, Thibault Le Meur wrote: >> So we just have to track INSERTS and UDPATE queries and see >> if the posts have been previously quoted. >> > > The attached patch is my first review of the code and it prove to be working > for quoting attributes and answers. > > Can you have a quick look at the code and see if other db_quote calls are > missing ? The code isn't very readable and I could have missed some calls. > > Do you want me to submit the patch to SVN ? > Looks good to me.. go right ahead. >> By the way I had a look at common.php and there is a >> 'mysql_escape_string' remaining. I think (but I'm not sure) >> that it should be replaced by a adodb quoting function such >> as $connect->escape. >> >> What do you think ? Am I right or wrong in this analysis ? >> > > I'm still not sure what to do with this ? > Please replace it. It's a leftover from the old mysql calls. Good work! Carsten |
From: Thibault Le M. <Thi...@su...> - 2007-01-25 17:58:07
|
> > Do you want me to submit the patch to SVN ? > > > Looks good to me.. go right ahead. Done > Please replace it. It's a leftover from the old mysql calls. > Good work! Applied, but not tested. I have no clue on how to test gestextendedanswers with type R ? Thibault |
From: Carsten S. <car...@gm...> - 2007-01-25 18:48:25
|
Thibault Le Meur wrote: > Done > > Great! >> Please replace it. It's a leftover from the old mysql calls. >> Good work! >> > > Applied, but not tested. I have no clue on how to test gestextendedanswers > with type R ? > Create a survey with a ranking question, activate it , make sure in the survey settings the results are sent to the admin by mail. Fill out the survey and check the mail. Regards Carsten |
From: Tom T. <tom...@he...> - 2007-02-01 02:27:00
|
Hi everyone Apologies for the lenthy post. I'm still getting my head around some things. The problems raised with using magic quotes are part of the problem I've been having porting to SQL Server, as the backticks just don't execute against that DB. It looks like this is being resolved, but there are a couple of other issues to think about. The first one is the "select * from table limit n". Again SQL Server chokes, as it expects the form "select top n * from table". The ADODB lib has a neat method for building up the correct limit sql like this: $connection->SelectLimit('select * from table', nn); I haven't used it yet, but if it works as described it would remove one big headache. I'm annoyed with myself because I wrote my own method to do the same thing - I've been away from PHP a while, so please forgive me if I may quite a few obvious blunders. Am I going down the right path here? There's a fair bit of sql that can't be ported so easily unfortunately, for instance calls to DECODE have no support in ADODB (and no real equivalent in T-SQL that I'm aware of). There's also a huge problems with how different data types are handled in the different DBs (e.g. can't ORDER BY an ntext field). The best advice on this I've seen is to treat database portability as a localization issue, and place the SQL in a separate file for each DB type. This doesn't seem like a bad idea, but I'd be keen to know what other people think. My first cut of porting to SQL Server works, but it's pig ugly - lots of case statements, testing the connection type over and over. I'd rather do it right. Cheers Tom |
From: Carsten S. <car...@gm...> - 2007-02-01 18:40:51
|
Hello Tom Tom Tuddenham wrote: > Hi everyone > > Apologies for the lenthy post. I'm still getting my head around some > things. > Nothing to apology here.... Communication is essential in a software development project. > The problems raised with using magic quotes are part of the problem I've > been having porting to SQL Server, as the backticks just don't execute > against that DB. It looks like this is being resolved, but there are a > couple of other issues to think about. > > The first one is the "select * from table limit n". Again SQL Server > chokes, as it expects the form "select top n * from table". The ADODB > lib has a neat method for building up the correct limit sql like this: > > $connection->SelectLimit('select * from table', nn); > > I haven't used it yet, but if it works as described it would remove one > big headache. I'm annoyed with myself because I wrote my own method to > do the same thing - I've been away from PHP a while, so please forgive > me if I may quite a few obvious blunders. Am I going down the right path > here? > Yes.. absolutely... Please change it in the code wherever it is necessary. > There's a fair bit of sql that can't be ported so easily unfortunately, > for instance calls to DECODE have no support in ADODB (and no real > equivalent in T-SQL that I'm aware of). There's also a huge problems > with how different data types are handled in the different DBs (e.g. > can't ORDER BY an ntext field). > Can you explain the decode issue please? What does decode in MSSQL? About the ntext i am not sure either... if there is a issue with it maybe MSSQL should use instead nvarchar fields. as I remember they can get pretty long too (5000 Chars?). > The best advice on this I've seen is to treat database portability as a > localization issue, and place the SQL in a separate file for each DB > type. This doesn't seem like a bad idea, but I'd be keen to know what > other people think. > I am skeptical about it since this won't make it easier... I am all for workarounds in the first way... only when there is no other way we should use case-depending code. Putting it in a separate file.. hmmmm... I am thinking about the debug problem... better have it in the function directly - people are lazy.. the will go to the mysql query.. change it and forget about the mssql query... To get more overview we could clean up the database functions. Someone started that by creating dbedit.php. Take a look into it. It's a lot more structured ..... but not used yet unfortunately! > My first cut of porting to SQL Server works, but it's pig ugly - lots of > case statements, testing the connection type over and over. I'd rather > do it right. Mabye you can post an example so I can understand you better how you would like it to look like. Best regards Carsten |
From: Tom T. <tom...@he...> - 2007-02-02 00:30:32
|
Hi Carsten, et al This is the sort of code I've ended up writing. As I've said, it can get ugly - especially this gets extended to support other databases. My thinking re the "localization" approach was that you'd have a single file to create if you wanted to add support for a new db. But, as you point out Carsten, there'd have to be some discipline to keep the files in sync. I've had a look at dbedit.php - it looks like a good move, putting all the db "knowldege: in one place, but I'm not sure I undertand how this will support alternative dbs. Would the alternative sql be kept in this file, or in separate dbedit files? Anyway, here's the code: $query =3D "SELECT COUNT(*) FROM {$dbprefix}saved_control\n" ."WHERE sid=3D$surveyid\n"; switch ($connect->databaseType) { case 'mysql': =09 $query .=3D "AND identifier=3D'".$_POST['savename']."'\n" ."AND access_code=3D'".md5($_POST['savepass'])."'\n"; case 'odbc_mssql': // casting ntext types to nvarchars to allow comparison of values $query .=3D "AND cast(identifier as nvarchar)=3D'".$_POST['savename']."'\n" ."AND cast(access_code as nvarchar)=3D'".md5($_POST['savepass'])."'\n"; =09 break; default:=20 die ("Couldn't create query for connection type '$connect->databaseType'");=09 }=09 Bad example perhaps, and maybe this isn't as big an issue as I think. The problem above could be solved by using dif types in the db. Most of the switches I had before related to quoted fields and LIMIT statements, both seem to be sorted. Things like DECODE are a problem, as there's no equivalent in SQL Server. Well, actually there's an undocumented feature that does something *similar* but it stores a hash not the encoded value so passwords will have to be handled completely differently - needing a switch statement or similar in the application anyway. As an aside, DECODE means something completely different in Oracle's PL/SQL. Maybe what we need is a wrapper function like the ADODB lib has for common (but differently implemented) features?, e.g. Enc($date, $fmt) I'm rabitting on again. I'll start converting LIMIT statements to use SelectLimit from ADODB and see how it goes from there. Cheers Tom |
From: Carsten S. <car...@gm...> - 2007-02-02 09:51:16
|
Hello! > Hi Carsten, et al > > This is the sort of code I've ended up writing. As I've said, it can get > ugly - especially this gets extended to support other databases. My > thinking re the "localization" approach was that you'd have a single > file to create if you wanted to add support for a new db. But, as you > point out Carsten, there'd have to be some discipline to keep the files > in sync. I've had a look at dbedit.php - it looks like a good move, > putting all the db "knowldege: in one place, but I'm not sure I > undertand how this will support alternative dbs. Would the alternative > sql be kept in this file, or in separate dbedit files? Anyway, here's > the code: I am all for keeping the queries in one file for all database types (if we take that approach) > $query = "SELECT COUNT(*) FROM {$dbprefix}saved_control\n" > ."WHERE sid=$surveyid\n"; > switch ($connect->databaseType) { > case 'mysql': > $query .= "AND identifier='".$_POST['savename']."'\n" > ."AND > access_code='".md5($_POST['savepass'])."'\n"; > case 'odbc_mssql': > // casting ntext types to nvarchars to allow comparison > of values > $query .= "AND cast(identifier as > nvarchar)='".$_POST['savename']."'\n" > ."AND cast(access_code as > nvarchar)='".md5($_POST['savepass'])."'\n"; > break; > default: > die ("Couldn't create query for connection type > '$connect->databaseType'"); > } > > Bad example perhaps, and maybe this isn't as big an issue as I think. > The problem above could be solved by using dif types in the db. Yes.. which i think we should just do. > Most of > the switches I had before related to quoted fields and LIMIT statements, > both seem to be sorted. Things like DECODE are a problem, as there's no > equivalent in SQL Server. Well, actually there's an undocumented feature > that does something *similar* but it stores a hash not the encoded value > so passwords will have to be handled completely differently - needing a > switch statement or similar in the application anyway. For the decode thing I am all for resolving it ouside the database in PHP code. I dont want the server to encode/decode anything. THis is just a left-over from the old mysql implementation that has to be replaced. > As an aside, DECODE means something completely different in Oracle's > PL/SQL. Maybe what we need is a wrapper function like the ADODB lib has > for common (but differently implemented) features?, e.g. Enc($date, > $fmt) > > I'm rabitting on again. I'll start converting LIMIT statements to use > SelectLimit from ADODB and see how it goes from there. Yes.. i think we should do that for now, replacing the datatypes in mssql with more condition-friendy ones and remove the mysql encode/decode completely and move the encoding/decoding into the php code. Can you do that? Best regards Carsten -- Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! Ideal für Modem und ISDN: http://www.gmx.net/de/go/smartsurfer |
From: Tom T. <tom...@he...> - 2007-02-04 22:38:10
|
Hi folks=20 Damn Outlook.. not 'quoting' the original message. Every day I hate = Windows more and more. -----Original Message----- From: php...@li... = [mailto:php...@li...] On Behalf = Of Carsten Schmitz Sent: Friday, 2 February 2007 8:21 PM To: php...@li... Subject: Re: [Phpsurveyor-developers] DB portability Hello!=20 > Hi Carsten, et al >=20 > This is the sort of code I've ended up writing. As I've said, it can=20 > get ugly - especially this gets extended to support other databases.=20 > My thinking re the "localization" approach was that you'd have a=20 > single file to create if you wanted to add support for a new db. But,=20 > as you point out Carsten, there'd have to be some discipline to keep=20 > the files in sync. I've had a look at dbedit.php - it looks like a=20 > good move, putting all the db "knowldege: in one place, but I'm not=20 > sure I undertand how this will support alternative dbs. Would the=20 > alternative sql be kept in this file, or in separate dbedit files?=20 > Anyway, here's the code: I am all for keeping the queries in one file for all database types (if = we take that approach) [tom] Possibly not necessary now if we get right of DECODE, use = SelectLimit and keep the sql otherwise fairly generic. =20 > $query =3D "SELECT COUNT(*) FROM {$dbprefix}saved_control\n" > ."WHERE sid=3D$surveyid\n"; > switch ($connect->databaseType) { > case 'mysql': =09 > $query .=3D "AND identifier=3D'".$_POST['savename']."'\n" > ."AND > access_code=3D'".md5($_POST['savepass'])."'\n"; > case 'odbc_mssql': > // casting ntext types to nvarchars to allow comparison of values > $query .=3D "AND cast(identifier as > nvarchar)=3D'".$_POST['savename']."'\n" > ."AND cast(access_code as > nvarchar)=3D'".md5($_POST['savepass'])."'\n"; =09 > break; > default:=20 > die ("Couldn't create query for connection type > '$connect->databaseType'");=09 > }=09 >=20 > Bad example perhaps, and maybe this isn't as big an issue as I think. > The problem above could be solved by using dif types in the db.=20 Yes.. which i think we should just do. [tom] OK, I'll do a new create-odbc_mssql.sql, coz the current one is = out of sync with the code, and just use equivalent types. > Most of > the switches I had before related to quoted fields and LIMIT=20 > statements, both seem to be sorted. Things like DECODE are a problem,=20 > as there's no equivalent in SQL Server. Well, actually there's an=20 > undocumented feature that does something *similar* but it stores a=20 > hash not the encoded value so passwords will have to be handled=20 > completely differently - needing a switch statement or similar in the = application anyway. For the decode thing I am all for resolving it ouside the database in = PHP code. I dont want the server to encode/decode anything. THis is just = a left-over from the old mysql implementation that has to be replaced. [tom] Then if it's ok with everyone I'll start stripping it out. Might = make some existing systems break though. > As an aside, DECODE means something completely different in Oracle's=20 > PL/SQL. Maybe what we need is a wrapper function like the ADODB lib=20 > has for common (but differently implemented) features?, e.g.=20 > Enc($date, > $fmt) >=20 > I'm rabitting on again. I'll start converting LIMIT statements to use=20 > SelectLimit from ADODB and see how it goes from there. Yes.. i think we should do that for now, replacing the datatypes in = mssql with more condition-friendy ones and remove the mysql = encode/decode completely and move the encoding/decoding into the php = code.=20 Can you do that? [tom] Already on it :) Best regards Carsten -- Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!=20 Ideal f=FCr Modem und ISDN: http://www.gmx.net/de/go/smartsurfer -------------------------------------------------------------------------= Using Tomcat but need to do more? Need to support web services, = security? Get stuff done quickly with pre-integrated technology to make your job = easier. Download IBM WebSphere Application Server v.1.0.1 based on Apache = Geronimo http://sel.as-us.falkag.net/sel?cmd=3Dlnk&kid=3D120709&bid=3D263057&dat=3D= 121642 _______________________________________________ PHPSurveyor-Developers mailing list PHP...@li... https://lists.sourceforge.net/lists/listinfo/phpsurveyor-developers |
From: Carsten S. <car...@gm...> - 2007-02-04 22:45:42
|
Hello Tom, Tom Tuddenham wrote: > Yes.. i think we should do that for now, replacing the datatypes in mssql with more condition-friendy ones and remove the mysql encode/decode completely and move the encoding/decoding into the php code. > > Can you do that? > > [tom] Already on it :) Great.. there is also a CREATE TEMPORARY TABLE somewhere, which we need to get rid of since alot of providers are blocking creation temp tables. (for whatever reason). If you have too much time please remove it.. if not i will take a look at it tomorrow. Another thing: Please commit at the end of every coding day . That will keep us all up-to-date regarding your progress. Thank you for your help! Carsten |