Scalar gets copied when moved revealing the secret value.

[RajeshRk18]

Scalar holds array of elements that implement Copy 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 the Box 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.

[burdges]

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.

[RajeshRk18]

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.

[tarcieri]

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 the Drop trait. If the type needs something special to happen when the value goes out of scope and we add the Copy 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.

[RajeshRk18]

Thanks for Clarification. More question;

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.

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-drop elliptic_curve::SecretKey type which is a wrapper for a Scalar 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.

[tarcieri]

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: The-DevX-Initiative/RCIG_Coordination_Repo#55

[RajeshRk18]

Note elliptic_curve::SecretKey has a drop impl. Your example does not, and you are calling Clone 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!

[tarcieri]

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]

@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.

[tarcieri]

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.