Open RajeshRk18 opened 9 months ago
I doubt Box makes sense, given this crates gets used without alloc.
I suppose documentation could suggest that secrets be Zeroize+Drop
and thus not Copy
, but even so it's still sensible & convenient to provide type aliases like ed25519_dalek::SecretKey
.
That was my suggestion to the best of my knowledge. Curious to know if this problem can be solved without alloc :)
Box is one of the ways if you want the value unmoved (at the cost of small runtime performance).
Yeah, separate type for Secret Key should be a good option (with wrappers like Box).
Does Zeroize + Drop
imply not Copy
? You can zeroize and drop, and still not zero out the original memory.
As has already been covered, in curve25519-dalek
the alloc
feature and liballoc are optional as this crate supports "heapless" embedded targets which keep everything on the stack. There are a lot of contexts where such a feature is potentially valuable including things like bootloaders, OS kernels, or other embedded programs that might want to verify a signature long before the heap is available.
Does Zeroize + Drop imply not Copy?
As a general rule in Rust, types which impl Drop
are !Copy
:
https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html#stack-only-data-copy
Rust won’t let us annotate a type with
Copy
if the type, or any of its parts, has implemented theDrop
trait. If the type needs something special to happen when the value goes out of scope and we add theCopy
annotation to that type, we’ll get a compile-time error.
It's important to note that not all Scalar
values are secret, and Scalar
currently MUST be a Copy
type due to bounds restrictions on subtle::ConditionallySelectable
intended to make writing constant-time code easier because it ensures conditional selection only makes memcpy-style copies.
FWIW the @RustCrypto elliptic-curve
crate opts for a sort of approach that @burdges is suggesting: it provides a zero-on-drop elliptic_curve::SecretKey
type which is a wrapper for a Scalar
and also implements all functionality related to serialization/deserialization of secret keys.
Also note that in Rust leaving transient copies of secrets on the stack is very difficult to avoid. Moves can sometimes make memcpy-style copies of !Copy
types. Stack spilling may copy values from a reference to a cache located on the stack. This is all rustc behavior we have very little control over.
To ensure the stack is clean after running a cryptographic operation, the caller needs to handle clearing out sensitive data from the stack.
Thanks for Clarification. More question;
It's important to note that not all
Scalar
values are secret, andScalar
currently MUST be aCopy
type due to bounds restrictions onsubtle::ConditionallySelectable
intended to make writing constant-time code easier because it ensures conditional selection only makes memcpy-style copies.
Yeah, I am talking about Scalar being used to hold sensitive data. What is the point of having constant time code if an attacker can inject a program into the victim's machine, inspect memory, and extract the secret keys (copies that are in stack)?
FWIW the @RustCrypto
elliptic-curve
crate opts for a sort of approach that @burdges is suggesting: it provides a zero-on-dropelliptic_curve::SecretKey
type which is a wrapper for aScalar
and also implements all functionality related to serialization/deserialization of secret keys.
I tried to emulate SecretKey here. It does not solve the problem. Please correct if I am wrong.
To ensure the stack is clean after running a cryptographic operation, the caller needs to handle clearing out sensitive data from the stack.
How can the caller be trusted with manually handling the sensitive data? They will often mess things up. Maybe a clear documentation as to how will be the way.
An attacker who can read secret keys out of memory is generally outside the threat model of most cryptographic libraries.
Note elliptic_curve::SecretKey
has a drop impl. Your example does not, and you are calling Clone
that will make copies. But as I already mentioned: moves will make copies. To prevent a type from being moved you need to use the pin!
macro to pin it in place.
How can the caller be trusted with manually handling the sensitive data? They will often mess things up. Maybe a clear documentation as to how will be the way.
This is an area where there aren't great solutions right now, and why I currently prefer that libraries don't attempt to include some half-baked solution. So I'm sorry but I don't have a good answer for you.
You can read some past discussion on this subject here: https://github.com/The-DevX-Initiative/RCIG_Coordination_Repo/issues/55
Note
elliptic_curve::SecretKey
has a drop impl. Your example does not, and you are callingClone
that will make copies.
Here is the updated one.
But as I already mentioned: moves will make copies. To prevent a type from being moved you need to use the
pin!
macro to pin it in place.
Yeah, I saw Zeroize docs also mentioning about using Pin
. I will look into it. Thanks!
You can read some past discussion on this subject here: The-DevX-Initiative/RCIG_Coordination_Repo#55
Will give it a read. Thank you sir!
Here is the updated one.
@RajeshRk18 is there something specific you're trying to illustrate with these examples?
This example contains moves, because you are allocating SigingKey
on the stack inside of a block, and then returning it from the block.
As I've said repeatedly, and you've just quoted, moving a type can involve a memcpy. So showing an example of some convoluted code that allocates a key and then moves it around leaving copies on the stack is entirely expected.
But even if it didn't, and you had code that didn't do anything explicitly that might leave a copy on the stack, rustc might choose to make a copy, e.g. to improve cache locality.
There is pretty much nothing you can do in pure Rust code to avoid things like that. It's a fraught endeavor.
@RajeshRk18 is there something specific you're trying to illustrate with these examples?
You told here that wrapping is a move towards solving the problem(not entirely). So, I tried to show that this doesn't solve the problem at all. Maybe I've misunderstood what you were intending to say. My bad.
That message concludes:
Also note that in Rust leaving transient copies of secrets on the stack is very difficult to avoid. Moves can sometimes make memcpy-style copies of
!Copy
types. Stack spilling may copy values from a reference to a cache located on the stack. This is all rustc behavior we have very little control over.To ensure the stack is clean after running a cryptographic operation, the caller needs to handle clearing out sensitive data from the stack.
Scalar
holds array of elements that implementCopy
trait. Thus, array gets copied when moved which reveals the value.I have reproduced the issue here: https://gist.github.com/RajeshRk18/eb10e3506c83c196d69116e86e0910e5
I have made
Scalar
field to public to reproduce this issue.Impact:
Whenever an user does operations with its private key, there is a high chance that the private key gets revealed.
Recommendation:
Wrap the byte array with
Box
as cloning theBox
is cheap and now byte array won't be moved. Let the library user decide which Scalar type (Boxed/Unboxed) he will use according to the context.