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: rework the SecretBox type #1140

Merged
merged 2 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 72 additions & 5 deletions secrecy/src/boxed.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,77 @@
//! `Box` types containing secrets

use super::{DebugSecret, Secret};
use alloc::boxed::Box;
use zeroize::Zeroize;
use core::{
any,
fmt::{self, Debug},
};

/// `Box` types containing a secret value
pub type SecretBox<S> = Secret<Box<S>>;
use zeroize::{Zeroize, ZeroizeOnDrop};

impl<S: DebugSecret + Zeroize> DebugSecret for Box<S> {}
use crate::{ExposeSecret, ExposeSecretMut};

/// Same as [`Secret`], but keeps the secret value in the heap instead of on the stack.
pub struct SecretBox<S: Zeroize> {
inner_secret: Box<S>,
}

impl<S: Zeroize + Clone> SecretBox<S> {
/// Create a secret value using the provided function as a constructor.
///
/// The implementation makes an effort to zeroize the locally constructed value
/// before it is copied to the heap, and constructing it inside the closure minimizes
/// the possibility of it being accidentally copied by other code.
pub fn new(ctr: impl FnOnce() -> S) -> Self {
let mut data = ctr();
let secret = Self {
inner_secret: Box::new(data.clone()),
};
data.zeroize();
secret
}
Copy link
Member

@tony-iqlusion tony-iqlusion Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tony-iqlusion tony-iqlusion Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@fjarri fjarri Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not according to my tests.

What tests did you do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

impl<S: Zeroize + Default> Default for SecretBox<S> {
fn default() -> Self {
Self {
inner_secret: Box::<S>::default(),
}
}
}

impl<S: Zeroize> Zeroize for SecretBox<S> {
fn zeroize(&mut self) {
self.inner_secret.as_mut().zeroize()
}
}

impl<S: Zeroize> Drop for SecretBox<S> {
fn drop(&mut self) {
self.zeroize()
}
}

impl<S: Zeroize> ZeroizeOnDrop for SecretBox<S> {}

impl<S> Debug for SecretBox<S>
where
S: Zeroize,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("SecretBox(")?;
f.write_str(any::type_name::<Self>())?;
f.write_str(")")
}
}

impl<S: Zeroize> ExposeSecret<S> for SecretBox<S> {
fn expose_secret(&self) -> &S {
self.inner_secret.as_ref()
}
}

impl<S: Zeroize> ExposeSecretMut<S> for SecretBox<S> {
fn expose_secret(&mut self) -> &mut S {
self.inner_secret.as_mut()
}
}
6 changes: 6 additions & 0 deletions secrecy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ pub trait ExposeSecret<S> {
fn expose_secret(&self) -> &S;
}

/// Expose a mutable reference to an inner secret
pub trait ExposeSecretMut<S> {
/// Expose secret: this is the only method providing access to a secret.
fn expose_secret(&mut self) -> &mut S;
}

/// Debugging trait which is specialized for handling secret values
pub trait DebugSecret {
/// Format information about the secret's type.
Expand Down