Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace unsafe ed25519 from security-critical functions with cryptography #59

Closed
wants to merge 2 commits into from

Conversation

py0xc3
Copy link

@py0xc3 py0xc3 commented May 3, 2020

I removed the unsafe ed25519 implementation from critical functions: the unsafe is still in use when only public information is involved (e.g. verification) as this is not a security problem but still very slow (the unsafe should be completely removed on the long term). PyCA's cryptography is used when a private key gets involved to ensure security. https://github.com/pyca/cryptography is well maintained. So, critical security issues are solved. Still, some things remain to be done: the unsafe ed25519 (see hyperledger#55) implementation should be completely removed to fully depend on cryptography from PyCA. The latter is faster and remains maintained if imported. The unsafe Ed25519 is also from PyCA (which marked it to be not safe and for testing use only) but was copied to the iroha-python repo, assuming it to be no longer maintained in there. Also, I suggest to remove Ed25519-related functions from IrohaCrypto class in order to use the cryptography module's classes directly. That's faster and easier to maintain on the long term, avoiding many unnecessary functions, serializations, ... in between.

Further, I have removed os.urandom, which can output cryptographic-level entropy but does not implement blocking. So, if not sufficient entropy is available, os.urandom would not be aware of it. This could be a problem in virtual machine use. Ed25519 private key creation uses entropy from OpenSSL (will be imported with cryotography using low-level bindings).

Therefore, it will be necessary to adjust the wheel file for pip, too: it needs to include cryptography as dependency.

I have tested all changed functions/classes on Fedora 32, including tests against the Iroha-Python usage example (https://github.com/hyperledger/iroha-python/blob/master/README.md) which runs all changed functions (private_key, derive_public_key, _signatur (the latter is covered indirectly through running sign_transaction)).

Still, to be further reviewed to ensure all-encompassing compatibility!

Unused module/variable from #58 solved

py0xc3 added 2 commits May 3, 2020 03:04
I removed the unsafe ed25519 implementation from critical functions: it it still in use when only public information is involved (e.g. verification) but this is not a security problem but it is still very slow. But PyCA's cryptography is used when a private key gets involved. https://github.com/pyca/cryptography is well maintained. So, critical security issues are solved. Still, some things remain to be done: the unsafe ed25519 (see hyperledger-iroha#55) implementation should be completely removed to fully depend on cryptography from PyCA. The latter is faster and remains maintained if imported. The unsafe Ed25519 is also from PyCA (which marked it to be not safe and for testing use only) but was copied to the iroha-python repo, assuming it to be no longer maintained in there. Also, I suggest to remove Ed25519-related functions from IrohaCrypto class in order to use the cryptography module's classes directly. That's faster and easier to maintain on the long term, avoiding many unnecessary functions, serializations, ... in between.
Unused module/variable from hyperledger-iroha#58 (2 lgtm-com alerts) solved
@appetrosyan
Copy link
Contributor

Hi! Please add a signed-off-by line to the end of your commits, as per the instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants