• Registered: 04/04/2009
  • Posts: 6

Topic: Security Issue

Just started to use FA and it's a breath of fresh air compared to using openERP for basic accounting functions. Sometimes, you just want to stick with the basics.

Well, I was looking to customize the customers page to display a paginated list of customers instead of the drop down list. I noticed some SQL code that I thought should be brought to you guys attention.

Look at customers line 221 (and several other places):

$sql = "SELECT * FROM ".TB_PREF."debtors_master WHERE debtor_no = '" . $_POST['customer_id'] . "'";

A big red flag is how data is being posted into the sql statement without being cleansed first. I know that these variables are posted with AJAX but with something as simple as Firebug (which allows you to change HTML in real time, i.e one could created their own form) one can easily post arbitrary code into these statements and even possibly wipe out an entire DB.

An easy fix would be to simply perform the db_escape function and set it equal to the post.

$_POST['customer_id'] = db_escape($_POST['customer_id']);
(might not work if this includes functions that require the connection to already be established.)

This will eliminate the need to change every instance in the entire script but either way this could be an unfortunate way to lose all your data.

This is just what I see on first glance, so if I'm wrong please correct me.

Thanks,
Yeshua

  • Registered: 12/25/2007
  • Posts: 2,446

Re: Security Issue

Hello Yeshua,

Yes you are definitely  right - this is not good way of dealing with POST vars.
In most cases this is  only potential danger as user have access to the destructive functions (delete,update) anyway. MySQL does not accept multiply statements in one query, so seems that operations coded in sql statements cannot be spread on other tables.

Anyway thank you very much for your piercing audit smile. 
Main CVS trunk updated.

Janusz

  • Registered: 04/04/2009
  • Posts: 6

Re: Security Issue

itronics wrote:

MySQL does not accept multiply statements in one query, so seems that operations coded in sql statements cannot be spread on other tables.
Janusz

You're right but could one not close the parameter out  and run a sub-query  Either way it's still a potential danger.

While I'm on a roll ( and being lazy), I'm trying to remove some of the columns in the invoice header2 (cust  ref, our ref, etc) but for the life of me, I can't seem to find where the columns are being set. I can see where the lines and fills are being drawn, but the vertical separations seem to be perplexing me.

Also, I'd like save the generated invoices in a blob table with the customers id in a batch function, but the generation only seems to occur on demand primarily (excluding the cache). Any pointers would be appreciated.

BTW, I couldn't wait to figure out how to generate the tables created by the UI classes (actually, I think their just functions that could easily be combined into a single class) so I made a dirty table and some while loops to generate list data into normal tables. My reasoning comes from the Ajax and the constant guessing (or maybe choosing the first one) of the list (products, customers, etc) and having to continually run wildcard searches. I would like if someone would to look at the file and convert it to more compatible (similar) code setup that the project already has in use. This would help me implement the code in other areas where searching may not be the fastest option (e.g. small number of records.)

Thanks for the help.

Yeshua

  • Registered: 12/25/2007
  • Posts: 2,446

Re: Security Issue

yeshuawatso wrote:

You're right but could one not close the parameter out  and run a sub-query  Either way it's still a potential danger.

I can't provide proof of concept code for mysql, but definitely this was security hole.

While I'm on a roll ( and being lazy), I'm trying to remove some of the columns in the invoice header2 (cust  ref, our ref, etc) but for the life of me, I can't seem to find where the columns are being set. I can see where the lines and fills are being drawn, but the vertical separations seem to be perplexing me.

Vertical lines are set in lines 47-50 of header2.inc.

Also, I'd like save the generated invoices in a blob table with the customers id in a batch function, but the generation only seems to occur on demand primarily (excluding the cache). Any pointers would be appreciated.

Yes, all reports including documents like invoices are generated on demand from db data. But in any case the resulting pdf is stored in temporary file in company/0/*.pdf. See line 385 or 410 of pdf_report.inc You can catch the pdf code here.

BTW, I couldn't wait to figure out how to generate the tables created by the UI classes (actually, I think their just functions that could easily be combined into a single class) so I made a dirty table and some while loops to generate list data into normal tables. My reasoning comes from the Ajax and the constant guessing (or maybe choosing the first one) of the list (products, customers, etc) and having to continually run wildcard searches. I would like if someone would to look at the file and convert it to more compatible (similar) code setup that the project already has in use. This would help me implement the code in other areas where searching may not be the fastest option (e.g. small number of records.)

If you simply want to have all customers available in list selector without searching - set 'Search Customer List' option in company preferences to off. This is good for small databases, but for the company data containing hundreds or thousands of customers you'll better leave this option on. The same list selector behaviour and related  company preference option you have for suppliers and items.

Janusz

  • Registered: 04/04/2009
  • Posts: 6

Re: Security Issue

Yeah I figured out the vertical lines (and everything else in the invoice report) only a few minutes after making that post. The lines I wanting to change were in a loop.

As far as the list/tables go, I didn't realize that leaving the search boxes off would default them to a table. If that's the case then I can change the code to search and give a table that has been paginated. Adding a limit and ranges on the ids should make this quiet possible.

BTW, back to the invoice templates, I'm going to work on a converter that will take a 2003 xlm excel file and output a usable header. These particular xlm files can be parsed without any additional classes or libraries. I'll let you guys know how it goes.

Thanks for all the help.

Yeshua

  • Registered: 12/25/2007
  • Posts: 2,446

Re: Security Issue

I'm still not sure what do you want to achieve.  IIf you need paged customer table you don't have to reinvent the wheel. We have table pager implemented in db_pager.inc file.

No, leaving the option does not change customer list selector into a table of course. But in fact there is not too much place on the page and customer record contains quite a lot of data, so using list selector instead of a table (even paged) is optimal solution. We try to implement user interface in such a way that scrolling page up and down is not needed.

Layout import from xls file seems to be interesting option, especially for those who have  already reports generated by another accounting  third party software.

Janusz

  • Registered: 04/04/2009
  • Posts: 6

Re: Security Issue

itronics wrote:

I'm still not sure what do you want to achieve.  IIf you need paged customer table you don't have to reinvent the wheel. We have table pager implemented in db_pager.inc file.

Again, just found this out too. I noticed it in the customer inquiry page--again, after I made that post from my BB.

itronics wrote:

No, leaving the option does not change customer list selector into a table of course. But in fact there is not too much place on the page and customer record contains quite a lot of data, so using list selector instead of a table (even paged) is optimal solution. We try to implement user interface in such a way that scrolling page up and down is not needed.

You are correct, however, this is not something that I am planning on submitting to the CVS but more of a personal customization. The table I've developed only displays the last 15 records and then paginates in descending order. The way our ERPs are being used, the last few customers, invoices, ect. are the ones being accessed blindly until the user becomes more familiar with the customer. At that point, searching would be most ideal (Space->name/num->select.)

As far as the layout from excel, I've already ran into a few snags; namely, borders and calculating the lengths of the lines in the PDF. I'm considering scrapping the original concept and just use a class to parse the xls file for preset variables, replace those variables with actual data. From there, I can either export the Excel file back to the user or take the parsed data and send it as html to domPDF (excel->html->domPDF). This is terribly inefficient but for my meager coding skills, it's much easier.

Normally, on instances like this, I would hire someone else to do the coding and send the code back to the community to do as they please. In this case, I don't need to do that as I've already changed the reports to output for my specific needs. The primary reason I switched from openERP to FA is because of the source code being in a language I am comfortable with. (Python sucks)

One suggestion though is the use of interchangeable report templates installable like modules. This way, a gallery could be put up of different types of templates (submitted by the users of course) for various reports similar to MS Office's and OpenOffice templates. Combined this with a SQLtable and customization could be a hell of a lot easier. Instead of changing files, one could change the template settings in a gui.

I'm just floating ideas and this has become more of a "Wish List" post now.