Implement Zeroize

[data-retriever]

I don't know how feasible it is, but as there are a lot of secrets here, it would make sense.

[apoelstra]

Moving to rust-secp. I'm surprised that no such issue already exists.

The TL;DR is that these zeroing crates (there are a couple, IIRC the other popular one is called subtle) can't make very strong guarantees at all, because Rust's model results in a ton of memcpys of types like this. So we'd probably get little benefit from this, and the costs are

• An extra dependency, with associated review burden and MSRV difficulty
• Prevents impl'ing Copy on SecretKey which is a pretty big ergonomics hit

Having said this, I would like to support this somehow, even if the library doesn't have first-class support for it. I don't know either whether this is feasible.

[Kixunil]

I believe this is a lost cause. The compiler may copy secret bytes to unexpected locations as a result of optimizations and there's no way to prevent that. Also the only thing this protects against is leakage due to UB. Rust has much less UB than C, so the value is questionable and the significant effort required to actually make this possible would be better spent at preventing (the remaining) UB in the first place.

[sanket1729]

https://docs.rs/zeroize/latest/zeroize/#what-guarantees-does-this-crate-provide is promising.

[apoelstra]

@sanket1729 but the immediately following section says that copies and moves undermine the crate and that you need to use Pin to prevent this. And it is basically impossible to use Pin in this library because we produce objects we don't own (after creation) and can't box due to nostd.

[sanket1729]

@apoelstra thanks for pointing that out. I am with @Kixunil now, this is a very hard problem to solve and our resources are better invested elsewhere.

[kayabaNerve]

subtle is about being constant time, not about zeroing memory.

zeroize is about traits to zeroize memory. Zeroize does not require removing the copy syntax which is a massive UX hit, as I've tried in the past. The nice thing though, beyond the ability to derive Zeroize for higher level objects since all members implement it, is the fact you can then use Zeroizing. Zeroizing wraps any type into a non-copy instance which will be zeroized on drop and IMO, is the preferable way to handle secret keys (so basically, you leave the choice to users).

Accordingly, I don't believe this is fruitless regarding functionality. I also don't believe implementing Zeroize would affect being no_std/Pin/whatever at all. It'd just be a function which accesses the underlying C struct and writes zeros (though, with syntax which makes it so it won't be optimized out. Ideally, you just call zeroize's <&mut [u8]>::zeroize).

Not to say that magically gives you time or makes it a priority. I just wanted to to highlight the benefits/impacts over it.

[Kixunil]

@kayabaNerve thanks for explaining, that sounds at least doable (even though I still find value questionable).

[kayabaNerve]

Yeah, memory access will always be a question of "If someone already has that much access, isn't it moot?" My preference is to minimize copies, and do what I can, even if sure, the second I perform multiplication, the underlying lib probably makes a few copies (hopefully on the stack, at least). The stack ones will hopefully get cleared out soon, and I can know my long lived ones aren't just available for the next program which calls malloc.

[Kixunil]

aren't just available for the next program which calls malloc

They already aren't. The kernel zeroizes allocated pages. It'd be a huge vulnerability if it didn't.

[kayabaNerve]

Some systems do not offer that behavior, per malloc's memory being undefined (justifying calloc), nor do I believe Linux is complete in that behavior. Beyond apparently being configurable when compiling the kernel, both Linux and Windows seem to only zero pages before handing off to a distinct process. While I did say, "next program" (which may still be feasible on some embedded systems), there are other concerns where a process both manages a secret key and exposes some level of memory buffer access to users (though Rust thankfully prevents most of these).

[Kixunil]

justifying calloc

That's something completely different. The process gets zeroed memory from the kernel but it doesn't zero it's own which means that if you allocate, write something and then deallocate the next allocation may obtain the pointer containing what you just wrote (or may not). calloc is for zeroing those.

a process both manages a secret key and exposes some level of memory buffer access to users (though Rust thankfully prevents most of these).

Rust protecting against this is exactly my point. It's no longer that big issue (as an interesting data point, according to a recent report, Google found no vulnerabilities in their Rust code so far). If we focus our time on getting unsafe right we can get much more security than chasing secrets all over memory.

[elichai]

Moving to rust-secp. I'm surprised that no such issue already exists.

It just exists as PRs :) : #102, #165, #262, #311

[benma]

Prevents impl'ing Copy on SecretKey which is a pretty big ergonomics hit

One could argue the opposite and Copy makes it unergonomical to prevent copies that will not be erased.

In any case, the functions on SecretKey that take mut self as an argument make it particularly difficult, for example:

    /// Negates the secret key.
    #[inline]
    #[must_use = "you forgot to use the negated secret key"]
    pub fn negate(mut self) -> SecretKey {
        unsafe {
            let res = ffi::secp256k1_ec_seckey_negate(
                ffi::secp256k1_context_no_precomp,
                self.as_mut_c_ptr(),
            );
            debug_assert_eq!(res, 1);
        }
        self
    }

