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

Enforce zeroizing on generic secret keys #147

Closed
AaronFeickert opened this issue Nov 3, 2022 · 0 comments · Fixed by #171
Closed

Enforce zeroizing on generic secret keys #147

AaronFeickert opened this issue Nov 3, 2022 · 0 comments · Fixed by #171
Labels
C-Good_first_issue Good for newcomers

Comments

@AaronFeickert
Copy link
Contributor

AaronFeickert commented Nov 3, 2022

Earlier work in PR 137 improves zeroizing support in the RistrettoSecretKey and RistrettoPublicKey types. However, neither of the generic SecretKey and PublicKey traits require zeroizing or zeroizing-on-drop support.

It may be useful to add trait bounds for these, in order to enforce better key hygiene at the trait level. We can already trivially add a Zeroize bound to both. However, much more useful would be to add a ZeroizeOnDrop bound to SecretKey, which requires that any implementation zeroize secret keys on drop. This can be especially useful in conjunction with other constructions, like signatures, that are defined generically.

The problem is that the current zeroize version does not support the ZeroizeOnDrop marker trait, instead only offering a custom macro for this purpose. The options are to either add our own marker trait and manually implement it for the Ristretto key wrappers (and any future key types), or update zeroize across the board and then replace the zeroize-on-drop macro with the more current #[derive(ZeroizeOnDrop)] macro. Updating to a "proper" version would conflict with the curve library. However, manual testing shows that a zeroize update in the curve library introduces no test failures (nor any failures on bulletproofs or bulletproofs-plus, which are dependencies of this crate).

I think this is another point in favor of doing a custom curve library fork. It seems unlikely that upstream curve library PRs will be accepted. Maintaining a fork seems straightforward, as upstream is infrequently updated. Further, this would enable functionality like precomputation support in bulletproofs-plus. The changes introduced by both of these features are minimal, suggesting little to no effort for ongoing maintenance; and no changes are breaking in a meaningful way that would prevent later reverting back to upstream.

@CjS77 CjS77 added the C-Good_first_issue Good for newcomers label Feb 7, 2023
@CjS77 CjS77 mentioned this issue Feb 7, 2023
stringhandler pushed a commit that referenced this issue Mar 16, 2023
Adds a trait bound requiring that a `SecretKey` zeroize on drop. Derives
this trait bound for `RistrettoSecretKey`.

Currently, we use a macro from `zeroize` to zeroize a
`RistrettoSecretKey` on drop. This works fine, but doesn't let us
require this behavior as a `SecretKey` trait bound. Now that we have a
custom curve library fork that supports an updated version of `zeroize`,
we can take advantage of a new `ZeroizeOnDrop` derive macro that adds a
corresponding marker trait. This PR adds the marker trait as a trait
bound to `SecretKey` and uses the derive macro for `RistrettoSecretKey`.
This adds an additional layer of safety to secret keys.

Closes [issue
#147](#147).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Good_first_issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants