Spike: Zeroize Senstive/Secret Information.

[gatoWololo]

Task and Previous Work

We only want sensitive/secret data in memory as long as needed. The memory allocator and Rust free values when they are deallocated, but these values remain intact (non-zeroized) in memory until something else overwrites them.

We need to do a pass through our code ensuring that sensitive/secret data is zeroed out when it is done being used. We plan to use the Zeroize crate to this this.

We have already done some work on this #342. However, we have identified various issues in this approach which can pose a security concern.

Existing Issues

Adding #[derive(ZeroizeOnDrop)] to all our types is not a full solution. (See this blog for a deeper dive: A pitfall of Rust's move/copy/drop semantics and zeroing data). In summary these are various technical issue our solution must handle:

• Re-allocation: Data structures like Vec may be reallocated by the system memory allocator if the Vec grow past its capacity. Even with ZeroizeOnDrop reallocation may leave the original data in memory.
• Stack copies. As explained in the blog, stack-allocated (local) values may be copied when they are moved (e.g. a local value is returned from a function). This will cause the old copy to NOT be zeroized. See Stack/Heap Zeroing Notes for more information.
• Ergonomics: If a type implements Drop then we cannot move out any field of that type. This is painful ergonomics wise as sometimes we want to simply move a field of a struct for use. This currently happens in our crypto code (See Paper Cuts working with generic::Secret. #476 (May be slightly outdated)). Currently we end up unnecessarily cloning these values to get around this restriction, this is a bad solution as we are unnecessarily making copies. Which is bad from both a performance (minor) and security perspective.

Solution

There is no simple programming solution that solves this problem. Instead, my proposed solution is a combination of code + best practices:

• Re-allocation: To avoid re-allocation we should avoid senstive/secret data stored directly in vectors. Vectors are quite versatile and combine many useful features into a single data structure, among them: a dynamically growable array. The main observation here is that we do not use vectors for their dynamic growth capability much (if at all). So instead of using Vec directly, we can create a wrapper around Vec with a subset API, specifically, one that doesn't expose methods that dynamic grow the Vec. (Alternatively we could consider using a constant-sized data structure e.g. Box<[T; N]> or some other data structure.
• Stack copies: This one is quite tricky, it is subtle how variables are allocated and stored on the stack. Data may be copied onto the stack when local variable are passed into or returned from functions. The Zeroize crate documentation recommends using Pin to ensure stack values are not moved but even this is tricky and does not seem to always work. The solution is to heap-allocate all these values, to ensure only one copy exists in memory.
• Ergonomics: Instead of adding ZeroizeOnDrop to all types that hold secret data, we should take a bottom-up approach. We define our core secret/sensitive primitive types. These types are our base types which implement ZeroizeOnDrop. Composite types (structs) that use these types should not be annotated with ZeroizeOnDrop. This will allow us to work with these types (move them) as needed.

Additionally, I recommend using the Secrecy to wrap our secret/sensitive data. This makes it explicit at the type-level that you are working with secret data. This prevent accidental uses and copies of the secret data, as a user has to call fn expose_secret(&self) -> &S to get the underlying data. (This also solves an ergonomic issue where we have refrained from implementing e.g. Clone on secret data, to avoid accidental cloning of secrets. However, later we are forced implement Clone on the type to make the lifetimes work.)
From some secret type S, Secrecy would allow us to have the underlying S implement whichever traits we need, but users would work with secrecy::Secret<S> which does not implement these traits. An explicit expose_secret function call is necessary to call .clone() on the underlying type. Making it obvious you are accessing secret data. Februrary 8, 2023 update: I no longer think we should use this crate. I think the zeroize crate + some good programming practices would be good enough.

TODOs

• Based on the information above (and any other) create a set of "best practices" that engineers must follow when implementing new secret data or working with secret data.
• Identify current sensitive/secret data in our code.
• Ensure all sensitive/secret data is treated as described above./

[gatoWololo]

@marsella Please review this and see if it matches your understanding and if I'm missing anything.

[marsella]

This looks good to me! Thanks for the write-up.