zeroize: add (back) `ZeroizeOnDrop` trait

[tarcieri]

The zeroize crate used to have a ZeroizeOnDrop trait: iqlusioninc/crates#168

It was removed in favor of #[zeroize(drop)] which largely turned out to be a mistake: iqlusioninc/crates#188

Originally filed by @fjarri as iqlusioninc/crates#757:

Person A creates a crate with a type that has some secret data inside, say, SecretKey { inner: SecretDataType }. They implement Zeroize and Drop (or #[zeroize(drop)]) for SecretDataType, and stop at that - the secret data is now safe (to the extend Rust can guarantee it), the drop() of SecretKey will call the zeroizing drop() of SecretDataType, so there's no need to define anything for SecretKey itself.

Person B uses A's crate and wants to use Secret<SecretKey>. But they cannot, because SecretKey does not implement Zeroize - so they have to wrap it in a newtype and implement an empty Zeroize for it. And they still cannot assert in code that SecretKey keeps its secret - they have to rely on the docs.

So what are the responsibilities of A here? Should they define an (empty) Zeroize for SecretKey? Should they export a Secret<Box<SecretKey>>? What's the convention?

I wonder if it would be more convenient to have a marker trait ZeroizedOnDrop, that A could implement for SecretKey, and Secret could require?

[daxpedda]

Would a PR for this be welcome?

I would introduce a new derive called ZeroizedOnDrop that implements Drop without implementing Zeroize. Either derive ZeroizedOnDrop or zeroize(drop) will implement the ZeroizedOnDrop marker trait too.

To get around the issue of requiring all fields to implement Zeroize I would abuse auto-deref:

trait AssertZeroizedOnDrop {
    fn zeroize_or_on_drop(&mut self);
}

impl<T: ZeroizedOnDrop> AssertZeroizedOnDrop for &mut T {
    fn zeroize_or_on_drop(&mut self) {}
}

trait AssertZeroize {
    fn zeroize_or_on_drop(&mut self);
}

impl<T: Zeroize> AssertZeroize for T {
    fn zeroize_or_on_drop(&mut self) {
        self.zeroize()
    }
}

This would allow us to do the following:

#[derive(ZeroizedOnDrop)]
struct Test {
  a: u32,
  // Some type only implementing `ZeroizedOnDrop`.
  b: MyType,
}

// Generated:

impl Drop for Test {
  fn drop(&mut self) {
    self.a.zeroize_or_on_drop();
    self.b.zeroize_or_on_drop();
  }
}

impl ZeroizedOnDrop for Test { }

Alternativel I can also add a helper attribute zeroize(dropped), that can generate the following implementation:

#[derive(ZeroizedOnDrop)]
struct Test {
  a: u32,
  // Some type only implementing `ZeroizedOnDrop`.
  #[zeroize(dropped)]
  b: MyType,
}

// Generated:

impl Drop for Test {
  fn drop(&mut self) {
    struct __AssertZeroizedOnDrop<__T: ::zeroize::ZeroizedOnDrop>(::core::marker::PhantomData<__T>);
    self.a.zeroize();
    let _: __AssertZeroizedOnDrop<MyType>;
  }
}

impl ZeroizedOnDrop for Test { }

WDYT?

[tarcieri]

I think ZeroizedOnDrop is a confusing name over ZeroizeOnDrop, especially as the crate used to have ZeroizeOnDrop trait, but also because it's putting in past tense something which will happen in the future.

I'd rather just go back to the previous implementation from iqlusioninc/crates#168 which was removed in iqlusioninc/crates#188 and not introduce a whole lot of additional complexity.

Beyond that, yes a PR would be welcome.

Edit: there's one change I think would be beneficial versus the previous implementation...

It should write a Drop handler which is identical to the derived Zeroize impl, using all of the existing attributes including skipping fields, rather than writing a Zeroize impl then calling that from Drop.

That would make it possible to impl ZeroizeOnDrop but not Zeroize, allowing types to maintain invariants while ensuring their fields are zeroized on drop.

If it's desirable for a type to have both, derive(Zeroize, ZeroizeOnDrop) could be used.

[daxpedda]

I think ZeroizedOnDrop is a confusing name over ZeroizeOnDrop, especially as the crate used to have ZeroizeOnDrop trait, but also because it's putting in past tense something which will happen in the future.

Sounds good.

Edit: there's one change I think would be beneficial versus the previous implementation...

It should write a Drop handler which is identical to the derived Zeroize impl, using all of the existing attributes including skipping fields, rather than writing a Zeroize impl then calling that from Drop.

That would make it possible to impl ZeroizeOnDrop but not Zeroize, allowing types to maintain invariants while ensuring their fields are zeroized on drop.

That is indeed what I was proposing above.

If it's desirable for a type to have both, derive(Zeroize, ZeroizeOnDrop) could be used.

So are we going to remove #[zeroize(drop)]? I would rather deprecate it to avoid a breaking change.

I'd rather just go back to the previous implementation from iqlusioninc/crates#168 which was removed in iqlusioninc/crates#188 and not introduce a whole lot of additional complexity.

So the reason I added the auto-deref example above is that there is no real other way to pass down a way to derive ZeroizeOnDrop.

#[derive(ZeroizeOnDrop)]
struct TestA {
  a: u8,
  b: u8,
}

#[derive(ZeroizeOnDrop)]
struct TestB {
  // `TestA` doesn't implement `Zeroize`, can't make the derive work.
  #[zeroize(skip)]
  a: TestA,
  b: u8,
}

The only way I can see us fix it is to introduce that auto-deref hack above. It is an implementation detail anyway, not something exposed to a user.

I'm gonna start on an implementation without the auto-deref for now.

[tarcieri]

So are we going to remove #[zeroize(drop)]? I would rather deprecate it to avoid a breaking change.

I think that'd be fine eventually, although maybe as a separate PR at the very least.

So the reason I added the auto-deref example above is that there is no real other way to pass down a way to derive ZeroizeOnDrop.

Your code example is exactly what I'd expect

[daxpedda]

Your code example is exactly what I'd expect

But skipping actually doesn't check if it implements ZeroizeOnDrop, considering this is about security it would be nice to add a bit of type-safety. Additionally you will need to keep skipping for every transitive item, both can be avoided by auto-deref. Even if auto-deref is something you would like to avoid, I would suggest introducing the #[zeroize(dropped)] then.

I suggest I make a PR for ZeroizeOnDrop first, then we can explore further options down the line.

[tarcieri]

But skipping actually doesn't check if it implements ZeroizeOnDrop

That's not necessarily desirable in all cases.

#[zeroize(skip)] already provides a DSL for annotating fields. I think it would good to keep things parsimonious between Zeroize and ZeroizeOnDrop, rather than introducing differing behaviors between them (and with it needless complexity in a library that's security-critical).

ZeroizeOnDrop should be "Like Zeroize, but writes a Drop impl rather than a Zeroize impl".

I suggest I make a PR for ZeroizeOnDrop first, then we can explore further options down the line.

Sounds good

[daxpedda]

#[zeroize(skip)] already provides a DSL for annotating fields. I think it would good to keep things parsimonious between Zeroize and ZeroizeOnDrop, rather than introducing differing behaviors between them (and with it needless complexity in a library that's security-critical).

ZeroizeOnDrop should be "Like Zeroize, but writes a Drop impl rather than a Zeroize impl".

I agree, that's why I would prefer the auto-deref implementation, it requires no further input from the user. In any case, we can do that later.