Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secrecy: How to construct SecretBox #546

Open
troublescooter opened this issue Oct 26, 2020 · 7 comments
Open

secrecy: How to construct SecretBox #546

troublescooter opened this issue Oct 26, 2020 · 7 comments

Comments

@troublescooter
Copy link

troublescooter commented Oct 26, 2020

Am I missing something obvious here, or how is a SecretBox constructed in practice?

fn foo {
    let secret_byte = Box::new(0_u8);
    let secret_box = Secret::new(secret_byte);
}
error[E0277]: the trait bound `std::boxed::Box<u8>: zeroize::DefaultIsZeroes` is not satisfied
   --> src/handshake.rs:151:37
    |
151 |        let secret_box = Secret::new(secret_byte);
    |                                     ^^^^^^^^^^^ the trait `zeroize::DefaultIsZeroes` is not implemented for `std::boxed::Box<u8>`
    |
    = note: required because of the requirements on the impl of `zeroize::Zeroize` for `std::boxed::Box<u8>`
    = note: required by `secrecy::Secret::<S>::new`

There seems to be an impl missing for Box.

@troublescooter troublescooter changed the title How to construct SecretBox secrecy: How to construct SecretBox Oct 26, 2020
@tony-iqlusion
Copy link
Member

The Box impls in zeroize are gated on the alloc feature in zeroize.

Are you using secrecy with --no-default-features?

@troublescooter
Copy link
Author

troublescooter commented Oct 28, 2020

Adding this to the tests in the zeroize crate

fn impls_zeroize<Z: Zeroize>(_: &Z) {}

#[cfg(feature = "alloc")]
#[test]
fn zeroize_box() {
      let mut boxed_arr = Box::new([42u8; 3]);
      // works
      <[u8; 3] as Zeroize>::zeroize(&mut *boxed_arr);
      // works
      boxed_arr.zeroize();
      // doesn't work
      <Box<[u8; 3]> as Zeroize>::zeroize(&mut boxed_arr);
      // doesn't work
      impls_zeroize(&boxed_arr);
      assert_eq!(boxed_arr.as_ref(), &[0u8; 3]);
 }

shows that Box appears to implement Zeroize due to deref coercion.
Adding

#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
impl Zeroize for Box<[u8; 3]>
{
    fn zeroize(&mut self) {
       (**self).zeroize()
  }
}

gets the test to compile, but adding

#[cfg(feature = "alloc")]
#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
impl<Z> Zeroize for Box<Z>
where
    Z: Zeroize,
{
    fn zeroize(&mut self) {
       (**self).zeroize()
  }
}

fails with the error

   Compiling zeroize v1.1.1 (/home/c/pjts/secretbox/src/vendor/zeroize)
error[E0119]: conflicting implementations of trait `Zeroize` for type `alloc::boxed::Box<_>`:
   --> src/lib.rs:375:1
    |
237 | / impl<Z> Zeroize for Z
238 | | where
239 | |     Z: DefaultIsZeroes,
240 | | {
...   |
244 | |     }
245 | | }
    | |_- first implementation here
...
375 | / impl<Z> Zeroize for Box<Z>
376 | | where
377 | |     Z: Zeroize,
378 | | {
...   |
385 | |     }
386 | | }
    | |_^ conflicting implementation for `alloc::boxed::Box<_>`
    |
    = note: downstream crates may implement trait `DefaultIsZeroes` for type `alloc::boxed::Box<_>`

Which is a false positive, DefaultIsZeroes being a subtrait of Copy.

@tony-iqlusion
Copy link
Member

Thanks for reporting this and the writeup. It seems there are major issues with both zeroize and secrecy.

Per the conflicting implementations error you're getting, I'm not sure how to address the Box situation in zeroize without specialization. I'll try to take a look at both of these issues in depth when I have some more spare time.

@troublescooter
Copy link
Author

While trying to see what can be done about this, I ran into a differently worded error:

291 |   impl Zeroize for Box<[u8;3]> {
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `alloc::boxed::Box<[u8; 3]>`
    |
    = note: upstream crates may add a new impl of trait `core::marker::Copy` for type `alloc::boxed::Box<[u8; 3]>` in future versions

So apparently Box could in the future implement Copy, making this not a false positive, but I haven't been able to find an issue or link with more information.
This doesn't help solve the problem, but it surprised me enough to share.

@ia0
Copy link

ia0 commented Jul 20, 2022

Isn't the problem that SecretBox<T> should not be implemented as Secret<Box<T>> but as a custom type instead? The reason is that we want T: Zeroize not Box<T>: Zeroize.

pub struct SecretBox<T: Zeroize>(Box<T>);

@tony-iqlusion
Copy link
Member

@ia0 yes, I've wanted to refactor it to something like that, but haven't had time

@ia0
Copy link

ia0 commented Jul 20, 2022

Cool! Thanks for the feedback and no worries. Nothing urgent, just wanted to check my understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants