secrecy: rework the SecretBox type

[fjarri]

Currently SecretBox = Secret<Box<S>> is not particularly convenient to use: Secret<S> requires S: Zeroize, but if one uses a boxed third-party type, it is impossible to impl Zeroize for it. Zeroize is implemented for Box<[T]>, but not for Box<T>.

This PR redefines SecretBox as a struct containing Box<S>, and impls the same traits for it that Secret had.

Changes:

• Removed everything but SecretBox. Secret cannot be created safely, or even used safely by mutating the contents within the secret (the secret lingers on the stack). SecretVec/String/Bytes may be safe if mutation is prohibited, but that just transfers the burden of creating them safely to the user. I suppose we can add them back with a safety note.
• DebugSecret removed, SecretBox just impls Debug now.
• ExposeSecretMut added. Not sure about that one, maybe we don't need it as long as SecretBox::new_with_mut() is available.
• SecretBox can be created: from an existing Box, by mutating the contents (new_with_mut()), from infallible constructor (new_with_ctr), from fallible constructor (new_with_fallible_ctr). The names may be changed. The latter method we need for the Deserialize impl. I added a safety note to the constructor method indicating that the first two methods are preferable when possible.

Testing notes:
I spent some time trying to introspect the memory to check that no secret data is left on the stack. I used https://docs.rs/psm and https://docs.rs/read-process-memory to check it. It seems that Secret::new(X::new()) does leave the contents of X::new() on the stack, but SecretBox::new(|| X::new()) (the best I could come up with without resorting to unsafe) doesn't. It may be affected by the nature of X::new() and Rust version/optimization flags though, but I couldn't come up with a better test.

It may even be made into an automatic test, if we pick some "safe" memory window to look into without risking a segfault.

[fjarri]

@tarcieri : any comments on this?

[tony-iqlusion]

My first thought looking at this is that there has to be another way and that this approach is suboptimal.

I am unfortunately quite aware of how tricky it is to ensure values are constructed on the heap, but this approach seems like it would make ensured inlining more difficult and force the value to be stack allocated, then moved to the heap, whereas with proper inlining I believe it's possible to avoid that initial stack allocation.

It's been awhile since I've checked that in godbolt though, and I think work on placement-by-return has largely stalled (and the box keyword removed).

