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

fix!: zeroize temporary scalar value #204

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
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
19 changes: 17 additions & 2 deletions src/extended_range_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! Extended range proofs

use std::{string::ToString, vec::Vec};
use zeroize::Zeroize;

use crate::{
commitment::{ExtensionDegree, HomomorphicCommitment},
Expand Down Expand Up @@ -99,13 +100,19 @@ pub trait ExtendedRangeProofService {

/// Extended blinding factor vector used as part of the witness to construct an extended proof, or rewind data
/// extracted from a range proof containing the mask (e.g. blinding factor vector).
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, Zeroize)]
pub struct ExtendedMask<K>
where K: SecretKey
{
secrets: Vec<K>,
}

impl<K:SecretKey> Drop for ExtendedMask<K>{
fn drop(&mut self) {
self.secrets.zeroize();
}
}
Comment on lines +110 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

The SecretKey trait already enforces zeroizing on drop, so I don't think this is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you still wanted to separately enforce zeroizing on drop at the ExtendedMask level as a safeguard against future structural changes, you should be able to simply derive ZeroizeOnDrop instead of this manual implementation.

Copy link
Contributor

@AaronFeickert AaronFeickert Sep 27, 2023

Choose a reason for hiding this comment

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

See this branch for the derived traits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if you derive ZeroizeOnDrop, I'd also do Zeroize too; this will allow a caller to manually call zeroize if they really want to. You don't get Zeroize for free for ZeroizeOnDrop.


impl<K> ExtendedMask<K>
where K: SecretKey
{
Expand Down Expand Up @@ -200,7 +207,7 @@ where PK: PublicKey

/// The extended witness contains the extended mask (blinding factor vector), value and a minimum value
/// promise; this will be used to construct the extended range proof
#[derive(Clone)]
#[derive(Clone, Zeroize)]
pub struct ExtendedWitness<K>
where K: SecretKey
{
Expand All @@ -212,6 +219,14 @@ where K: SecretKey
pub minimum_value_promise: u64,
}

impl<K: SecretKey> Drop for ExtendedWitness<K>{
fn drop(&mut self) {
self.mask.zeroize();
self.value.zeroize();
self.minimum_value_promise.zeroize();
}
}
Comment on lines +222 to +228
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to get this functionality for free by deriving ZeroizeOnDrop.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note, ZeroizeOnDrop also has the benefit of being future-proof against structural changes.

Copy link
Contributor

@AaronFeickert AaronFeickert Sep 27, 2023

Choose a reason for hiding this comment

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

See this branch for the derived traits.


impl<K> ExtendedWitness<K>
where K: SecretKey
{
Expand Down
9 changes: 4 additions & 5 deletions src/ristretto/pedersen/extended_commitment_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
//! Extended commitments are commitments that have more than one blinding factor.

use alloc::vec::Vec;
use core::{borrow::Borrow, iter::once};
use core::{iter::once};

use curve25519_dalek::{
ristretto::{CompressedRistretto, RistrettoPoint},
scalar::Scalar,
traits::{Identity, MultiscalarMul},
};
use zeroize::Zeroizing;

#[cfg(feature = "precomputed_tables")]
use crate::ristretto::pedersen::scalar_mul_with_pre_computation_tables;
Expand Down Expand Up @@ -84,13 +85,11 @@ impl ExtendedPedersenCommitmentFactory {
}

/// Creates a Pedersen commitment using the value scalar and a blinding factor vector
pub fn commit_scalars(
fn commit_scalars(
&self,
value: &Scalar,
blinding_factors: &[Scalar],
) -> Result<RistrettoPoint, CommitmentError>
where
for<'a> &'a Scalar: Borrow<Scalar>,
{
if blinding_factors.is_empty() || blinding_factors.len() > self.extension_degree as usize {
Err(CommitmentError::CommitmentExtensionDegree {
Expand Down Expand Up @@ -166,7 +165,7 @@ impl ExtendedHomomorphicCommitmentFactory for ExtendedPedersenCommitmentFactory
k_vec: &[RistrettoSecretKey],
v: &RistrettoSecretKey,
) -> Result<PedersenCommitment, CommitmentError> {
let blinding_factors: Vec<Scalar> = k_vec.iter().map(|k| k.0).collect();
let blinding_factors: Zeroizing<Vec<Scalar>> = Zeroizing::new(k_vec.iter().map(|k| k.0).collect());
let c = self.commit_scalars(&v.0, &blinding_factors)?;
Ok(HomomorphicCommitment(RistrettoPublicKey::new_from_pk(c)))
}
Expand Down
20 changes: 12 additions & 8 deletions src/ristretto/ristretto_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,19 @@ impl borsh::BorshSerialize for RistrettoSecretKey {
impl borsh::BorshDeserialize for RistrettoSecretKey {
fn deserialize_reader<R>(reader: &mut R) -> Result<Self, borsh::maybestd::io::Error>
where R: borsh::maybestd::io::Read {
let bytes: Vec<u8> = borsh::BorshDeserialize::deserialize_reader(reader)?;
Self::from_canonical_bytes(bytes.as_slice())
let mut bytes: Vec<u8> = borsh::BorshDeserialize::deserialize_reader(reader)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a Zeroizing wrapper here and remove the entire match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but it doesn't deserialize, I could have added a wrapper after the deserialize, but it's basically the same thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Which tests fail? I tried it on this branch and everything seems to work fine.

match Self::from_canonical_bytes(bytes.as_slice())
.map_err(|e| borsh::maybestd::io::Error::new(borsh::maybestd::io::ErrorKind::InvalidInput, e.to_string()))
{
Ok(k) => {
bytes.zeroize();
Ok(k)
},
Err(e) => {
bytes.zeroize();
Err(e)
},
}
}
}

Expand Down Expand Up @@ -234,12 +244,6 @@ impl From<u64> for RistrettoSecretKey {
}
}

impl From<Scalar> for RistrettoSecretKey {
fn from(s: Scalar) -> Self {
RistrettoSecretKey(s)
}
}

Comment on lines -237 to -242
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change. Has it been confirmed that tari doesn't use this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked that this doesn't introduce problems against this PR in tari (checking against its main branch isn't possible), so removing the functionality is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth updating the PR description to clarify that this is a breaking API change.

//--------------------------------------------- Borrow impl -------------------------------------------------//

impl<'a> Borrow<Scalar> for &'a RistrettoSecretKey {
Expand Down
Loading