-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add feature = "zeroize" #92
base: master
Are you sure you want to change the base?
Conversation
At first glance, there are two problems I see here:
I think a better approach would be to hook the actual allocator. You could do this today by setting a
I'm no cryptographer, but it doesn't seem very useful to address just this one small part of security. Reading data from old allocations in Rust means that something must have gone terribly wrong in the borrow checker or more likely unsafe code. On the other hand, timing issues are an every day fact of life that Rust does nothing to prevent, so this seems like a much more important concern. |
Excellent review of For the crypto side, a couple of points:
I believe a better approach would be to write an allocation-free version of |
Oh dear. I've already thought of more things to add. There are mathematical solutions to timing issues which can be implemented on top of the non-constant-time library as it currently is, e.g. the RSA blinding implementation. The notes on
I'm done for real this time. Short summary: I believe this usefully increases safety, despite the downsides, some of which can be improved (in other libraries). |
@cuviper Now that Zeroize does zero out the full capacity, would you accept a PR for this? IMHO it's better to do best effort than nothing, and currently a cryptographer using this library can't make best effort here without invoking UB |
@elichai Would it be sufficient to you if we simply implemented |
@cuviper IMHO that's already better than nothing, and we can push experimentations/thoughts about how to handle temporaries to the future, maybe the future allocator api could be involved here. (It is also an interesting idea to take a performance critical application, replace the global allocator with one that zeroize everything and test the performance) |
For now adding zeroize is the simplest solution. |
FWIW here is the https://github.com/dignifiedquire/num-bigint/blob/c64439f/src/biguint.rs#L136-L141 I also just opened a PR which improves the impl on |
Add a feature which causes us to run
zeroize()
onDrop
. It is disabled by default.zeroize tries to ensure that the memory inside the object is zeroed on
Drop
, not just left lying around on the heap. This may improve security in certain types of application, whereBigInt
orBigUint
is being used for cryptography. It does not address any timing issues.It is useful to have this inside this crate. Even if the backing storage was exposed (which it is not), the crate uses temporary allocation all over the place internally, so you would want to wipe all of those instances, too. I checked many places it appears that the crate is using temporary vectors, and, in all cases, it appears that these are moved into a
Big(U)int
, so would be wiped onDrop
.I do not believe the feature should be enabled by default, it necessarily and intentionally adds extra performance overhead which many users would not care about.
The code change is not entirely trivial, because adding a
Drop
implementation to an existingstruct
stops you from moving out of thestruct
's fields (I learned, today!).In one case, this can be fixed by inlining a variable, with no semantic changes.
In two others, I have added
#[cfg(
flags to use an existing "copying" implementation when inzeroize
mode. These two are probably avoidable, with larger code changes.