Skip to content

Add SecretPoint. Improve SecretScalar#92

Open
maurges wants to merge 6 commits into
mfrom
zeroize-encoded
Open

Add SecretPoint. Improve SecretScalar#92
maurges wants to merge 6 commits into
mfrom
zeroize-encoded

Conversation

@maurges

@maurges maurges commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
  • SecretPoint is like SecretScalar but for points
  • Add Zeroize instance to scalar and point byte representations
  • SecretPoint and SecretScalar provide methods to get secret bytes

maurges added 6 commits June 17, 2026 11:27
Signed-off-by: maurges <git@morj.men>
Signed-off-by: maurges <git@morj.men>
Signed-off-by: maurges <git@morj.men>
Signed-off-by: maurges <git@morj.men>
Signed-off-by: maurges <git@morj.men>
For some reason cargo build --no-default-features didn't fully check
every definition?

Signed-off-by: maurges <git@morj.men>
@maurges maurges marked this pull request as ready for review June 17, 2026 14:33
@maurges maurges requested a review from survived June 17, 2026 14:34

/// Convert this value into a `NonZero<SecretPoint<E>>`. You should do this at the end
/// of computations that produce a secret, like a key exchange
pub fn into_secret(self) -> NonZero<SecretPoint<E>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should probably add #[inline(always)] here as well. If a separate function is called, a copy of secret point will be left on stack, we will not be able to clear it up

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, because this one I mindlessly copied from SecretScalar and the other one I wrote myself. I'll add the inline to all occurings of this method

#[cfg(feature = "alloc")]
pub fn to_be_bytes(&self) -> alloc::boxed::Box<zeroize::Zeroizing<EncodedScalar<E>>> {
let bytes = zeroize::Zeroizing::new(self.as_ref().to_be_bytes());
alloc::boxed::Box::new(bytes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here, the value of bytes is semantically moved into Box, but a value is left on stack. It'd be better to clean up the bytes on stack afterwards. Something like:

let mut bytes = self.as_ref().to_be_bytes();
let boxed_bytes = alloc::boxed::Box::new(zeroize::Zeroizing::new(bytes));
zeroize::Zeroize::zeroize(bytes.as_mut());
boxed_bytes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I specifically took care that the value left on stack is Zeroizing so that it will get cleared after the function quits. It may be still that the intermediary value is left in the memory though, which maybe has less chance to happen with your approach

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In your code, Zeroizing does not clear up the value left on stack, it will only clear up the value on the heap

@survived survived Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Zeroizing clears up data on drop, there's no drop happening in this function


/// Encodes scalar as bytes in big-endian order
#[cfg(feature = "alloc")]
pub fn to_be_bytes(&self) -> alloc::boxed::Box<zeroize::Zeroizing<EncodedScalar<E>>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Up until now, we've been providing the same API whether alloc feature is used or not. This method can be provided to non-alloc targets by having

#[cfg(feature = "alloc")]
pub struct EncodedSecretScalar<E: Curve>(alloc::boxed::Box<zeroize::Zeroizing<EncodedScalar<E>>>);
#[cfg(not(feature = "alloc"))]
pub struct EncodedSecretScalar<E: Curve>(zeroize::Zeroizing<EncodedScalar<E>>);

similar to how secret scalar/point are defined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But then the api is not the same, you can't reuse the code between alloc and non-alloc. I could come up with a wrapper struct that exposes the same AsRef whether the underlying bytes are boxed or not. Or do you have a better idea?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wdym? If SecretScalar::to_be_bytes() returns EncodedSecretScalar regardless of alloc feature, we will have exactly the same API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as long as EncodedSecretScalar exposes the same methods

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see any better solution than a wrapper struct

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

..I don't know how I read your message yesterday, but yes, what you wrote there is also what came to my mind first


/// Convert this value into a `NonZero<SecretPoint<E>>`. You should do this at the end
/// of computations that produce a secret, like a key exchange
pub fn into_secret(self) -> NonZero<SecretPoint<E>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you also add a similar method to NonZero<Scalar<E>> and Scalar<E> ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NonZero<Scalar<E>> already has it. Added to scalar

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.

2 participants