If my type was already a Zeroize<SecretKey>, calling .negate() on it creates a copy that will not be automatically erased. Changing this function to pub fn negate(&mut self) { would help with that.

Same for mul_tweak() and maybe others.

I believe this is a lost cause.

Even if one can't have 100% guarantees with Rust, one can go a long way with using Zeroize<Box<..>>, so moves of the value will copy the pointer and not the value. We may not want wrap SecretKey in a Box, but at least the functions on SecretKey should allow in-place modification so that a copy is not forced (see above).

@apoelstra would it be possible to change negate(), mul_tweak() etc. to work on &mut self instead, or add functions next to the existing ones that do?

[Kixunil]

Copy makes it unergonomical to prevent copies that will not be erased.

It doesn't because you can't prevent them. Every move is a memcpy (unless optimized-out), there's no guarantee your secret data won't be copied somewhere and not erased and there cannot be. Copy is a bad name for what really should be !Drop.

This is a lost cause, just make your software secure by using other techniques.

Zeroize<Box<..>>, so moves of the value will copy the pointer and not the value.

That's an illusion since compiler can decide to optimize-out the Box or leave the data in registers or wherever else it wants. Even Pin is not immune to this. Also initializing the Box without accidentally spilling the secrets into memory is not trivial and it involves either unsafe or hoping that the compiler will optimize it correctly or taking performance hit (but then also is your RNG avoiding spilling secret data?)

would it be possible to change negate(), mul_tweak() etc. to work on &mut self instead, or add functions next to the existing ones that do?

We already had such API and it led to horrible code where a meaning of a variable suddenly changed without changing it's name. At best I would be sympathethic to take &self for secret keys so people don't accidentally make the intermediate copy but that also shouldn't matter much since if you're relying on the optimizer to avoid moves then this is not needed and if you're not relying on it you're trying to do the impossible.

[benma]

That's an illusion since compiler can decide to optimize-out the Box

I am aware there is no 100% guarantee, but Zeroize<Box<...>> goes a long way in practice when used properly. Box::new() is going to allocate on the heap. I was under the impression this was guaranteed, but even if not, I have checked the output often in the past and not seen an instance of it being optimized away yet.

https://docs.rs/zeroize/latest/zeroize/ contains a good summary. Stack values are problematic, heap values less so (when not re-allocating).

Even if there is no guarantee, it is better to try to avoid copies than not to try or even do the opposite: making it hard not to leave copies, which seckey.negate() or seckey.mul_tweak() currently do. A &mut self version of these would make it possible to leave fewer (or even no) copies in practice.

At best I would be sympathethic to take &self for secret keys so people don't accidentally make the intermediate copy

If it returns self that does not help much. The only meaningful improvements here would be to have &mut self versions, or to wrap the secret byte array in a Box. The latter is probably too intrusive. The first would be a non-intrusive change, so why not?

[Kixunil]

https://docs.rs/zeroize/latest/zeroize/ contains a good summary. Stack values are problematic, heap values less so

I don't see such claim in the linked doc and it makes no sense why it should be the case. It's just a piece of memory either way and it has no effect on how the compiler generates the code that accesses it.

A &mut self version of these would make it possible to leave fewer (or even no) copies in practice.

At the cost of worse readability. There are no solutions, just tradeoffs.

If it returns self that does not help much.

It does if you instantly wrap it in some zeroize thing or a Box - that's how you had to initialize it in the first place! IOW if this is broken then your code is already broken anyway.

The first would be a non-intrusive change, so why not?

I've already explained this.

[benma]

I suggest adding &mut self versions, not replacing the old ones, maybe there was a confusion there.

Afaik Box::new(value) might copy the value from stack to heap, and Zeroize::new(value) would also copy value and leave the old one. I often use Zeroize::new(vec![0; 32]) to allocate first, then modify in-place as a workaround.

I suppose I could still try do manually erase all the copies I have some control over, but that is difficult to get right. &mut self versions would make it a lot easier. &self would be slightly better than status quo, as it at least removes one copy that cannot be erased.

What is your general advise here when using SecretKey? Do nothing and accept that copies of secret keys could remain? I might have been too optimistic about it being possible to erase all copies, but when I spent time in the past checking the compiled output it seemed to work quite okay most of the time :shrug:

[apoelstra]

I'd be fine adding a negate_assign method, and so on for the other key-modifying functions. I think the _assign prefix is long and annoying enough to discourage people from using it, since I agree with Kix that it's unergonomic.

I tend to be more optimistic than Kix that Box<SecretKey> will actually avoid copies in practice, and that even if not, something is better than nothing. (I don't believe that people will somehow code more recklessly due to a false sense of security. This is transparent defense in depth.) Also even if Box<SecretKey> is no good, maybe it will be possible for some users to use secure OS memory or even CPU support, and they will also be aided in their newtyping by the &mut methods.

What is your general advise here when using SecretKey? Do nothing and accept that copies of secret keys could remain?

In short, yes.

[Kixunil]

Afaik Box::new(value) might copy the value from stack to heap

Correct.

I often use Zeroize::new(vec![0; 32]) to allocate first, then modify in-place as a workaround.

If you really want a box this should be more efficient but it's relying on optimizer doing RVO which is currently not guaranteed even though likely. (There is a proposal to guarantee RVO which would help you a lot.)

let boxed = Box::new_uninit();
let boxed = Box::write(boxed, construct_secret());
let secret = Zeroize::new(boxed);

This needs nightly but the API was approved for stabilization.

However given how much effort you and other people put into arguing about zeroization and making the crate I find it really weird that zeroize doesn't have a similar API:

impl<'a, T: Zeroize> Zeroizing<StackBox<'a, T>> {
    fn in_place(place: &'a mut MaybeUninit<T>, value: T) -> Self { /* ... */ }
}

It'd be far better if you added that API to the library than ask every crate that might hold secrets to change its. If you find RVO to be unreliable that may be something for Rust/LLVM devs to work on but even then we should rather support an API that allows one to express the operation more cleanly. Something like:

fn negate_in_place<'a>(&self, place: &mut 'a MaybeUninit<Self>) -> &'a mut Self { /* ... */ }

What is your general advise here when using SecretKey?

First you need to realize that zeroizing in Rust has completely different cost/benefit than in C++ where it was probably popularized. Both because Rust is memory-safe (lower chance of bugs in the first place, thus lower benefit) and because Rust doesn't have move constructors (more difficult to implement - higher cost). So I'd say consider investing in other areas - e.g. proving your unsafe code sound.

If you still want to do it, try relying on RVO as I demonstrated and check the assembly.