Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
*** Serious vulnerability
02-15-2014, 10:46 PM, (This post was last modified: 02-19-2014, 09:13 PM by Forums.)
#1
*** Serious vulnerability
Revision 6571 with commit message "Andrew Galuski: WOSerialNos fix for quantity number format error;" overwrites the the $_SESSION['DatabaseName'] variable with a value from the $_GET array which is undesirable for obvious reasons. Can somebody fix it?

Thanks
Tim
Reply
02-18-2014, 12:00 AM,
#2
RE: Potential vulnerability
Tim,
The only revision I sent on this file was the below.
/*if (!is_numeric($_POST['Quantity' . $i])){*/
if (!is_numeric(filter_number_format($_POST['Quantity' . $i]))){

Can you tell me where the revision is in the code that you are referring to?
Reply
02-18-2014, 12:33 AM,
#3
RE: Potential vulnerability
(02-18-2014, 12:00 AM)agaluski Wrote: Tim,
The only revision I sent on this file was the below.
/*if (!is_numeric($_POST['Quantity' . $i])){*/
if (!is_numeric(filter_number_format($_POST['Quantity' . $i]))){

Can you tell me where the revision is in the code that you are referring to?

Hi Andrew

it's here:

https://sourceforge.net/p/web-erp/code/6571/

Looking at it I am guessing that the updates to GL_TrialBalance_csv.php are not related to your work, and are possibly an accident by the person committing the work.

Thanks
Tim
Reply
02-19-2014, 06:00 PM,
#4
RE: Potential vulnerability
Hi everyone,

I'm not familiar with the GLTrialBalance_csv.php function. I have a few basic questions, any help would be appreciated.

1. I'm guessing that this function is still supposed to run if and only if the appropriate values are passed in via the URL as per the function note (and given there is no link to it in webERP). eg
//Page must be called with GLTrialBalance_csv.php?CompanyName=XXXXX&FromPeriod=Y&ToPeriod=Z[undefined=undefined]

2. Is the function also meant to run without login to webERP or should one be logged in? (reason for this post with the overwritten session var I guess)

3. If one is logged in then is it correct to say that the user should be able to pass in another database name in the URL to get it to spit out the Trial Balance for that company database (assuming the other connection info is the same)?

4. Is it true that there could be a number of scripts packaged in the webERP distribution that will run without login or are supposed to - (this one included) - going by the comment in ConnectDB, if the database name is supplied they should run? eg
/* Scripts that do not require a login must have the $DatabaseName variable set in hard code */
$_SESSION['DatabaseName'] = $DatabaseName;
$_SESSION['CompanyName']= $DatabaseName;
include_once ($PathPrefix . 'includes/ConnectDB_' . $DBType . '.inc');

I am following up on a few areas related to this, so please excuse all my basic questions; I'd just like to be sure of expected behaviours as in webERP now.

Thanks

Reply
02-19-2014, 06:24 PM, (This post was last modified: 02-19-2014, 06:24 PM by phil.)
#5
RE: Potential vulnerability
Yes this script probably should be authenticated througn the API - not sure how else to do it really.
It is for use with my webERP GL importer in libre office macro. But I tried to get it to get utf-8 characters and struggling.
The functionality was mucked up by the database obfuscation code you did - it was probably insecure previously too.
Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Reply
02-19-2014, 06:30 PM,
#6
RE: Potential vulnerability
Hi Phil,

(02-19-2014, 06:24 PM)phil Wrote: Yes this script probably should be authenticated througn the API - not sure how else to do it really.
It is for use with my webERP GL importer in libre office macro. But I tried to get it to get utf-8 characters and struggling.
The functionality was mucked up by the database obfuscation code you did - it was probably insecure previously too.

Thanks for the quick response.

I'm sure we can get it to run again but wondering what security we do really want/need in this case. eg

1. Should user be logged in?
2. Will it work for your importer if login is required?
3. Do you want it to run with different databases (all with same connect info) or was it just to run for the one specified database in the URL?

Cheers,
Reply
02-19-2014, 07:28 PM,
#7
RE: Potential vulnerability
So a TB is available to any user without logging in?? You are right I have just tried it. A quick search for internet facing webERP installations shows I can download any TB just by changing the URL. This is quite frightening really, the vulnerability is worse than I thought. I think Jo's code on the company names actually breaks this script so anybody with 4.11.2 or greater should be ok, but less than that your TB is available to all.

If this script is part of a commercial addon it should really just be distributed with that and a big warning.

Tim
Reply
02-19-2014, 08:55 PM,
#8
RE: Potential vulnerability
(02-19-2014, 07:28 PM)Forums Wrote: I think Jo's code on the company names actually breaks this script
Yes it does break the script, as long as that session var is not overwritten, and as long as no company name is passed in. As soon as the latter happens the session is set and ....

In my investigations I noticed there is a var: $AllowAnyone = true
That needs to be set for the above to happen. I'd suggest that is removed and the script is run from log in user only.

Are we sure we need that $AllowAnyone var? A few other scripts use it which may be ok - not tested them. Perhaps we can get around the script requiring AllowAnyone with some changes in the code.

I notice that this AllowAnyone was tested for in session.inc as well - not a fan of it as it allows the script to fall through and accumulate session info as if users are logged in. In fact, it actually allows the script to log in a user although they have no role permissions assigned. it doesn't feel right tho (was useful to find out as I've been investigating some other session issues resulting in users iogged in with no permissions and unable to get out of it).

I also wondered why there was the ConnectDB :
/* Scripts that do not require a login must have the $DatabaseName variable set in hard code */
Not really a fan of that - I'm sure there must be some other way around it to do the job required.

Does anyone mind me taking out the AllowAnyone from the GLTrialBalance_csv.php script at least for now?

We can then look at fixing the said function to run, with a logged in user. If that causes problems there could be a way around it depending on what is calling the script.

Thanks
Reply
02-19-2014, 09:12 PM,
#9
RE: Potential vulnerability
Hi Jo,

It needs taking out urgently. As it is, anybodies TB is downloadable by anybody on the same network. If the webERP installation is on the internet then this means anybody on the internet can download your TB.

I would advise anybody who doesn't have an urgent need for this script to delete it from their installations immediately. It was deliberately committed back in November 2009 so all code since then is vulnerable.

I was project admin at the time and I missed it, so I apologise to everyone :-(

Tim
Reply
02-19-2014, 10:24 PM,
#10
RE: Potential vulnerability
I have commented out the $AllowAnyone= true;

This fixes the immediate security issue - it allows anyone to use if required by temporarily uncommenting that line; and fixes security for those that have the fix ...

I am interested to look at the code surrounding this further in relation to login and connectdb. Further I will look at a solution for logged in users for the function in question.

Phil, there could be a solution if you need to access it from your macro but I'm not familiar with libre office macros.

Cheers,


Reply


Forum Jump:


Users browsing this thread: 1 Guest(s)