Use SecretBox for all secret values
#144
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #144 +/- ##
==========================================
+ Coverage 90.72% 90.88% +0.15%
==========================================
Files 44 45 +1
Lines 8066 8162 +96
==========================================
+ Hits 7318 7418 +100
+ Misses 748 744 -4 ☔ View full report in Codecov by Sentry. |
| @@ -542,14 +555,17 @@ impl<P: SchemeParams, I: Debug + Clone + Ord + Serialize> FinalizableToNextRound | |||
| let cap_delta = cap_gamma * self.context.k; | |||
|
|
|||
| let alpha_sum: Signed<_> = payloads.values().map(|p| p.alpha).sum(); | |||
| let beta_sum: Signed<_> = artifacts.values().map(|p| p.beta).sum(); | |||
| let beta_sum: Signed<_> = artifacts.values().map(|p| p.beta.expose_secret()).sum(); | |||
There was a problem hiding this comment.
I wonder how far should we go to protect the secrets? In this sum, for example, the safest way to do it is to actually write out an explicit loop mutating the sum inside a SecretBox.
There was a problem hiding this comment.
Yeah I have the same concern and a poor intuition for what "good enough" means here.
By explicit loop, do you mean something like this?
let beta_sum =
artifacts
.values()
.fold(SecretBox::new(Box::new(Signed::default())), |mut sum, p| {
*sum.expose_secret_mut() = sum.expose_secret().add(p.beta.expose_secret());
sum
});In this case here I'm not so sure it's worth it. One line below we'd expose_secret again to add beta_sum to delta. But perhaps delta is a secret too? 😱
synedrion/src/paillier/keys.rs
Outdated
|
|
||
| let p_rem_mod = p_rem_half.to_mod(self.precomputed_mod_p()); | ||
| let q_rem_mod = q_rem_half.to_mod(self.precomputed_mod_q()); | ||
|
|
||
| // TODO (#77): zeroize intermediate values |
There was a problem hiding this comment.
Perhaps they can be wrapped in SecretBox so that we can let RAII do its job? Or would that make too much boilerplate?
There was a problem hiding this comment.
I think it would be very noisy and difficult to read.
I wonder if a macro could help us to be able to say, dunno, secrets_containing_block! { /* lots of secret calculations here, intermediate values included */ } and write normal code inside the block and be sure that everything inside will be zeroized and return values will be SecretBoxed.
Could be a rather big undertaking though.
|
I think this one should go in next, could you rebase it to |
| ) | ||
| } | ||
|
|
||
| pub fn commit_wide( | ||
| &self, | ||
| secret: &Signed<P::WideUint>, | ||
| // TODO(dp): @reviewers Question unrelated to the PR, just something I noticed: Why is the |
There was a problem hiding this comment.
I guess it's just a naming issue. Technically, this method is commit_wide_secret_wide_randomizer(), and the next one is commit_normal_secret_xwide_randomizer(). These are just overrides for the same operations with different types of arguments. This could be made more straightforward if #60 is fixed.
Wrap more secrets in
SecretBox.Keeping this PR as a draft, given it depends on a pre-release candidate for
secrecy(see this issue).When
crypto-bigintv0.6 is released we can address one moreTODO(this one).Fixes #77