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 all crypto libraries with Botan #6209

Merged
merged 2 commits into from
Apr 6, 2021
Merged

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Feb 28, 2021

Selected the Botan crypto library due to feature list, maintainer support, availabilty across all deployment platforms, and ease of use. Also evaluated Crypto++ as a viable candidate, but the additional features of Botan (PKCS#11, TPM, etc) won out.

The random number generator received a backend upgrade. Botan prefers hardware-based RNG's and will provide one if available. This is transparent to KeePassXC and a significant improvement over gcrypt.

Replaced Argon2 library with built-in Botan implementation that supports i, d, and id. This requires Botan 2.11.0 or higher. Also simplified the parameter test across KDF's.

Aligned SymmetricCipher parameters with available modes. All encrypt and decrypt operations are done in-place instead of returning new objects. This allows use of secure vectors in the future with no additional overhead.

Took this opportunity to decouple KeeShare from SSH Agent. Removed leftover code from OpenSSHKey and consolidated the SSH Agent code into the same directory. Removed bcrypt and blowfish inserts since they are provided by Botan.

Additionally simplified KeeShare settings interface by removing raw certificate byte data from the user interface. KeeShare will be further refactored in a future PR.

NOTE: This PR breaks backwards compatibility with KeeShare certificates due to different RSA key storage with Botan. As a result, new "own" certificates will need to be generated and trust re-established.

Removed YKChallengeResponseKeyCLI in favor of just using the original implementation with signal/slots.

Removed TestRandom stub since it was just faking random numbers and not actually using the backend. TestRandomGenerator now uses the actual RNG.

Greatly simplified Secret Service plugin's use of crypto functions with Botan.


This refactor directly supports the move the MSVC on Windows. It also supports the inclusion of PKCS#11 hardware tokens for database credentials and use of TPM for in-memory encryption.

Special thanks to @randombit for his work on Botan!


Testing strategy

Tested on all platforms. Xenial and Bionic require use of PPA to bring Botan 2.11 or higher. This version requirement is mainly driven by the Argon2 support.

Type of change

  • ✅ Refactor (significant modification to existing code)

@droidmonkey droidmonkey added high priority 🚨 pr: refactoring Pull request that refactors code labels Feb 28, 2021
@droidmonkey droidmonkey added this to the v2.7.0 milestone Feb 28, 2021
@droidmonkey droidmonkey force-pushed the refactor/botan branch 2 times, most recently from 3929675 to ba4b4ea Compare March 2, 2021 01:57
@hifi
Copy link
Member

hifi commented Mar 3, 2021

I've been using this for a few days with KDBX4+AES-256+AES-KDF, no issues yet. SSH Agent can also handle unencrypted ED25519 and RSA keys correctly in daily use and my personal test database that decrypts and loads a bunch of different keys seems to work as well.

All non-GUI tests have passed on both x86_64 and AArch64 without any issues.

@yan12125
Copy link
Contributor

yan12125 commented Mar 5, 2021

This also works for ChaCha20+Argon2id and encrypted Ed25519 SSH keys. Fabulous work!

Looks like botan is not installed on CI machines yet?

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monster PR, but nice net code reduction.

CMakeLists.txt Outdated Show resolved Hide resolved
src/browser/BrowserAction.cpp Show resolved Hide resolved
src/crypto/Crypto.cpp Outdated Show resolved Hide resolved
src/crypto/Crypto.cpp Outdated Show resolved Hide resolved
src/crypto/Crypto.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
tests/TestSharing.h Show resolved Hide resolved
tests/TestSignature.cpp Show resolved Hide resolved
tests/TestSignature.cpp Show resolved Hide resolved
@ofer-dev
Copy link

ofer-dev commented Mar 6, 2021

Does this affect key derivation time? My understanding is that Botan currently do not provide simd(sse, avx/2/512) acceleration for argon2 while libargon2 does.

@randombit
Copy link

As best I can tell SSE2/AVX2 does not actually help Argon2 performance that much, probably because most of the cost in in memory access. Comparing argon2 command line util vs Botan, Argon2i, 3 passes, 1 thread

64M libargon2 163 ms Botan 134 ms
256M libargon2 670 ms Botan 560 ms
1024M libargon2 2783 ms Botan 2323 ms

This is on an i7-6700K running Linux.

Possibly I'm comparing the wrong libargon2? These numbers are with https://github.com/P-H-C/phc-winner-argon2

@hifi hifi mentioned this pull request Mar 7, 2021
@droidmonkey
Copy link
Member Author

droidmonkey commented Mar 7, 2021

Possibly I'm comparing the wrong libargon2? These numbers are with P-H-C/phc-winner-argon2

This was the previous implementation of Argon2 that we were using and Botan has replaced in this PR. At the end of the day I would hope that Argon2 cannot be optimized, otherwise it would be a terrible time-hard KDF. The whole point of Argon2 is to be slow everywhere and incur cost in CPU and Memory.

@phoerious
Copy link
Member

I guess the point is that you want your own Argon2 implementation to be as fast as possible so as to be able to perform as many rounds as possible, because an attacker most certainly will use the most optimised version out there. I really wouldn't use KeePass2's C# implementation for exactly that reason, which is orders of magnitude slower than what we have. Though I doubt that the difference between Botan and libargon2 is really significant, but I didn't benchmark it.

@droidmonkey
Copy link
Member Author

Right, I meant hardware optimization in my comment above.

@phoerious
Copy link
Member

It will be possible to some extent, but it shouldn't give you a 10x boost.

@droidmonkey
Copy link
Member Author

@phoerious ready for re-review. Left some comments unresolved for your acceptance.

@droidmonkey droidmonkey requested a review from phoerious April 4, 2021 14:43
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is good to go once CI is green, but you didn't unindent the namespaces?

@droidmonkey
Copy link
Member Author

I can't unindent them, at least not locally, every time I hit format they re-indent. Do we need to update our format file?

@droidmonkey droidmonkey force-pushed the refactor/botan branch 2 times, most recently from ecce5bc to b0b006b Compare April 5, 2021 18:39
Selected the [Botan crypto library](https://github.com/randombit/botan) due to its feature list, maintainer support, availability across all deployment platforms, and ease of use. Also evaluated Crypto++ as a viable candidate, but the additional features of Botan (PKCS#11, TPM, etc) won out.

The random number generator received a backend upgrade. Botan prefers hardware-based RNG's and will provide one if available. This is transparent to KeePassXC and a significant improvement over gcrypt.

Replaced Argon2 library with built-in Botan implementation that supports i, d, and id. This requires Botan 2.11.0 or higher. Also simplified the parameter test across KDF's.

Aligned SymmetricCipher parameters with available modes. All encrypt and decrypt operations are done in-place instead of returning new objects. This allows use of secure vectors in the future with no additional overhead.

Took this opportunity to decouple KeeShare from SSH Agent. Removed leftover code from OpenSSHKey and consolidated the SSH Agent code into the same directory. Removed bcrypt and blowfish inserts since they are provided by Botan.

Additionally simplified KeeShare settings interface by removing raw certificate byte data from the user interface. KeeShare will be further refactored in a future PR.

NOTE: This PR breaks backwards compatibility with KeeShare certificates due to different RSA key storage with Botan. As a result, new "own" certificates will need to be generated and trust re-established.

Removed YKChallengeResponseKeyCLI in favor of just using the original implementation with signal/slots.

Removed TestRandom stub since it was just faking random numbers and not actually using the backend. TestRandomGenerator now uses the actual RNG.

Greatly simplified Secret Service plugin's use of crypto functions with Botan.
@droidmonkey droidmonkey merged commit ed0ece3 into develop Apr 6, 2021
@droidmonkey droidmonkey deleted the refactor/botan branch April 6, 2021 02:56
@droidmonkey
Copy link
Member Author

We are now a Botan family!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority 🚨 pr: refactoring Pull request that refactors code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants