Closed paragonie-scott closed 4 years ago
Hey Scott:
I talked to Buster this morning about some of this stuff + other securing things
Sounds like a perfect issue for him to implement later on :)
I CC’d him on my response to this comment and will talk with him later about
On Feb 5, 2019, at 11:22 AM, Scott notifications@github.com wrote:
This is related to a lot of existing issues (e.g. #2181 and #2024) and some recent pull requests (#2043, #2047). Implementing this suggestion may solve a lot of cryptography problems for OpenEMR (and the EMR industry in general, which should do a lot to help protect patients from data breaches).
Background Information
In #2043, @bradymiller said:
btw, am looking forward to future when OpenEMR only support PHP 7.2+, because we can then convert over to libsodium for this stuff.
If I understand correctly, this means that OpenEMR needs to be able to support OpenSSL and Libsodium at different stages in its lifetime. This can be dangerous, since cryptography weaknesses usually occur at the joinery of protocols rather than within the protocols themselves.
The good news is, Paragon has developed a library that fits neatly into OpenEMR's needs, called CipherSweet:
It supports OpenSSL (using only FIPS 140-2 approved cryptographic algorithms). It supports libsodium (PHP 7.2 or sodium_compat). It allows seamless migrations between different backends and/or keys. Additionally, it offers a lot of other benefits:
It abstracts key management so that you can integrate with a cloud provider's KMS solution. It provides searchable encryption using a technique called blind indexing. (For this point, the security experts you trust will probably be mostly interested in this page of the documentation.) It's schema-agnostic. Learn more:
Building Searchable Encrypted Databases with PHP and SQL CipherSweet: Searchable Encryption Doesn't Have to be Bitter Infosec Stack Exchance: How secure is the Ciphersweet library for searchable encryption, and why is a duplicate entry leak not a problem? Source Code and Documentation on Github Proposal
I believe all of the operational concerns with any symmetric encryption feature for OpenEMR are easily solved by CipherSweet, with a small amount of integration (glue) code.
Any features that require asymmetric cryptography can be solved using a combination of CipherSweet and EasyECC.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
@paragonie-scott ,
Proposal looks great. Now that OpenEMR's encryption/decryption is centralized, bumping up to a more solid library wouldn't be too tough (although would be best to only take it on if it fixed or improved something). Centralizing the encrypt/decrypt functions in OpenEMR was tough because needed to support different decryption functions depending on what originally encrypted it (ie. needed to maintain backward compatibility).
Related to issues, got 2 questions:
While centralizing stuff in OpenEMR, I had to maintain support for a wonky feature that allows user to download a encrypted document using only a passphrase, which can then be uploaded to another OpenEMR instance and decrypted with just the passphrase. Does Ciphersweet support that kind of use case. Would be very interested in standard practice on something like that. What i ended up doing was using a hash of password for both the encryption and hmac key: https://github.com/openemr/openemr/blob/master/library/crypto.php#L100-L102
OpenEMR stores sensitive data in both the database and on drive. Does it make sense to ensure that the key is stored on the opposite source? (I am planning on doing this; now only encrypting stuff in database which uses keys from drive, but also plan to encrypt things on the drive which will use keys from database).
thanks! -brady
This would all be an optional feature, right? I do a lot of ad hoc queries for clients....
hi @sunsetsystems ,
Note the only things we encrypt in the database at the openemr level are:
If we desire full database encryption then could just rely on that at the mysql/mariadb layer (then your ad hoc queries would work)
And the plans to encrypt stuff on the drive (patient documents and other stuff stored in documents) would definitely be a optional feature.
Related to issues, got 2 questions:
- While centralizing stuff in OpenEMR, I had to maintain support for a wonky feature that allows user to download a encrypted document using only a passphrase, which can then be uploaded to another OpenEMR instance and decrypted with just the passphrase. Does Ciphersweet support that kind of use case. Would be very interested in standard practice on something like that. What i ended up doing was using a hash of password for both the encryption and hmac key: https://github.com/openemr/openemr/blob/master/library/crypto.php#L100-L102
You can define a password-derived key provider, like so: https://gist.github.com/paragonie-scott/3d06442e39d27e9295b2826ac84860f3
- OpenEMR stores sensitive data in both the database and on drive. Does it make sense to ensure that the key is stored on the opposite source? (I am planning on doing this; now only encrypting stuff in database which uses keys from drive, but also plan to encrypt things on the drive which will use keys from database).
Yes, that's a sane approach.
@sunsetsystems
This would all be an optional feature, right? I do a lot of ad hoc queries for clients....
Correct. This would certainly be optional.
If something changed in the future that warranted encrypting everything by default for all OpenEMR installs, it would almost certainly land in a new v6 or v7 release because backwards compatibility would break hard. I suspect such a hypothetical change would be heavily discussed for a long time before it landed.
Great, thanks guys!
hi @paragonie-scott , Super interesting stuff in the PasswordDerivedKeyProvider.php class. Learned about use of "slow" hash and the hash_pbkdf2() function. In my use case a salt would be very difficult to deal with; since the user encrypts file with passphrase on one openemr instance and then can take it to another one to decrypt with just the passphrase. Is seems like not having a salt is a bad idea, but can't figure out how to work in a salt. Is there a way to deal with that? thanks, -brady
Is seems like not having a salt is a bad idea, but can't figure out how to work in a salt. Is there a way to deal with that?
The salt is meant for multi-user systems to ensure the same password results in a different password hash.
If you only have a single "user", you can use the same salt for all keys. It's generally considered a bad practice, but without the risk mentioned above, there's no real downside. Just make sure the hard-coded value is distinct per instance, not hard-coded in OpenEMR itself.
By doing so, although you'll be using the salt feature, it will cease to be a proper salt. It's downgraded to a domain separation constant.
hi @paragonie-scott , This is what makes the use case wonky, though. There is no user or domain separation. The passphrase needs to be able to decrypt it on any openemr instance for any user. Perhaps there is no secure workable solution for this use case? -brady
The simplest solution is to store the salt with the encrypted file.
- header || nonce || ciphertext || tag
+ header || pbkdf2salt || nonce || ciphertext || tag
So far CipherSweet has only been concerned with database fields, but if you wanted to have an EncryptedFile
API that implemented an encryptWithPassword()
and decryptWithPassword()
API, that's doable.
@bradymiller https://github.com/paragonie/ciphersweet/pull/36 exposes the API I proposed above.
hi @paragonie-scott ,
Would something like this work for the salts: https://github.com/bradymiller/openemr/commit/db103c168327299faede9fcb744636c4dfe77e7e
Note sure if overkill 2 have a different salt for the cypher key and hmac.
Your library does look nice. After the 3 use cases(use key on drive, use key in database, user passphrase) are all hammered out in current library, which will be done here (https://github.com/openemr/openemr/pull/2211), then will look into possibly integrating ciphersweet.
Would something like this work for the salts: bradymiller@db103c1
That's a bit overkill. I'd suggest doing something like this instead:
You'll cut your performance overhead by 50% that way and get the same security.
After the 3 use cases(use key on drive, use key in database, user passphrase) are all hammered out in current library, which will be done here (#2211), then will look into possibly integrating ciphersweet.
Great, glad to hear it. @danehrlich1 said he might ask Buster to work on implementing it in the future. We've previously spoken about cryptography implementations in another open source project, so if you're comfortable with Buster taking point on that, I already know he's a joy to work with. :)
hi @paragonie-scott , Thanks for the guidance. I think I got it using HKDF-HMAC-SHA2 (or I hope I got it at least :) ): https://github.com/openemr/openemr/pull/2211/commits/e1cc090364b40327f23a56e7a3fa1994f417cde4
CipherSweet v1.9.0 has been released which includes the password-based file encryption/decryption.
Worth noting: our design mitigates race conditions (the sort of attack that might be exploitable by cloud file providers if they conspired against their users; e.g. Dropbox).
@bradymiller Your HKDF-HMAC-SHA2 usage is correct.
This issue has been automatically marked as stale because it has not had any recent activity within the past 90 days. It will be closed in 7 days if no further activity occurs.
This issue has been automatically closed because it has not had any recent activity within the past 97 days.
This is related to a lot of existing issues (e.g. #2181 and #2024) and some recent pull requests (#2043, #2047). Implementing this suggestion may solve a lot of cryptography problems for OpenEMR (and the EMR industry in general, which should do a lot to help protect patients from data breaches).
Background Information
In #2043, @bradymiller said:
If I understand correctly, this means that OpenEMR needs to be able to support OpenSSL and Libsodium at different stages in its lifetime. This can be dangerous, since cryptography weaknesses usually occur at the joinery of protocols rather than within the protocols themselves.
The good news is, Paragon has developed a library that fits neatly into OpenEMR's needs, called CipherSweet:
Additionally, it offers a lot of other benefits:
Learn more:
Proposal
I believe all of the operational concerns with any symmetric encryption feature for OpenEMR are easily solved by CipherSweet, with a small amount of integration (glue) code.
Any features that require asymmetric cryptography can be solved using a combination of CipherSweet and EasyECC.