secrecy: SecretString::From<String> and SecretSlice<S>::From<Vec<S>> potentially leaking secrets

[xanathar]

Hi,
first of all, this is deduced by looking at the code, so I apologize if I'm missing some detail, but it seems to me as the secrecy crate can inadvertently create a copy that will not be zeroized of secret strings and slices.

Strings

The implementation of From<String> is:

impl From<String> for SecretString {
    fn from(s: String) -> Self {
        Self::from(s.into_boxed_str())
    }
}

Nothing fancy, but it calls String::into_boxed_str() which is:

    pub fn into_boxed_str(self) -> Box<str> {
        let slice = self.vec.into_boxed_slice();
        unsafe { from_boxed_utf8_unchecked(slice) }
    }

Again, jumping into Vec::into_boxed_slice() we get:

    pub fn into_boxed_slice(mut self) -> Box<[T], A> {
        unsafe {
            self.shrink_to_fit();
            let me = ManuallyDrop::new(self);
            let buf = ptr::read(&me.buf);
            let len = me.len();
            buf.into_box(len).assume_init()
        }
    }

checking the documentation of shrink_to_fit it says:

The behavior of this method depends on the allocator, which may either shrink the vector
in-place or reallocate. The resulting vector might still have some excess capacity, just as
is the case for [with_capacity]. See [Allocator::shrink] for more details.

Thus, I assume, any call to From<String> with a string that has a capacity larger than the length might leak the previous contents as a copy gets created (depending on allocator behavior).

SecretSlice

impl<S> From<Vec<S>> for SecretSlice<S>
where
    S: Zeroize,
    [S]: Zeroize,
{
    fn from(vec: Vec<S>) -> Self {
        Self::from(vec.into_boxed_slice())
    }
}

And from Vec::into_boxed_slice we can follow the same logic as before, with the only difference that probably it's even more likely for a Vec to have capacity > len.

Solutions

Not sure how this can be solved, though maybe a TryFrom<String> could fail if the String needs to be shrinked, or at least a warning in documentation can be added. Ideally, though, code can check for the need of a shrink, and do the shrink manually (by copy to a new buffer and explicity zeroize of the whole previous buffer).

I can help with the impl of the lattest, but let's first agree/discuss if there is an issue here in the first place, and how to solve it.

[tony-iqlusion]

Well, that's annoying.

One option would be to implement unsafe conversions which avoid the use of shrink_to_fit (though we would need to remove forbid(unsafe_code) to do that)

[xanathar]

Btw:

Ideally, though, code can check for the need of a shrink, and do the shrink manually (by copy to a new buffer and explicity zeroize of the whole previous buffer).
I can help with the impl of the lattest, but let's first agree/discuss if there is an issue here in the first place, and how to solve it.

After thinking about it for 5 more minutes, it doesn't work in the general because it would require the underlying types to be Copy or Clone, which is not required right now (probably solvable going through swap/replace/unsafe shaenanigans that would probably break for !Unpin types, but that's no better than avoiding shrink_to_fit, I guess).