[Phpmyadmin-devel] Problems with Garvin's Patches
Hi Garvin & list, You've submitted a huge feature-bomb, so what about RC1 next Sunday? ;-p Du scheinst irgendwie nicht ausgelastet zu sein. Oder hattest du einfach nur zu viel Zeit? :o) OK, let's get serious. When trying to browse any table, I get: SELECT column_name, mimetype, transformation, transformation_options FROM `column_comments` WHERE db_name = 'my_database' AND table_name = 'my_table' AND ( mimetype != '' OR transformation != '' OR transformation_options != '' ) Unkown field 'mimetype' in field list. The strange thing is: Even if I send a query that does not affect any database, like "SHOW STATUS" for instance, this error appears. Don't you need this query for table-specific SELECTs only? Your new code obviously does not seem to accept the old layout of the column_comments table. Of course, it's no problem for me to update my table structure, but we'll probably get flooded with support request concerning this. About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else. It's just meant for quick queries, not for large imports. That is why I'd like to drop the whole upload stuff. We still keep the SQL tab for this. The link "import textfile" and the table field list don't work at all, so let's drop them, too. Keep on the good work, Alexander M. Turek <alex@bugfixes.info> +-----------------------------+ | The phpMyAdmin Project | | http://www.phpmyadmin.net | | rabus@users.sourceforge.net | +-----------------------------+ | [bugfixes.info] | | http://www.bugfixes.info | | rabus@bugfixes.info | +-----------------------------+
Hi Alex & list!
You've submitted a huge feature-bomb, so what about RC1 next Sunday? ;-p Du scheinst irgendwie nicht ausgelastet zu sein. Oder hattest du einfach nur zu viel Zeit? :o)
Well, I was on vacation and wanted to give something back to the community. ;)
SELECT column_name, mimetype, transformation, transformation_options FROM `column_comments` WHERE db_name = 'my_database' AND table_name = 'my_table' AND ( mimetype != '' OR transformation != '' OR transformation_options != '' )
This query should only be used to build the table list in the left frame and for the strucutre (if 'display comments' is true) and for the headers/results of any query. I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else.
I can make the query history configurable, but I definitely want to have it.
It's just meant for quick queries, not for large imports. That is why I'd like to drop the whole upload stuff. We still keep the SQL tab for this.
Okay, I can talk about that. But as you can see, the query window only uses the snippet from tbl_query_box.php3, so that has to be modified as well.
The link "import textfile" and the table field list don't work at all, so let's drop them, too.
They don't work? I didn't test importing yet, but selecting fields worked for me once...
Keep on the good work,
Thanks! I appreciate that a lot :) -- Bye, Garvin.
On Tue, Feb 25, 2003 at 08:50:55AM +0100, Garvin Hicking wrote:
SELECT column_name, mimetype, transformation, transformation_options FROM `column_comments` WHERE db_name = 'my_database' AND table_name = 'my_table' AND ( mimetype != '' OR transformation != '' OR transformation_options != '' ) This query should only be used to build the table list in the left frame and for the strucutre (if 'display comments' is true) and for the headers/results of any query. One bug to report, on the table create/alter pages the number of data fields and the actual fields do NOT line up.
on the header row, after extra, there is: Comments, Comments, MIME-type, Browser transformation, Transformation options*** for the actual data rows: text field, dropdown of auto-detect and mimetypes, dropdown of transformations, text field, (missing cell). This is just a little off by one error somewhere. Looking at the HTML for that page, I see you are passing filenames for the field_transformation variable. Take out the .inc.php3 part and go and double check that code for security holes, as it may be susceptible to data injection.
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way? There should be a variable in the configuration file controlling if the transform system should be used at all. If it is set to false, then it does not get used. For the present moment, lets default it to false while we work on the code. If the user activates it, look at showing an error on the relations error page, and just a note elsewhere that they should check that page.
I don't think it is right to allow the code to upgrade the comment-table structure automatically. certainly we can have a script page that might offer to do this if the PMA user has the correct privileges in it's database, but we do not do it automatically. point the user to the manual moreover. I believe the PMA user permissions that are mentioned in the manual are only SELECT, INSERT, UPDATE, DELETE. no ALTER/CREATE table there... On that note, please update Documentation for your tranforms...
About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else. I can make the query history configurable, but I definitely want to have it. It's just meant for quick queries, not for large imports. That is why I'd like to drop the whole upload stuff. We still keep the SQL tab for this. Okay, I can talk about that. But as you can see, the query window only uses the snippet from tbl_query_box.php3, so that has to be modified as well. Here is an idea for the quickbox, give it three mini-tabs: "SQL", "Files", "History" SQL - just the query box with the go and the field helper Files - query from uploaded file and insert textfile into table History - SQL history feature
The link "import textfile" and the table field list don't work at all, so let's drop them, too. They don't work? I didn't test importing yet, but selecting fields worked for me once... The field helper does work here, but ONLY if you start the quickbox from a table page.
Garvin, a question + suggestion: does the SQL history work thru links only? What about doing it via a new table in the PMA database instead, configurable properly. This is after it gets moved to a seperate tab. it should have a configuration option to only display the N most recent items. suggested table layout: id - longint auto_increment primary key username - varchar(64) index db - varchar(64) index timevalue - timestamp index sqlquery - varchar(255) should work for short queries, but we may want one of the TEXT types instead (i think 64k is a better limit) behaviour: on login and logout or a large number of rows existing, run a query on the table that deletes ALL but the most N recent items for the current user+database pair. also have an explict button on the SQL history page that can clean up the table. To cater for as many usergroups as possible, there should be yet another config option to allow the admin to set the code to use either the current SQL history via links or the new idea via an SQL table, or disable it totally. (Do NOT use the value of the tablename for this behaviour).
Keep on the good work, Thanks! I appreciate that a lot :) Same from this side, it all looks like it will be really good once the bugs are worked out.
-- Robin Hugh Johnson E-Mail : robbat2@orbis-terrarum.net Home Page : http://www.orbis-terrarum.net/?l=people.robbat2 ICQ# : 30269588 or 41961639 GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85
robbat2@orbis-terrarum.net wrote:
About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else.
I can make the query history configurable, but I definitely want to have it.
It's just meant for quick queries, not for large imports. That is why I'd like to drop the whole upload stuff. We still keep the SQL tab for this.
Okay, I can talk about that. But as you can see, the query window only uses the snippet from tbl_query_box.php3, so that has to be modified as well.
Here is an idea for the quickbox, give it three mini-tabs: "SQL", "Files", "History" SQL - just the query box with the go and the field helper Files - query from uploaded file and insert textfile into table History - SQL history feature
Since lots of users are looking for "Import", why not: SQL Import History And we could probably remove in table view the SQL tab, just be sure we do something logical for people with $cfg['DefaultTabTable']='tbl_properties.php3' like showing the SQL window? Marc
Hi Robin! I just reply to parts of your mail, because other items simply went to my TODO-List.
One bug to report, on the table create/alter pages the number of data fields and the actual fields do NOT line up.
I today fixed that issue, it was because of to conflicting DIFFs.
Looking at the HTML for that page, I see you are passing filenames for the field_transformation variable. Take out the .inc.php3 part and go and double check that code for security holes, as it may be susceptible to data injection.
I got a grip of that problem on the other side. I don't do any input validation, but rely on the dropdown-input. Because when evaluation the columns in browse mode, there's a check to only allow a transformation, if it fits certain rules (<mimetype>_<subtype>__<transformation>.inc.php3), and only in the libraries/transformations directory. Because data is passed via a select field I surely think we could trust user data there. Warning shouldn't be an issue, because you can only trick false data in by using your own POST/GET-requests, so in this case the user already knows, he is hacking into our application and may get false results. But even if he gets an invalid transform into the table, it simply isn't used in the output. Any objections to this approach, which is already coded this way and thereby takes no coding-time to keep it that way (*broad hint* ;-))
On that note, please update Documentation for your tranforms...
This is the last point on my TODO-list, but I won't forget that. :)
Here is an idea for the quickbox, give it three mini-tabs:
Maybe it is a bit complicated to make a three-part query window, so I'll see what can be done there. Worst thing would be to have input-buttons as TAB-icons instead of text links, or to rely on javascript for that, again.
Garvin, a question + suggestion: does the SQL history work thru links only?
In reality, this works by javascript and POSTing the data. So, yes. Query history can and will get tuned to table support.
behaviour: on login and logout or a large number of rows existing, run a query on
How to you get to know, if a user "logs in" or "logs out", without session management? You mean, everytime a user makes a call to main.php3? -- Bye, Garvin.
On Tue, Feb 25, 2003 at 03:03:55PM +0100, Garvin Hicking wrote:
Looking at the HTML for that page, I see you are passing filenames for the field_transformation variable. Take out the .inc.php3 part and go and double check that code for security holes, as it may be susceptible to data injection. I got a grip of that problem on the other side. I don't do any input validation, but rely on the dropdown-input. Because when evaluation the columns in browse mode, there's a check to only allow a transformation, if it fits certain rules (<mimetype>_<subtype>__<transformation>.inc.php3), and only in the libraries/transformations directory. Because data is passed via a select field I surely think we could trust user data there. This is where a malicous input can be made, and it's not as difficult as a custom POST/GET even, just copying the HTML page, and changing a few URLS.
Warning shouldn't be an issue, because you can only trick false data in by using your own POST/GET-requests, so in this case the user already knows, he is hacking into our application and may get false results. But even if he gets an invalid transform into the table, it simply isn't used in the output. In this case, evil user is a malicuos user that has access to a database or table already, and wants to root the system. evil user adds a tranform that reads a piece of data from the server as a root user, somewhere else on the file system, say /tmp (using the docSQL bug). the fix can conform to your naming requirements or not. Now evil user makes his own table, and puts in a value of '/etc/shadow' or any file he wants. he then gets the exact transform he wants to run on the '/etc/shadow' string. He's now got your entire /etc/shadow file, with your passwords or worse.
Any objections to this approach, which is already coded this way and thereby takes no coding-time to keep it that way (*broad hint* ;-)) The fix shouldn't be hard. Just remove the .inc.php3 part off the data that is passed to the browser, and add it back in when the data comes back, along with making sure all the input data is properly escaped to prevent the user getting out of the specific directory.
Here is an idea for the quickbox, give it three mini-tabs: Maybe it is a bit complicated to make a three-part query window, so I'll see what can be done there. Worst thing would be to have input-buttons as TAB-icons instead of text links, or to rely on javascript for that, again. Just seperate the code for the different parts of the tab window into different files maybe? or is the bug JS related?
behaviour: on login and logout or a large number of rows existing, run a query on How to you get to know, if a user "logs in" or "logs out", without session management? You mean, everytime a user makes a call to main.php3? logout - the logout link in the left frame login - on load of the frameset
-- Robin Hugh Johnson E-Mail : robbat2@orbis-terrarum.net Home Page : http://www.orbis-terrarum.net/?l=people.robbat2 ICQ# : 30269588 or 41961639 GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85
robbat2@orbis-terrarum.net wrote:
In this case, evil user is a malicuos user that has access to a database or table already, and wants to root the system. evil user adds a tranform that reads a piece of data from the server as a root user, somewhere else on the file system, say /tmp (using the docSQL bug). the fix can conform to your naming requirements or not. Now evil user makes his own table, and puts in a value of '/etc/shadow' or any file he wants. he then gets the exact transform he wants to run on the '/etc/shadow' string. He's now got your entire /etc/shadow file, with your passwords or worse.
Robin, I don't understand your point. I think that, for this to work, the web server would have to run under a privileged user (a thing definitely not recommended) and/or PHP would not be set in safe mode. And if PHP is not in safe mode, it's a lot easier than you describe to read in some protected files. Marc
Hi Robin!
This is where a malicous input can be made, and it's not as difficult as a custom POST/GET even, just copying the HTML page, and changing a few URLS.
But this is certainly not standard workflow, you have to put your hands on an editor.
In this case, evil user is a malicuos user that has access to a database or table already, and wants to root the system. evil user adds a tranform that reads a piece of data from the server as a root user, somewhere else on the file system, say /tmp (using the docSQL bug). the fix can conform to your naming requirements or not. Now evil user makes his own table, and puts in a value of '/etc/shadow' or any file he wants. he then gets the exact transform he wants to run on the '/etc/shadow' string. He's now got your entire /etc/shadow file, with your passwords or worse.
No. Evil user only transmits the filename he wants to have. This is now inserted into the database. Now, nothing else happens, he has to browse through a table. There, PMA reads what transformation should be applied. Because his new entry is not inside PMA/libraries/transformations (checked via RegEx), the function inside this file is not executed. Remember, no files are uploaded inside the directory. Only if the user can put his new file into the libraries/transformation directory, he can gain access to file functions. But then, he could just delete all files because he already has access. :)
Here is an idea for the quickbox, give it three mini-tabs: Maybe it is a bit complicated to make a three-part query window, so I'll see what can be done there. Worst thing would be to have input-buttons as TAB-icons instead of text links, or to rely on javascript for that, again. Just seperate the code for the different parts of the tab window into different files maybe? or is the bug JS related?
It is JS related, because currently all history-actions are put into a single form and transmitted from there to itself over and over again.
logout - the logout link in the left frame login - on load of the frameset
Alright. Stupid me. :-) -- Bye, Garvin.
On Tue, Feb 25, 2003 at 08:50:55AM +0100, Garvin Hicking wrote:
You've submitted a huge feature-bomb, so what about RC1 next Sunday? ;-p Actually, as an idea, how about we get those features stable as per my other email, and merge in the rest of the stuff from the patch tracker, and then do a release?
3-4 weeks for coding+fixes, then a 2 week release cycle, or maybe a little longer. It'll be our easter release.
Du scheinst irgendwie nicht ausgelastet zu sein. Oder hattest du einfach nur zu viel Zeit? :o) Well, I was on vacation and wanted to give something back to the community. ;) My german's not so great, I only caught the last sentance: "Do you simply have too much time?".
Time is something we all value, and put to great use. The SQL parser system is a result of a chunk of time I had last summer. Coming this summer (or maybe sooner ;-) the long-discussed DB-based configuration system! It's going to be another feature-bomb, as it will cause a major revolution in how large ISP setups of phpMyAdmin work. (There is will actually be a question following on this note in a few days, for a problem i've had...) -- Robin Hugh Johnson E-Mail : robbat2@orbis-terrarum.net Home Page : http://www.orbis-terrarum.net/?l=people.robbat2 ICQ# : 30269588 or 41961639 GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85
Garvin Hicking wrote:
Hi Alex & list!
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
I agree with Robin's comment about this.
About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else.
I can make the query history configurable, but I definitely want to have it.
It's just meant for quick queries, not for large imports. That is why I'd like to drop the whole upload stuff. We still keep the SQL tab for this.
Okay, I can talk about that. But as you can see, the query window only uses the snippet from tbl_query_box.php3, so that has to be modified as well.
The link "import textfile" and the table field list don't work at all, so let's drop them, too.
Here the table field list work. But the "Insert data from textfile" brings another page in this window, we lose the previous contents, and I would prefer the mini-tab idea. But even with the mini-tab "Import", the Insert data from textfile dialog should go on the backpage, where we see the results of a query. I like the query history!
They don't work? I didn't test importing yet, but selecting fields worked for me once...
Keep on the good work,
Thanks! I appreciate that a lot :)
Well I also encourage you about those big features! Worthy of being named 2.5.0.
Hi all! To summarize, I will incorporate all those things you mentioned in the next time. I especially like the mini-tab idea, I'll see how to work that out (shouldn't be that hard). I also like the idea of a new sql-history PMA-Table, but I will make that optionally. So everybody can get the history ready-to-go, but advanced users are able to get a persistent history. Just give me a few days for that. :-)
Well I also encourage you about those big features! Worthy of being named 2.5.0.
Thanks a lot! :) Regards, Garvin.
Hi Garvin & list, -----Original Message----- From: Garvin Hicking
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
I disagree with that because you cannot expect the user to have the privileges he needs to change the table structure. Imho, the best solution would be to modify the code that checks the status of the relational tables. Furthermore, PMA_mysqlDie() shouldn't be called if the query fails: In this case, your feature should be disabled, instead. Nevertheless, you should documentate the new configuration directives and table structure.
About you query box: It appears a little large to me. Imho, we should only have the <textarea> and the submit button there, nothing else.
I can make the query history configurable, but I definitely want to have it.
Sorry, I did not notice the history feature at first look. Of course I don't want to drop it, either! Happy fixing :o) Regards, Alexander M. Turek <alex@bugfixes.info> +-----------------------------+ | The phpMyAdmin Project | | http://www.phpmyadmin.net | | rabus@users.sourceforge.net | +-----------------------------+ | [bugfixes.info] | | http://www.bugfixes.info | | rabus@bugfixes.info | +-----------------------------+
'morning, On Tue, Feb 25, 2003 at 01:43:40PM +0100, Rabus wrote:
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
I disagree with that because you cannot expect the user to have the privileges he needs to change the table structure. Imho, the best solution would be to modify the code that checks the status of the relational tables. Furthermore, PMA_mysqlDie() shouldn't be called if the query fails: In this case, your feature should be disabled, instead.
What about an automatic check on root login ? - if all the tables are existing & uptodate -> work as usual - if some tables are outdated: display a kind of splash-screen suggesting the required changes, with an "Upgrade" button which fixes everything - and if there are no tables at all (bookmarks, relations, etc): allow the root user to create everything "on the fly" (text fields asking for User/DB name to use), and which display a part of code to copy paste for the config.inc.php file (a little bit like phpMyEdit). so a kind of user-friendly "setup.php" :) Cheers, Olivier -- _________________________________________________________________ Olivier Mueller - om@8304.ch - PGPkeyID: 0E84D2EA - Switzerland qmail projects: http://omail.omnis.ch - http://webmail.omnis.ch
On Tue, Feb 25, 2003 at 02:00:34PM +0100, Olivier M. wrote:
What about an automatic check on root login ? No, rather than root login, check that the user they logged in as has permissions to the tables required, or create table if the tables don't exist. This is because a lot of users only have a small setup from an ISP with their own copy of PMA.
See RFE #486080, #577328, #601016...
- if all the tables are existing & uptodate -> work as usual
- if some tables are outdated: display a kind of splash-screen suggesting the required changes, with an "Upgrade" button which fixes everything
- and if there are no tables at all (bookmarks, relations, etc): allow the root user to create everything "on the fly" (text fields asking for User/DB name to use), and which display a part of code to copy paste for the config.inc.php file (a little bit like phpMyEdit).
so a kind of user-friendly "setup.php" :)
-- Robin Hugh Johnson E-Mail : robbat2@orbis-terrarum.net Home Page : http://www.orbis-terrarum.net/?l=people.robbat2 ICQ# : 30269588 or 41961639 GnuPG FP : 11AC BA4F 4778 E3F6 E4ED F38E B27B 944E 3488 4E85
On Tue, Feb 25, 2003 at 05:16:02AM -0800, robbat2@orbis-terrarum.net wrote:
On Tue, Feb 25, 2003 at 02:00:34PM +0100, Olivier M. wrote:
What about an automatic check on root login ? No, rather than root login, check that the user they logged in as has permissions to the tables required, or create table if the tables don't exist. This is because a lot of users only have a small setup from an ISP with their own copy of PMA.
yes, that may be a problem... in this case, the only option would be to display a warning, or to disable the feature based on the "outdated" tables... Olivier
Olivier M. wrote:
'morning,
On Tue, Feb 25, 2003 at 01:43:40PM +0100, Rabus wrote:
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
I disagree with that because you cannot expect the user to have the privileges he needs to change the table structure. Imho, the best solution would be to modify the code that checks the status of the relational tables. Furthermore, PMA_mysqlDie() shouldn't be called if the query fails: In this case, your feature should be disabled, instead.
What about an automatic check on root login ?
I like this idea. Using $is_superuser (which is set with a check on 'USE mysql')? Or checking directly the user's rights on $cfg['Servers'][$i]['pmadb']? (Let's assume he has full rights on all the underlying tables).
- if all the tables are existing & uptodate -> work as usual
- if some tables are outdated: display a kind of splash-screen suggesting the required changes, with an "Upgrade" button which fixes everything
- and if there are no tables at all (bookmarks, relations, etc): allow the root user to create everything "on the fly" (text fields asking for User/DB name to use), and which display a part of code to copy paste for the config.inc.php file (a little bit like phpMyEdit).
so a kind of user-friendly "setup.php" :)
Cheers, Olivier
-----Original Message----- From: Olivier M. [mailto:qmail@orion.8304.ch]
On Tue, Feb 25, 2003 at 01:43:40PM +0100, Rabus wrote:
I will add a code to "upgrade" the comment-table structure, if the fields are missing. Or is emitting a red warning on the start page a better way?
I disagree with that because you cannot expect the user to have the privileges he needs to change the table structure. Imho, the best solution would be to modify the code that checks the status of the relational tables. Furthermore, PMA_mysqlDie() shouldn't be called if the query fails: In this case, your feature should be disabled, instead.
What about an automatic check on root login ?
- if all the tables are existing & uptodate -> work as usual
- if some tables are outdated: display a kind of splash-screen suggesting the required changes, with an "Upgrade" button which fixes everything
- and if there are no tables at all (bookmarks, relations, etc): allow the root user to create everything "on the fly" (text fields asking for User/DB name to use), and which display a part of code to copy paste for the config.inc.php file (a little bit like phpMyEdit).
so a kind of user-friendly "setup.php" :)
Sounds like a part of feature #601016. Since I like this idea, I'll try (to find some time to) code feature #601016. Only, of course, if you don't have any objections. ;-) Regards, Alexander M. Turek <alex@bugfixes.info> +-----------------------------+ | The phpMyAdmin Project | | http://www.phpmyadmin.net | | rabus@users.sourceforge.net | +-----------------------------+ | [bugfixes.info] | | http://www.bugfixes.info | | rabus@bugfixes.info | +-----------------------------+
Hi!
so a kind of user-friendly "setup.php" :)
Since I like this idea, I'll try (to find some time to) code feature #601016. Only, of course, if you don't have any objections. ;-)
I like the idea as well! Would be great if you find the time to code this. I will then only issue a warning (don't know on which pages yet, but I'll find the appropriate places) if old comment-tables are found and point to the script provided by you. :) -- Bye, Garvin.
participants (6)
-
Garvin Hicking -
Marc Delisle -
Marc Delisle
-
Olivier M. -
Rabus -
robbat2＠orbis-terrarum.net