Preliminary version of password security changes. https://github.com/yehster/openemr/tree/security_consolidated
The basic premise is anywhere client-side SHA1 hashing of passwords was previously done has been replaced with RSA encryption of the password.
In addition, simple SHA1 hashing previously in place has been replaced with blowfish + salt. Now stored in new table "users_secure". With the intention of implementing a mechanism where a database user with higher privileges will be used to handle user/password operations in the near future. This approach should mitigate some risk of SQL-Injection issues.
An administrator creating a user or changing an existing user's password requires re-confirmation of the administrator's password.
A user changing his own password also must confirm his present password.
The RSA encryption and blowfish strategies are being applied in the ON-site portal as well. Offsite portal is unchanged.
SSL is still highly recommended, but what happens is whenever a password needs to be transmitted, the client requests a one-time-use public-key from the server. Server stores the corresponding private key in a new table rsa_pairs. When the client sends the data, the server retrieves the private-key from the DB and uses it to decrypt the sensitive information from the client. The private-key is then deleted from the table after it's used.
Assuming there are no major bugs, migration of the data in the tables themselves does not require any effort. The first time a user logs in the SHA1 hash is replaced with blowfish/salt automatically.
There is a new global flag 'password_compatibility' that can be toggled after all user's password have been updated, that prevents any "stray use" of SHA1. (The compatibility/migration routines are always skipped and only authentication by blowfish will work.
The main point is to address potential "pass-the-hash" and rainbow table vulnerabilities.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Kevin,
Agree with Tony. Looks like OpenEMR's password/hashing mechanism is about to jump into the future. Will also plan to test/review it over the next couple days.
-brady OpenEMR
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thanks for your feedback guys.
Brady,
The biggest "open question" is what to do about compatibility with PHP 5.2.
I don't think we ought to expend too many resources worrying about users who are using 5.2. They should probably upgrade at this point, given that the last release was about 3.5 years ago and the lack of maintenance from PHP folks means they probably have other significant security issues to worry about.
Also the crypt manual says this:
crypt() will return a hashed string using the standard Unix DES-based algorithm or alternative algorithms that may be available on the system.
...
As of PHP 5.3.0, PHP contains its own implementation and will use that if the system lacks of support for one or more of the algorithms.
So, I think a 5.2 user may actually have blowfish available.
One of the cool things about crypt is that the algorithm used is determined by the prefix in the salt. So changing to a different algorithm is just a matter of changing the prefix. (So if something better comes along in the future it won't be too hard for us to change...) That said, I think I'm going to rename function blowfish_salt() to password_salt(). Better to describe the "purpose" of the routine and not the "implementation".
With regard to the random number generation when building the salt. It's not really that critical that salts be "perfectly random" they just need to be different for each user and each password change. The point is to "increase the cost" of brute force attacks. Put another way, the "uniqueness" of a salt is what is valuable, not the "entropy".
I still have a fair amount of additional testing I want to do, but I think the code itself is in reasonable shape at this point.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
As a practical matter many clinics will be running older Linux distributions. It's easy to say they should upgrade, but for the non-tech-savvy that's a huge undertaking. If we can show them some mercy without much effort, we should. This is why it's long standing OpenEMR policy to avoid forcing a PHP upgrade without a somewhat good reason.
I used the dev virtual appliance to test on 5.2.
Instead of using crypt directly, I defined a new function password_hash() which chooses either crypt or SHA1 depending on the CRYPT_BLOWFISH flag indicating whether or not blowfish is available on the system. 5.2 systems will still get additional security from salted passwords, but not from blowfish.
There is also a mechanism to optionally setup a "privileged DB user" specifically for access to the users_secure password table as a partial mitigation for SQL Injection in the rest of the code base.
Last edit: Kevin Yeh 2013-05-22
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Brady,
Thanks for the review.
Testing this round included PHP 5.2 using the dev appliance. Did both a clean 4.1.2 install and an upgrade from 4.1.1 to 4.1.2.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Preliminary version of password security changes.
https://github.com/yehster/openemr/tree/security_consolidated
The basic premise is anywhere client-side SHA1 hashing of passwords was previously done has been replaced with RSA encryption of the password.
In addition, simple SHA1 hashing previously in place has been replaced with blowfish + salt. Now stored in new table "users_secure". With the intention of implementing a mechanism where a database user with higher privileges will be used to handle user/password operations in the near future. This approach should mitigate some risk of SQL-Injection issues.
An administrator creating a user or changing an existing user's password requires re-confirmation of the administrator's password.
A user changing his own password also must confirm his present password.
The RSA encryption and blowfish strategies are being applied in the ON-site portal as well. Offsite portal is unchanged.
SSL is still highly recommended, but what happens is whenever a password needs to be transmitted, the client requests a one-time-use public-key from the server. Server stores the corresponding private key in a new table rsa_pairs. When the client sends the data, the server retrieves the private-key from the DB and uses it to decrypt the sensitive information from the client. The private-key is then deleted from the table after it's used.
Assuming there are no major bugs, migration of the data in the tables themselves does not require any effort. The first time a user logs in the SHA1 hash is replaced with blowfish/salt automatically.
There is a new global flag 'password_compatibility' that can be toggled after all user's password have been updated, that prevents any "stray use" of SHA1. (The compatibility/migration routines are always skipped and only authentication by blowfish will work.
The main point is to address potential "pass-the-hash" and rainbow table vulnerabilities.
Awesome sounding. Will pull and do some testing this weekend.
--Tony
Hi Kevin,
Agree with Tony. Looks like OpenEMR's password/hashing mechanism is about to jump into the future. Will also plan to test/review it over the next couple days.
-brady
OpenEMR
Thanks for your feedback guys.
Brady,
The biggest "open question" is what to do about compatibility with PHP 5.2.
I don't think we ought to expend too many resources worrying about users who are using 5.2. They should probably upgrade at this point, given that the last release was about 3.5 years ago and the lack of maintenance from PHP folks means they probably have other significant security issues to worry about.
Also the crypt manual says this:
One of the cool things about crypt is that the algorithm used is determined by the prefix in the salt. So changing to a different algorithm is just a matter of changing the prefix. (So if something better comes along in the future it won't be too hard for us to change...) That said, I think I'm going to rename function blowfish_salt() to password_salt(). Better to describe the "purpose" of the routine and not the "implementation".
With regard to the random number generation when building the salt. It's not really that critical that salts be "perfectly random" they just need to be different for each user and each password change. The point is to "increase the cost" of brute force attacks. Put another way, the "uniqueness" of a salt is what is valuable, not the "entropy".
I still have a fair amount of additional testing I want to do, but I think the code itself is in reasonable shape at this point.
As a practical matter many clinics will be running older Linux distributions. It's easy to say they should upgrade, but for the non-tech-savvy that's a huge undertaking. If we can show them some mercy without much effort, we should. This is why it's long standing OpenEMR policy to avoid forcing a PHP upgrade without a somewhat good reason.
Rod
http://www.sunsetsystems.com/
Yeah, renaming the function to "password_salt" and testing the CRYPT_XXXX flags should be easy enough for compatibility with older systems.
The thing is I don't really want to setup an instance of 5.2 to confirm that it works...
Regarding php 5.2, there's an appliance waiting right here :)
http://www.open-emr.org/wiki/index.php/OpenEMR_Developer_Virtual_Appliance
Just download it, start it up and watch the php 5.2 magic...
This is what makes the Development Demo:
http://www.open-emr.org/wiki/index.php/Development_Demo
-brady
OpenEMR
Branch with updates
https://github.com/yehster/openemr/commits/security_updates_consolidated
Link that doesn't include whitespace changes
https://github.com/yehster/openemr/commit/d541c5dec26d1521cc45e16f9a14461b8e267e43?w=0
I used the dev virtual appliance to test on 5.2.
Instead of using crypt directly, I defined a new function password_hash() which chooses either crypt or SHA1 depending on the CRYPT_BLOWFISH flag indicating whether or not blowfish is available on the system. 5.2 systems will still get additional security from salted passwords, but not from blowfish.
There is also a mechanism to optionally setup a "privileged DB user" specifically for access to the users_secure password table as a partial mitigation for SQL Injection in the rest of the code base.
Last edit: Kevin Yeh 2013-05-22
https://github.com/yehster/openemr/commit/b8cf0146b518d22ad45d36d90d8f706bdc68195b
The sample scripts in the previous commit for privDB accidentally contained tables which are part of my non-standard stuff.
Hi Kevin,
Tested it a bit and placed a review on github. This is getting very close to being commitable to sourceforge.
-brady
OpenEMR
https://github.com/yehster/openemr/commit/53d214ea4f8a737ff9efa72a6b094fc8bda2639b
Brady,
Thanks for the review.
Testing this round included PHP 5.2 using the dev appliance. Did both a clean 4.1.2 install and an upgrade from 4.1.1 to 4.1.2.
This code has been pushed into the official repository.