dalek-cryptography / x25519-dalek

X25519 elliptic curve Diffie-Hellman key exchange in pure-Rust, using curve25519-dalek.
BSD 3-Clause "New" or "Revised" License
329 stars 133 forks source link

Ensure that all data of StaticSecret is cleared on drop #41

Closed untoldwind closed 5 years ago

untoldwind commented 5 years ago

The clone inside clamp_scalar seems to to have no use and might lead to a situation where the StaticSecret remains in memory after dropping it since the original byte-array is not cleared.

hdevalence commented 5 years ago

The modified version of the code is certainly cleaner, but I don't think it makes any difference regarding memory clearing: in either case, the array is kept only on the stack, and we don't explicitly wipe the stack, so while this prevents a redundant copy, it doesn't affect memory wiping.

hdevalence commented 5 years ago

Merged, thanks for submitting this patch!

untoldwind commented 5 years ago

Actually (and unluckily) you are right. I've taken a look at generate llvm code of both versions and while they are marginally different they both have have an @llvm.memcpy.p0i8.p0i8.i64 of the original array to somewhere further up the stack. And on top of that the Scalar::from_bits is doing another @llvm.memcpy.p0i8.p0i8.i64 (though from the previously memcpyed version down the stack)

I know this is kind of academic (since the stack is overwritten all the time), but I think the only way to ensure that there are no remains anywhere-ever would be to either