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

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

Merged
merged 1 commit into from Apr 23, 2019

Conversation

untoldwind
Copy link
Contributor

@untoldwind untoldwind commented Apr 12, 2019

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 hdevalence changed the base branch from master to develop April 23, 2019 17:06
@hdevalence
Copy link
Contributor

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 hdevalence merged commit 1cc37a1 into dalek-cryptography:develop Apr 23, 2019
@hdevalence
Copy link
Contributor

Merged, thanks for submitting this patch!

@untoldwind
Copy link
Contributor Author

untoldwind commented May 3, 2019

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

  • replace the [u8;32]-parameters with &mut [u8;32], doing the memcpy manually (e.g. with
    copy_from_slice or so) and then zero out the original (https://crates.io/crates/secrets is doing it this way)
    or
  • Wrap [u8;32] in a struct with a clear-on-drop just like the StaticSecret itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants