fix!: zeroize temporary scalar value #204
Conversation
| impl<K: SecretKey> Drop for ExtendedWitness<K>{ | ||
| fn drop(&mut self) { | ||
| self.mask.zeroize(); | ||
| self.value.zeroize(); | ||
| self.minimum_value_promise.zeroize(); | ||
| } | ||
| } |
There was a problem hiding this comment.
You should be able to get this functionality for free by deriving ZeroizeOnDrop.
There was a problem hiding this comment.
As a side note, ZeroizeOnDrop also has the benefit of being future-proof against structural changes.
There was a problem hiding this comment.
See this branch for the derived traits.
| impl<K:SecretKey> Drop for ExtendedMask<K>{ | ||
| fn drop(&mut self) { | ||
| self.secrets.zeroize(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The SecretKey trait already enforces zeroizing on drop, so I don't think this is necessary.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
See this branch for the derived traits.
There was a problem hiding this comment.
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.
# Conflicts: # src/ristretto/ristretto_keys.rs
| @@ -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_bytes(bytes.as_slice()) | |||
| let mut bytes: Vec<u8> = borsh::BorshDeserialize::deserialize_reader(reader)?; | |||
There was a problem hiding this comment.
You can use a Zeroizing wrapper here and remove the entire match.
There was a problem hiding this comment.
I tried, but it doesn't deserialize, I could have added a wrapper after the deserialize, but it's basically the same thing
There was a problem hiding this comment.
Which tests fail? I tried it on this branch and everything seems to work fine.
| impl From<Scalar> for RistrettoSecretKey { | ||
| fn from(s: Scalar) -> Self { | ||
| RistrettoSecretKey(s) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a breaking change. Has it been confirmed that tari doesn't use this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Probably worth updating the PR description to clarify that this is a breaking API change.
|
Superseded by #209. |
This adds `Zeroize` and `ZeroizeOnDrop` to `ExtendedMask` and `ExtendedWitness` for improved memory handling functionality. It restricts the visibility of an internal commitment constructor and adds zeroizing to temporary secret values. It updates secret key deserialization to add zeroizing to a temporary byte array. Finally, it removes a secret key constructor. Supersedes #204. BREAKING CHANGE: Changes the commitment and secret key APIs.
|
Closing since #209 has been merged. |
Zeroize a temporary array of Scalars. Scalar is
Copyso these would be placed into a newVecin memory.I have also removed
commit_scalarsfrom the public interface, since it is a foot gun for anyone who uses it but does not zeroize the blinding factors