-
Notifications
You must be signed in to change notification settings - Fork 53
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
Expose the LMDB Encrypt/Decrypt feature #134
base: main
Are you sure you want to change the base?
Conversation
41bd7c7
to
13a78ff
Compare
bb35c93
to
b75b672
Compare
I see multiple issues with the current PR, mostly coming from the LMDB API. TLDR: LMDB should probably expose an AEAD API instead of the current "handmade" API. UnsoundnessUnwinding across FFI is UB, so this is unsound. The callback should be wrapped in a catch_unwind. AEADInstead of rolling your own traits I'd recommend leveraging the existing RustCrypto ecosystem, especially the aead crate. What you'd probably be looking for are the AeadMutInPlace (using their AEAD stands for Authenticated Encryption with Associated Data, which is the standard API for authenticated data. It is much better to not separate the encryption from the authentication (checksum), as it leaves less room for error and also can improve performance. For crates implementing AEADs look at (X)ChaCh20Poly1305, AES-GCM and AES-GCM-SIV. The exampleThe example is somewhat broken. CRC codes are not cryptographically secure. Even with the encryption, it may be possible for an attacker to modify the data so that the checksum passes and modify it subtly without it being noticeable. AEADs prevent that kind of issue :) Limits of LMDBThe API proposed by LMDB separates the encryption from the authentication(checksum), which is bad as it prevents efficient usage of standard, misuse-resistant AEAD constructs. LMDB also performs the checksum before the actual encryption, which is very dangerous. Also, just looking at the code I wasn't able to find how the nonce (iv) is generated. If the same iv is used twice with the same key, it leads to the plaintext being recoverable and sometimes even the key itself. I'm worried that if the nonce is generated randomly, it could lead to nonce reuse, since the nonces of standard AEADs are rather small (96bits), and you reach 50% chances of a collision after It appears that the functionality is not yet stabilized (still a release candidate) so I'd recommend contacting them to change the encryption API to be that of an AEAD:
The associated data could be whatever metadata LMDB needs but cannot encrypt. The AEAD ensures that is hasn't been tampered. If needed a checksum could also be used, but it should be separate from the encryption (even maybe disallowed to be used at the same time) and used only to detect accidental data corruption. |
Thank you very much @sgued for this detailed explanation, I just pushed a commit to catch the possible panics of the checksum and encryption functions. Unfortunately, there is no way to tell LMDB that the checksum function failed. Should I abort the program instead of doing nothing? About the example that is wrong, as I am using a non-cryptographic valid checksum method, what do you recommend? Is there anything that can be done without implementing an AEAD interface that checksums by itself? I will try to ping @hyc here, and if he doesn't respond I will probably contact him via Twitter. So Howard, if you read me, I am trying to expose a safe LMDB wrapper for Rust users. As I am not a cryptographer, I asked the community for help with how I have designed this wrapper so far. @sgued answered just above. Could you take a look at this, if you have time, please? What do you think about exposing an API closer to an AEAD interface? |
With respect, @sgued's review is incorrect. LMDB's API is fully compliant with AEAD APIs and the code already provides an example implementation thereof. https://github.com/LMDB/lmdb/blob/mdb.master3/libraries/liblmdb/crypto.c The LMDB checksum API is only for use when you only want a checksum and no encryption. |
Oh, sorry, I was mislead by the documentation:
That doesn't leave any place for outputting the authentication tag, and given that Looking at your example it appears that |
Thank you very much Howard, for your intervention. It is much appreciated 🔥 Thank you @sgued, for helping me. Could you help me understand and design a good interface on top of LMDB? Can I ask the user to give me an According to Howard's analysis, I conclude that I should remove the comment I added about preferring encryption with checksum and interpret the checksum error as a wrong password key. Instead, we should be able to use a valid AEAD algorithm that also checksums cryptographically correctly and returns an appropriate error. Am I right? |
By the way, I would encourage you to copy the interface used in liblmdb/crypto.c and encapsulate it as a dynamic object, the way that sample does. That way the LMDB commandline tools can load your crypto mechanism and operate on your encrypted databases still. |
You don't need to pass user data. The use aead::{AeadMutInPlace, Key, KeyInit, Nonce, Tag};
fn encrypt<A: AeadMutInPlace + KeyInit>(
key: &[u8],
nonce: &[u8],
aad: &[u8],
plaintext: &[u8],
chipertext_out: &mut [u8],
auth_out: &mut [u8],
) {
chipertext_out.copy_from_slice(plaintext);
let key: &Key<A> = key.try_into().unwrap();
let nonce: &Nonce<A> = nonce.try_into().unwrap();
let mut aead = A::new(key);
let tag = aead
.encrypt_in_place_detached(nonce, aad, chipertext_out)
.unwrap();
auth_out.copy_from_slice(&tag);
}
fn decrypt<A: AeadMutInPlace + KeyInit>(
key: &[u8],
nonce: &[u8],
aad: &[u8],
chipher_text: &[u8],
output: &mut [u8],
auth_in: &[u8],
) -> bool {
output.copy_from_slice(chipher_text);
let key: &Key<A> = key.try_into().unwrap();
let nonce: &Nonce<A> = nonce.try_into().unwrap();
let tag: &Tag<A> = auth_in.try_into().unwrap();
let mut aead = A::new(key);
aead.decrypt_in_place_detached(nonce, aad, output, tag)
.is_ok()
}
fn with_chacha() {
use aead::{generic_array::typenum::Unsigned, AeadCore, KeySizeUser};
use chacha20poly1305::ChaCha20Poly1305;
let mut pl = [1; 1024];
let aad = [];
let mut ciphertext = [0; 1024];
// AeadCore and KeySizeUser are required for all T: AeadMutInPlace + KeyInnit, so you can use them to get the size of the key/tag/nonce.
let nonce = [0; <ChaCha20Poly1305 as AeadCore>::NonceSize::USIZE];
let mut auth = [0; <ChaCha20Poly1305 as AeadCore>::TagSize::USIZE];
let key = [2; <ChaCha20Poly1305 as KeySizeUser>::KeySize::USIZE];
encrypt::<ChaCha20Poly1305>(&key, &nonce, &aad, &pl, &mut ciphertext, &mut auth);
pl = [0; 1024];
assert!(decrypt::<ChaCha20Poly1305>(&key, &nonce, &aad, &ciphertext, &mut pl, &auth));
} |
I tried working with the Could someone help me with that subject? You can reproduce the issue by |
7bf57bf
to
248ed54
Compare
Ok, so I was able to encrypt/decrypt an LMDB database with @hyc and @sgued, Can I ask you a technical question about how I should design an app on top of LMDB? I am designing a small app that stores files in an encrypted LMDB; this app will likely be encrypted with What is your advice about that? |
Again, just study the example in liblmdb/crypto.c. Just truncate. |
XChaCha20 is used to protect against nonce misuse in systems that use randomly generated nonces (i.e., to protect against weak RNGs). There is no need to use that in LMDB since LMDB nonces aren't random and are guaranteed to be unique.
What you're asking about here is known as a Key Derivation Function, KDF. https://en.wikipedia.org/wiki/Key_derivation_function Popular choices are pbkdf or argon2. Again, for LMDB, you should expose whatever you use in your module as the string2key function. |
b9d1c21
to
75bdf1f
Compare
Call for TestingHey Rustaceans, I did a lot of work to provide the "recently" released support for page-level encryption of LMDB in heed. It is exposed as an
I would be glad if you help me improve this crate. Note that this PR is part of a massive rewrite of the crate to use the latest version of LMDB. You'll notice many undocumented breaking changes compared to the other versions of heed. |
I must cherry-pick these 18 commits on top of #128 instead of rebasing... |
7d7dac4
to
df71fed
Compare
6efe3b8
to
341bcb6
Compare
341bcb6
to
c800c0a
Compare
This branch has just been rebased on the latest main commit 3a20c96. |
There is an open report claiming that AEAD is not working. I've not seen a reproducer for it. https://bugs.openldap.org/show_bug.cgi?id=9920 If your testing turns up any problems in LMDB please followup there. |
Call for Advice
As a reminder, heed is a wrapper on top of LMDB. LMDB is a memory-mapped key-value store battle-tested for a long time. This branch introduces a wrapper on top of the "recently" released support for page-level encryption. It has been released in the latest version of LMDB, and heed has been updated in #128. This PR will be merged in it after the reviews.
I would like help from people who know how to use encryption algorithms. Maximum feedback on the new encryption and checksum API would be appreciated. As I am not very proficient in cryptography, would it be possible to help me with these subjects:
Encrypt::encrypt_decrypt
trait method?auth_size
parameter of theEnvOpenOptions::encrypt_with
method?encrypt_with
method to set a checksum too, and to interpret a checksum error as a decryption error. Is it correct to do so?Checksum
andEncrypt
traits when we don't need them in theEnvOpenOptions
instead of having dummy types? (Would be so good when the never type is released).Note that I already have successfully used this new API and exposed a simple example that uses the
ChaCha20
crate. I would like to know if this example is valid, please.You'll probably need the documentation of the API of LMDB. Unfortunately, the only one that is available only is outdated. I advise you to install and run Doxygen and generate the doc from
lmdb-master3-sys/lmdb/libraries/liblmdb
(don't forget togit submodule init --update
when you clone this repo), or you can directly read the raw documentation from there too.Where Should I Start?
To help me, you can simply create a new Rust crate, add this line to your
dependencies
and inspire yourself by looking at the simple example I linked above ☝️:Once you've found improvements, please comment directly in this PR ❤️ 👨💻
What should I do next?
EnvOpenOptions::encrypt_with
method accept aKey
of the right length and let the user provide it by using its own KDF and salt. Heed must not know about the KDF used.DummyEncrypt
traits impls elsewhere e.g., end of the file, another module, to avoid polluting this part of the code.XChaCha20 needs a bigger nonce than the one provided but there is no need in using it with LMDB anyway.
DummyEncrypt
type and rename itNoopEncrypt
to miror the Support customized key compare function #212 changes.