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

Add zeroizing to DynResidue #235

Merged
merged 2 commits into from
May 22, 2023

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented May 15, 2023

Currently, DynResidue does not support zeroizing. This PR implements it, albeit probably controversially.

Adding zeroizing suppport seems tricky, in part because modulus parameters are stored in the struct. The question becomes what to do with them. Are they left alone? Are they set internally to all zeros? Are they set to some nominal but valid modulus?

The case for Residue is more clear, since its modulus parameters are generic.

This PR makes the design choice to leave the DynResidue parameters alone, which has two potential advantages:

  • The treatment of modulus parameters is somewhat unified in both Residue and DynResidue
  • The zeroized DynResidue is still valid using its original parameters

However, leaving the modulus parameters in memory may not be in line with the intent of the trait.

I'd appreciate any thoughts or discussion on the best way to handle this, or if it shouldn't be implemented at all.

@AaronFeickert
Copy link
Contributor Author

It may be worth noting that DynResidue already doesn't enforce consistent moduli for arithmetic operations.

@tarcieri
Copy link
Member

Offhand I can't think of a cryptosystem which uses a secret modulus. Public moduli seem pretty foundational to public key cryptosystems.

But I can see someone being surprised it isn't zeroized.

Zeroize itself can't enforce invariants aren't violated, because it borrows the value rather than consuming it. This makes it possible to call from Drop handlers, where it's okay to violate invariants because the value is now out-of-scope.

@AaronFeickert
Copy link
Contributor Author

@tarcieri: Would you advocate for a different design choice here, or do you think the PR's approach is reasonable as is? Perhaps adding a careful note to the DynResidue documentation could be useful, just in case the user expects certain behavior.

@AaronFeickert AaronFeickert requested a review from tarcieri May 16, 2023 02:24
AaronFeickert and others added 2 commits May 21, 2023 19:58
Co-authored-by: Tony Arcieri <bascule@gmail.com>
@AaronFeickert
Copy link
Contributor Author

Rebased.

@tarcieri tarcieri merged commit 3318efa into RustCrypto:master May 22, 2023
@AaronFeickert AaronFeickert deleted the zeroize-dynresidue branch May 22, 2023 03:19
@tarcieri tarcieri mentioned this pull request Sep 4, 2023
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