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

impl Zeroize for BytesMut (optional) #335

Closed
wants to merge 1 commit into from

Conversation

tarcieri
Copy link

@tarcieri tarcieri commented Dec 1, 2019

zeroize is a popular Rust crate for securely zeroing memory:

https://crates.io/crates/zeroize

(I am the author)

Presently it's possible to use Zeroize with BytesMut via DerefMut coercion (Zeroize is impl'd for slice::IterMut).

The implementation in this PR is more complete in that it handles clearing data in the remaining capacity, in the event that it previously contained secrets.

zeroize presently has a bytes-preview feature (which depends on bytes 4.0) which provides this implementation:

https://docs.rs/zeroize/1.0.0/zeroize/#bytes-preview-feature-zeroize-support-for-bytesmut

I'd like to either upstream that implementation, i.e. this PR, and/or drop the bytes-preview feature in zeroize as otherwise it seems deref coercion is sufficient to get a "good enough" implementation (and anyone who wants something better can make a newtype which does what's in the impl in this commit, in the event this PR doesn't get merged).

tl;dr: this PR provides an optimal implementation of Zeroize, but if adding an optional dependency is too onerous, no worries - what we get through deref coercion is "good enough"

`zeroize` is a popular Rust crate for securely zeroing memory:

https://crates.io/crates/zeroize

(I am the author)

Presently it's possible to use `Zeroize` with `BytesMut` via `DerefMut`
coercion (`Zeroize` is impl'd for `slice::IterMut`).

The implementation in this commit is more complete in that it handles
clearing data in the remaining capacity, in the event that it previously
contained secrets.

`zeroize` presently has a `bytes-preview` feature (which depends on
`bytes` 4.0) which provides this implementation:

https://docs.rs/zeroize/1.0.0/zeroize/#bytes-preview-feature-zeroize-support-for-bytesmut

I'd like to either upstream that implementation, i.e. this commit, or
drop the `bytes-preview` feature in `zeroize` as otherwise it seems
deref coercion is sufficient to get a "good enough" implementation
(and anyone who wants something better can make a newtype).
@seanmonstar
Copy link
Member

My gut feeling is to reduce the 3rd party dependencies. @carllerche ?

I'd like to either upstream that implementation, i.e. this PR, and/or drop the bytes-preview feature in zeroize as otherwise it seems deref coercion is sufficient to get a "good enough" implementation

That makes sense from a maintenance point of view. 👍

@tarcieri
Copy link
Author

tarcieri commented Dec 2, 2019

@seanmonstar one semi-related thing which I don't think can be solved without bytes leveraging zeroize as an optional dependency is a Zeroize impl for Bytes. Previously we used this strategy for SecretBytes:

https://github.com/iqlusioninc/crates/blob/544abcc/secrecy/src/bytes.rs#L57

...or in brief: in a Drop impl for (Secret)Bytes, check if the reference count is now 0 (previously by invoking try_mut), and if it is, zero out the backing buffer (or if it isn't, do nothing).

However, it appears try_mut was removed in #298. That's probably for the best, however there's now no way to zeroize Bytes on drop.

I'm not sure the best way to solve this. I've thought about adding a TryZeroize trait which could potentially fail in these sort of cases. Then a SecretBytes wrapper could always call try_zeroize() in its Drop handler, and if it holds the last reference, it would succeed in zeroing out the buffer.

tarcieri pushed a commit to iqlusioninc/crates that referenced this pull request Dec 2, 2019
- Removes the `bytes-preview` feature from `zeroize`
- Upgrades `secrecy` to use `bytes` v0.5

Now that `bytes` v0.5 is out, I've opened a PR to upstream the `Zeroize`
impl for `BytesMut`:

tokio-rs/bytes#335

Unfortunately it's no-longer possible to impl `Zeroize` for `Bytes` as
of `bytes` v0.5, as the `try_mut` method was dropped in this PR:

tokio-rs/bytes#298

I have brought this up on the first PR.

In the meantime, this vendors the previous `BytesMut` impl of `Zeroize`
into `secrecy`'s `SecretBytesMut` type, and drops `SecretBytes` as it's
no-longer possible to implement.
@carllerche
Copy link
Member

Bytes is now a "trait object", though the vtable is not publicly exposed yet.

If we expose it, you should be able to define your own SecretBytes type and convert it to Bytes. Thoughts?

tarcieri pushed a commit to iqlusioninc/crates that referenced this pull request Dec 2, 2019
- Removes the `bytes-preview` feature from `zeroize`
- Upgrades `secrecy` to use `bytes` v0.5

Now that `bytes` v0.5 is out, I've opened a PR to upstream the `Zeroize`
impl for `BytesMut`:

tokio-rs/bytes#335

Unfortunately it's no-longer possible to impl `Zeroize` for `Bytes` as
of `bytes` v0.5, as the `try_mut` method was dropped in this PR:

tokio-rs/bytes#298

I have brought this up on the first PR.

In the meantime, this vendors the previous `BytesMut` impl of `Zeroize`
into `secrecy`'s `SecretBytesMut` type, and drops `SecretBytes` as it's
no-longer possible to implement.
@tarcieri
Copy link
Author

tarcieri commented Dec 2, 2019

@carllerche that might work. I'd have to play with it. What I'd like to do is be able to give out references to &Bytes when expose_secret() is called, but then ensure the backing buffer is zeroed on Drop when the reference count is 0:

https://docs.rs/secrecy/0.5.1/secrecy/struct.SecretBytes.html#implementations

If I can still do that without any modifications to bytes, great.

@carllerche
Copy link
Member

Ok, I'm going to close this for now. We are trying to prune external deps (even optional) to try to reach stability.

@carllerche carllerche closed this Dec 2, 2019
gakonst added a commit to interledger/interledger-rs that referenced this pull request Jan 15, 2020
This is done due to the recent Secrecy breaking changes induced by Bytes 0.5
tokio-rs/bytes#335
iqlusioninc/crates#301
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

Successfully merging this pull request may close these issues.

None yet

3 participants