Latest version of OpenEMR seems to be fraught with SQL Injection Vulnerabilities

ariesbeck wrote on Wednesday, April 10, 2013:

I installed OpenEMR and have been doing some minor customization work for a client of ours. I decided to look into the login process to get a general sense of the security of the system, figuring the login would be the most reviewed and secured area, and I have to say I was quite shocked. Once I got past the fact that the latest version still uses a salt-less hashing algorithm I noticed that all of the SQL queries in auth.inc are susceptible to SQL injection.

Am I wrong about this? As far as I can see the call to authNewSession uses unaltered POST variables as its arguments. Those arguments are directly passed into an SQL call such as “update users set password=’$new_upgrade_pass’ where username = ‘$user’”.

I hope I am wrong about this. A lot of new features are being added everyday to OpenEMR and I am truly grateful for all it provides. But if SQL Injection holes exist, and especially during login, then the focus really needs to be security before all else. We are talking about patient data here, arguably some of the most sensitive data existent in the commercial world.

The combination of the auth module having sql injection vulnerabilities AND the fact that hashing is done on the client side basically means that if the sql injection allowed a user to see the hashed password of any user, all they would have to do would be send that hash back to the login script to gain access to that account. Hashing on the client side is completely and totally pointless. One of the huge benefits of hashing is to never store the raw value that would need to be passed to the form to authenticate. This way if a user somehow retrieved the hashed password of a user from a misguided sql statement they would not be able to simply use that password to login.

It seems someone could do a regular expression query to identify any calls to sqlStatement which contain a $ in the first argument.

I can’t dedicate enough time at this exact moment but I may become more involved and try to help patch these vulnerabilities because they are very very serious. I will need to come up with quicker solutions and probably limit access to my install to specific IP addresses and client side certificates until these issues can be addressed. I cant believe a hacker hasn’t completely dumped every OpenEMR system deployed to date.

Aaron

bradymiller wrote on Wednesday, April 10, 2013:

Hi Aaron,

Check out this page on OpenEMR Security and ongoing projects:
http://www.open-emr.org/wiki/index.php/Codebase_Security

As I recall, can’t use sql-injection on the login script pre-authentication because of the acl user check first(if I am wrong then please correct me; our priority from a security perspective now is to at least close all the pre-authentication vulnerabilities). Of course, as is covered in the wiki page, there are numerous sql-injection post-authentication vulnerabilities along with a plan (and existing tools) to shut them all down. At this point, all new scripts have to follow the guidelines discussed there, but there is still lots of legacy code, including the auth.inc, to convert. I think converting the auth.inc related module/scripts obviously makes a lot of sense, but before doing this, suggest waiting until Kevin is done with his current work to migrate the hashing to the server side(along with what appears to be a salting mechanism in his ongoing github branch on this item):
http://sourceforge.net/p/openemr/discussion/202506/thread/e704ff97/

-brady
OpenEMR

robertrambo wrote on Wednesday, April 10, 2013:

Hi Aaron,

As you stated there is most to be done on the installer side as far as limiting access.

There are some topics on the forum for hardening a OpenEMR install.
http://www.oemr.org/wiki/OpenEMR_4.1.1_Linux_Installation
^ Above covers some of the directory allow disallow also the chmod for sensitive files
If you look into professional support and hosting you will see that most IT use a VPN
Also bcrypt for drive encryption

It is extremely effective to run a intranet as opposed to a internet system
of course you can always provide internet service on chosen nodes by networking hardware firewalls etc. This is where it is up to an individual installer and we hope that the people working for us can be trusted and have signed a hipaa compliance agreement as part of their employment!

Brady has been very pro actively closing pre-authorization security holes as a priority
I am glad that someone else has a interest in the security of the system these are the things the team (last check was 190 developers?)are aware of and some others,

http://www.open-emr.org/wiki/index.php/Codebase_Security

you will find complete transparency here while other vendors may not disclose their security holes we want ours fixed as soon as possible the easiest way is by full disclosure
Constantly testing.

Other issues that affect the project are security holes from time to time In Apache MySQL and PHP these will need to be patched ASAP following Best Practices the code is constantly evolving as well as the means attackers use to gain access. Cyber threat is a national security issue.

Please to help start yourself a git fork on sf
Use a test server setup the way OpenEMR recommends above.
Share your setup advice with us we are here for each other

Any bad security threats you find you may want to PM to a board member they usually have a patch out in a day or so then they disclose it to the public that way we don’t have any 0 day exploits on the loose. As a security analyst or a techie we always want to give the exploit to the vendor before we make a release. Then they have a couple days to fix it.

There are some security holes still listed by sec db’s out there that have been patched and have not been corrected on everyone’s websites to reflect OpenEMR’s remedy.
This need to be addressed still because it is time consuming.

I provided this link from the AMA on PHI and HIPAA it links to NIST and others although it is not as current as the information daily pen testers have due to their innate knowledge of of current technologies its a starting reference! It seems that you already have knowledge of all of this but maybe helpful for someone else.

http://www.ama-assn.org/resources/doc/psa/hipaa-phi-encryption.pdf

You have a very valid point that much work needs to be done luckily there are many developers that devote countless hours to this project thats what makes it exist and possible.

Please continue to share I appreciate your effort I understand we all very busy

I hope this information helps you with your install
Its all I could think to share in a nutshell.

As stated with a hardened install your concerns will probably not be as worrisome till they are corrected and or improved.

Thank you for hearing the extended rant
-Rob

ariesbeck wrote on Wednesday, April 10, 2013:

Brady,

You might be right. If we assume that the ACL check function is not vulnerable then the first sql statement in authNewSession on line 115 is “technically” ok.

However, the sql statements in auth.inc are still something I would say are vulnerable. They can easily be updated to use the query builder and there are only a few. The potentially vulnerable queries are on lines:
115
131
144
171
212
224
232
237
242
269
274
337

The SQL queries on lines 294, and 304 are not susceptible but don’t follow the convention that I have seen going forward for openemr.

I think these vulnerabilities should be patched sooner than I will be able to do so considering I don’t have a branch setup yet. Its a very simple procedure, pull out all variables from the sql statement and put in ? then put the variables in an array as the second parameter.

Aaron

bradymiller wrote on Thursday, April 11, 2013:

Hi,

Check this out for the steps to migrate scripts to the new model:
Codebase_Security#SQL-Injection_and_Cross-Scripting_Prevention

Note that it needs be done at the script/module level to avoid breaking things for users that are still using magic quotes, so the correct way to go about it is to convert the login screen along with the auth.inc library. See this Implmentation example section for some examples.

Check out this thread to see how those migration steps came to be:
http://sourceforge.net/p/openemr/discussion/202506/thread/566ef9c0

By not pushing the panic button here and systematically approaching the security issue(s), we can both harden the security on multiple levels while also not breaking things for users. The project has a surplus of time, but very minimal resources (ie. it can wait for you and others to set up git branches and contribute to the codebase :slight_smile: ).

-brady
OpenEMR

bradymiller wrote on Thursday, April 11, 2013:

Hi Robert,

Recommend not directing folks to the oemr.org wiki, but instead to the open-emr.org wiki for documentation since the oemr.org wiki is not maintained and is outdated (for example, note the installation instructions wiki page you posted has a dead link to the Security page at the end of the instruction set, which is really important for users to see after an installation). Note that the open-emr.org wiki page for those instructions includes links that work to those vital pages.

-brady
OpenEMR

visolveemr wrote on Wednesday, April 17, 2013:

Hi Brady,

Just thought of sharing - Apart from OpenEMR app security vulnerabilities, we need to identify security vulnerabilities with the third party applications integrated in OpenEMR like phpMyAdmin, Smarty PostCalendar, phpGACL, etc and seems we’re using the older versions.

There are chances that these third-party applications are also security vulnerable, where in latest versions that might have been fixed.

For instance, Currently OpenEMR is using phpMyAdmin v 2.11.10 and the latest release is 3.5.8 with 4.0 beta release. v 2.11.10 is vulnerable to cross-site scripting and the same is fixed in the immediate release (v 2.11.10.1). Even the Smarty PostCalendar is of old version 4.0 where the latest releases is 8.0. Upgrading these third-party applications to their latest stable release may address the known security vulnerabilities.

Hope this helps.

Thanks
Devi

bradymiller wrote on Thursday, April 18, 2013:

Hi Devi,

Agreed (especially considering that OpenEMR’s worst vulnerability to date was brought in by the third party Open Flash Chart code) and just added another section to the ongoing Security project wiki page:
http://www.open-emr.org/wiki/index.php/Codebase_Security#Secure_the_Embedded_Third_Party_Elements

-brady
OpenEMR

visolveemr wrote on Monday, April 29, 2013:

Hello Brady,

Attached is the Security vulnerability report from ZAP tool for a specific set of configuration and currently we are testing with another set of configuration aiming to cover most of the code.

We would like to contribute towards fixing. Before we start, we would like to know - does any other team working on the security vulnerability fixes.

We have plans to upgrade phpMyadmin application to the latest version and perform QA after integration. Please do share your views

Thanks
Devi

yehster wrote on Monday, April 29, 2013:

I am still working to address the deficiencies with password verification/hashing that have been discussed before (mostly as part of the Ad Hoc meeting process/emails).