Without placement-by-return though, I think this will leave a copy on the stack (i.e. from ctr's original stack frame) unless LLVM decides to inline ctr, so this feels like something of a fraught endeavor.

[tony-iqlusion]

A simpler alternative is to accept Box<T> as an argument.

I think a complex constructor like this needs to be informed by and justified by the generated assembly on popular platforms. If it leaves any copies on the stack it defeats the point (and potentially the inliner).

[fjarri]

I am unfortunately quite aware of how tricky it is to ensure values are constructed on the heap, but this approach seems like it would make ensured inlining more difficult and force the value to be stack allocated, then moved to the heap, whereas with proper inlining I believe it's possible to avoid that initial stack allocation.

I don't think it is supported in Rust currently. It's called "placement new", and currently only exists as a proposal. At the moment we have to accept the fact the we have to construct the value on stack, and all we can do is at least contain it within a small closure and zeroize it. (Edit: sorry, just realized that you alluded to the same feature).

Without placement-by-return though, I think this will leave a copy on the stack

Not according to my tests.

A simpler alternative is to accept Box as an argument.

That is certainly a good idea as an additional constructor, but it's not always possible to already have a Box.

[tony-iqlusion]

I don't think it is supported in Rust currently. It's called "placement new", and currently only exists as a proposal.

You just linked to the placement-by-return proposal I was referring to in my comments.

But without placement-by-return, you're going to make a copy when ctr returns a value, which defeats the whole point.

The only way to eliminate the copy is by eliminating the intermediate stack frame, i.e. inlining, which it seems like this approach will always defeat.

[tony-iqlusion]

In other words, this constructor needlessly forces a stack allocation of a value, then copies it to the heap.

I think we should avoid making copies as much as possible.

[tony-iqlusion]

Not according to my tests.

What tests did you do?

[fjarri]

You just linked to the placement-by-return proposal I was referring to in my comments.

Yes, sorry again, I knew this feature as "placement new", and evidently it got renamed. Either way, it's not in Rust right now, and won't be for a while.

But without placement-by-return, you're going to make a copy when ctr returns a value, which defeats the whole point.

I need the stack frame to get access to the value to zeroize it. ctr passes the ownership of the value, so as far as I understand, there's no additional copy being made within ctr, it's allocated on the stack such that when ctr returns, it's in the outer frame's stack.

Without the placement new you have to have a stack value. It's currently unavoidable, there's no inlining possible that will place it directly in the heap. Best thing we can do is to zeroize it.

[fjarri]

In other words, this constructor needlessly forces a stack allocation of a value, then copies it to the heap.

Well, either you have a Box::new(T::default()) and then mutate it, or you construct it on the stack. The former is not always possible or convenient, and the latter seems to be safe with the constructor I provided.

What tests did you do?

Creating Secret values in a function and inspecting the stack with psm and read-process-memory I linked in the description. I'm thinking on how to make it into an autotest, since it's a little flaky.

[tony-iqlusion]

I think your proposed constructor would need a lot of testing on different platforms to determine it accomplishes the desired goals, yes. Can we hold off on it for now?

[fjarri]

We can if we must, but I'll probably still have to use it in my own code since I cannot create a random crypto_bigint::Uint or k256::Scalar directly in a Box.

[tony-iqlusion]

The rest of this PR is great and the direction I wanted to go with secrecy.

If you can provide a simpler constructor, e.g. one which accepts Box as an argument, we can get it merged and then perhaps circle back on a more complex constructor.

[fjarri]

Any comments on the rest of the listed things in the starting post? Or should they be left for other PRs?

[tony-iqlusion]

• If you just accept a Box<T> in the constructor you don't need a Clone bound
• The point of DebugSecret was to prevent leakage of secret values. I'd personally prefer to retain it. Without it, it becomes very easy to log secrets. If the type doesn't impl DebugSecret, they lose the Debug impl. If they really want to log the secret, they can access the secret value.
• The reason for an inherent method as an accessor method was to make secret accesses easy to search for. AsRef/AsMut defeat that, and combined with functions that bound on AsRef/AsMut and call the relevant trait methods internally would make secret accesses very difficult to locate.
• I'm not sure there's enough functionality to warrant decomposing into submodules

[fjarri]

If the type doesn't impl DebugSecret, they lose the Debug impl.

Yes, but what if I want a Debug impl (which masks the secret value) for a SecretBox<T> where T is a third-party type? I can't implement Debug on SecretBox, and I can't implement DebugSecret on T. It seems to me that leakage can just as easily be prevented by simply providing impl Debug for SecretBox using any, which is what I did.

The reason for an inherent method as an accessor method was to make secret accesses easy to search for. AsRef/AsMut defeat that, and combined with functions that bound on AsRef/AsMut and call the relevant trait methods internally would make secret accesses very difficult to locate.

I was thinking that it would simplify using secret values in generic context, but I see your point.

I'm not sure there's enough functionality to warrant decomposing into submodules

If we remove everything but SecretBox then certainly. But currently there are submodules.

[tony-iqlusion]

Okay, your Debug impl is fine. type_name wasn't available at the time secrecy was originally written.

If we remove everything but SecretBox then certainly. But currently there are submodules.

I think we should get rid of all of the types besides SecretBox.

SecretString can be pub type SecretString = SecretBox<str> (or perhaps SecretStr).

SecretVec can be replaced with SecretBox<[T]> (which could potentially have a SecretSlice alias if helpful).

[fjarri]

Also,

If you just accept a Box in the constructor you don't need a Clone bound

That's not a problem, we can have a separate impl block bound on Clone for that specific constructor, without requiring it for the rest of the methods.

[tony-iqlusion]

@fjarri but it's currently the only constructor, leading to a de facto Clone bound for the whole type. Though the type system doesn't mandate it, SecretBox is otherwise unconstructable without using Clone.

If you had pub fn new(secret: Box<T>) then that's no longer an issue.

You can then add a second constructor that does your FnOnce-based allocator with a mandatory Clone bound.

[fjarri]

Oh yes, I have no problems with having a constructor that takes Box (or even a From impl?). I actually wanted to add it myself, but I must have forgotten to do it.

[tony-iqlusion]

A From impl is fine as long as orphan rules allow it to be sufficiently generic

[tony-iqlusion]

either you have a Box::new(T::default()) and then mutate it

This potentially seems like a better pattern worth supporting, i.e. a constructor which handles the allocation using Default, then passes &mut T to a callback function for initialization, e.g.:

pub fn init_with(initializer: impl FnOnce(&mut S)) -> Self
where
    S: Default

[fjarri]

Removed all the stuff, updated the PR description accordingly.

[tyranron]

Any new on this?

[tony-iqlusion]

I'd be happy to merge it if it weren't still marked a draft