[crypto] zeroize bls12381 secrets #733
Conversation
punwai
changed the title
| @@ -403,7 +404,7 @@ impl KeyPair for BLS12381KeyPair { | |||
| } | |||
|
|
|||
| fn private(self) -> Self::PrivKey { | |||
| self.secret | |||
| BLS12381PrivateKey::from_bytes(self.secret.as_ref()).unwrap() | |||
There was a problem hiding this comment.
required, because we implemented drop for KeyPair.
There was a problem hiding this comment.
Left a few indications, but this globally looks v. good.
This is the important one: #733 (comment)
crypto/src/tests/bls12381_tests.rs
Outdated
| unsafe { | ||
| for i in 0..BLS12381PrivateKey::LENGTH { | ||
| assert!(*ptr.add(i) == 0); | ||
| } | ||
| } | ||
|
|
||
| // Check that self.bytes is zeroized | ||
| unsafe { | ||
| let mut vec = Vec::new(); | ||
| for i in 0..BLS12381PrivateKey::LENGTH { | ||
| vec.push(*bytes_ptr.add(i)); | ||
| } | ||
| assert_ne!(&vec[..], &sk_bytes[..]); | ||
| } | ||
| } |
There was a problem hiding this comment.
YMMV, but something like
let memory: &[u8] = unsafe { ::std::slice::from_raw_parts(ptr, BLS12381PrivateKey::LENGTH) } may just give you something easier to inspect, no?
There was a problem hiding this comment.
Thanks for this! I think it is much cleaner - I haven't touched unsafe rust much so I just didn't know about this.
crypto/src/tests/bls12381_tests.rs
Outdated
| let kp = keys().pop().unwrap(); | ||
| let mut sk = kp.private(); |
There was a problem hiding this comment.
Are we sure keys delivers some non-zero bytes in the first place for sk? Are we sure that invariant will remain the same forever if somebody modifies how keys() works?
There was a problem hiding this comment.
I've added a deterministic key generation within the test itself.
crypto/src/tests/bls12381_tests.rs
Outdated
| let kp = keys().pop().unwrap(); | ||
| let sk = kp.private(); | ||
| sk_bytes.extend_from_slice(sk.as_ref()); | ||
|
|
||
| ptr = std::ptr::addr_of!(sk.privkey) as *const u8; | ||
| bytes_ptr = &sk.as_ref()[0] as *const u8; | ||
| } |
There was a problem hiding this comment.
I for one am OK testing just zeroize on drop, and inferring that if it works on drop, it works everywhere.
But I won't fight against the inclusion of one more test, of course.
There was a problem hiding this comment.
I think that makes sense, drop calls zeroize itself so it makes no sense to test manually calling zeroize.
There was a problem hiding this comment.
I've changed the PR to only zeroize on drop
* [crypto] implement zeroize on bls12381 secrets
* [crypto] implement zeroize on bls12381 secrets
Currently, deleted secret keys are kept in memory after they are dropped. This could be a problem as it could potentially later be recovered by an attacker. This PR implements zeroize on all BLS12381 secrets (KP and privkey), and includes tests that ensure that the secret key is deleted once secrets are dropped in the program.