Make sure secret keys are always inside a `ClearOnDrop`. about hbbft HOT 8 CLOSED

Just a thought: There is an MIT-licensed secrets crate that does clear on drop:

impl<T> Drop for Sec<T> {
    fn drop(&mut self) {
        if !thread::panicking() {
            debug_assert_eq!(0,              self.refs.get());
            debug_assert_eq!(Prot::NoAccess, self.prot.get());
        }

        unsafe { sodium::free(self.ptr) }
    }
}

It promises a lot more:

Buffers allocated through this library:

• restrict themselves from being read from and written to by default
• allow access to their contents in explicit, limited scopes
• are never included in core dumps
• are never swapped to permanent storage (using mlock)
• are protected from overflows and underflows by inaccessible guard pages (using mprotect)
• are protected from underflows by a random canary
• immediately zero out the contents of the memory used to initialize them
• immediately zero out the contents of their allocated memory when they leave scope

It might be worth examining that crate. Maybe we could use it, fix some bugs, send fixes upstream? If the design does not hold up though, at least going over this list and ensuring everything that's listed there is true for our implementation can't hurt, I guess.

The only caveat I see is a dependency on NaCl, which has an excellent reputation but it still something we would require at link time.

from hbbft.

Reopening issue for lack of mlock. The solution should have the attributes:

• Must not result in an insecure copy/move of a secret.
• The portion of memory where the secret is allocated must not be swapped to disk.
• The portion of memory where the secret is allocated must be cleared on drop.
• The clearing of data must be elision proof.
• Must be implemented for the hbbft types: SecretKey, SecretKeySet/Poly, BivarPoly (Fr and Vec<Fr>).
• Must not require the user to bring in non-Rust dependencies.
• Must not leak secret values through printing (implement custom Debug).

from hbbft.

Also, it doesn't seem to have the restriction that T needs to implement Default, so we should be able to switch to:

pub struct SecretKey(Secret(Fr))

We should ideally do the same for SecretKeySet, but that currently has a Polynomial as its only field. One solution would be to use SecretVec inside Poly and BivarPoly. (Although it feels a bit wrong to introduce secret into the innocent poly module that doesn't even know it has anything to do with cryptography. 😁 )

from hbbft.

I don't think that the secrets crate takes the same precautions to circumvent the rustc optimization of eliding zeroing out data if that memory won't be used again; clear_on_drop takes special care in this regard. However, secrets does add a few extra security features like making sure secret values are not included in core dumps and the addition of stack canaries.

Also, the dalek cryptography crate designers chose to use clear_on_drop over secrets, I'm not sure why exactly, I'll have to read up on it. Here is an issue where the dalek authors discuss the problem of clearing secrets from memory.

What do you (@afck, @mbr) think about adding the extra libsodium dependency? Apt and HomeBrew both have packages, but it's one more dependency to add considering we we just got rid of the dependency on protoc.

From what I have read, I think that secrets has a cleaner API for our use case.

from hbbft.

unsafe { sodium::free(self.ptr) }

After thinking about it, I don't think that the compiler is smart enough to elide a call to a C function. So elision of the memory clearing code may not be a problem with secrets.

from hbbft.

Yes, I imagine that won't be optimized away, but I agree that the sodium dependency is annoying. Maybe secrets could be modified to depend on one of the sodium Rust crates instead, that vendor the C library?

Anyway, I don't have a strong opinion regarding clear_on_drop vs. secrets in general.

from hbbft.

I did not understand yet why we are not using an "established" crate that protects memory, I assume it was due to the libsodium dependency? Or was it just not generic enough? I would think that disabling swap is fairly important for keys.

There's also the tradeoff between implementing Debug and otherwise preventing accidental leakage. I did try to address this with another crate a while ago: https://github.com/49nord/sec-rs , it is probably more of a nice to have.

@DrPeterVanNostrand: If you think these are valid concerns, feel free to reopen the issue. Sorry!

from hbbft.

I agree Marc, At some point we should use something like https://github.com/quininer/seckey.

from hbbft.