bradymiller wrote on Tuesday, April 30, 2013:

Hi Devi,

I am a bit clueless as to what the attachment is reporting/doing, but upgrading phpMyadmin sounds like a great idea. Kevin is the only one (see his message below) that is currently actively working on security items.

-brady
OpenEMR

bradymiller wrote on Tuesday, April 30, 2013:

Hi,

Haven’t look at the code in too much detail, but a quick read over is rather exciting. Is it ready for testing?

-brady
OpenEMR

visolveemr wrote on Thursday, May 02, 2013:

Hi Brady,

The attached report is the output from the ZAP cross-site scripting tool. The Security test was performed using the Active Scan method in ZAP v2.1 tool which attempts to find the vulnerabilities which are listed in OWASP top 10 (Ref https://www.owasp.org/index.php/Category:OWASP_Top_Ten_Project ) of selected target such as SQL Injection, Cross Site Scripting, Private IP disclosure…

Though that report has low-warnings, it is suggested to go for fixing the same.

Recently we have generated few more security report and it has some ‘high’ vulnerability warnings.

N.B Please suggest whether to post the security report in the forum or shall i mail to interested developers. Please do share your views.

Thanks
Devi

visolveemr wrote on Monday, May 06, 2013:

Hi Brady,

Just waiting for your views.

Since it is not advisable to share application vulnerability report in forum i will share the same to your mail. Please do share with other interested developers

Thanks
Devi

bradymiller wrote on Monday, May 06, 2013:

Hi Devi,

Recommend uploading the reports to the wiki and making a section on the wiki page and placing links to your reports there:
http://www.open-emr.org/wiki/index.php/Codebase_Security#Assessment
(Place your Analysis section in the Assessment section above the Sans institute analysis)

Producing these reports with numerous entries isn’t very helpful, though, unless plan to analyze the results. For example, what do all those entries in the previous report (like “X-Frame-Options header not set”) even represent? My honest interpretation of most of those categories is that I have no idea what they mean.

-brady
OpenEMR

yehster wrote on Thursday, May 09, 2013:

Fixing one of the security warnings as the report describes will break the “top.sessionRestore()” mechanism OpenEMR uses to manage sessions.

A cookie has been set without the HttpOnly flag, which means that the cookie can be accessed by JavaScript. If a malicious script can be run on this page then the cookie will be accessible and can be transmitted to another site. If this is a session cookie then session hijacking may be possible.

If the session cookie is set to HttpOnly as the tool suggests, then the javascript that switches sessions won’t work any more.

visolveemr wrote on Thursday, May 09, 2013:

Kevin,

Thanks for your views. Before we start fixing, we’ll be analyzing the suggestion in the report. We will also validate the report generated using other tools (list shared in our earlier post). As brady suggested, I’m in the process of consolidating the vulnerabilities based on the Severity. We will be focusing on the High severity bugs immediately and then with Medium/low, if needed.

Thanks
Devi ViSolve

yehster wrote on Thursday, May 09, 2013:

As far as I can tell, the issues identified by ZAP and other cross-site scripting issues are things that an outsider would use to trick a valid user into revealing information (such as their password) to that outsider.

Where as SQL Injection is something that a valid OpenEMR user can exploit to see and do things that they shouldn’t be allowed to.

Because of this, I am of the opinion that SQL injection issues are generally bigger risks than cross site scripting issues.

That said, it might just take a few extra lines of in globals.php or some other appropriate common SINGLE location to set the “X-Frame-Options header” correctly for a majority of the issues listed in the ZAP report.

visolveemr wrote on Tuesday, May 21, 2013:

Hi Team,

Attached is the SQL injection vulnerabilities report. Attached has 2 sheets - 1. Consolidated Report 2. Failure report and as a next step we need to focus on fixing the failures.

Kevin/Brady - I’ll consolidate the ZAP report for failures with high severity and will be sharing that soon.

Thanks
ViSolve

yehster wrote on Tuesday, May 21, 2013:

There is at least one false negative in that SQL injection report.
As a demonstration it says that “edit_facility” is ok.

After logging in an attack like this is possible because the fid isn’t sanitized.

http://emr100/openemr/interface/usergroup/facility_admin.php?fid=4’ union select ‘1’,‘2’,‘3’,‘4’,‘5’,‘6’,‘7’,‘8’,‘9’,‘10’,‘11’,‘12’,‘13’,‘14’,‘15’,‘16’,‘17’,‘18’,‘19’,‘20’,‘21’,‘22’,'23

As an attacker, I could put something after the after the union select that will modify data I shouldn’t be able to.

Given that “SQL Inject Me” missed a very obvious injection vulnerability, I have to question the validity of the results.