Skip to content

Derive Hash for a bunch of types #663

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

Closed
wants to merge 1 commit into from

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Aug 22, 2024

Some of the types in the crate do not derive Hash which make them difficult to use in hashmaps or sets.

It would be nice to make Checked also be Hash, but that requires dalek-cryptography/subtle#138 so perhaps that can be a separate PR.

@@ -68,7 +68,7 @@ pub trait ConstMontyParams<const LIMBS: usize>:
/// The modulus is constant, so it cannot be set at runtime.
///
/// Internally, the value is stored in Montgomery form (multiplied by MOD::ONE) until it is retrieved.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
Copy link
Member

@tarcieri tarcieri Aug 25, 2024

Choose a reason for hiding this comment

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

I'm a little concerned about deriving Hash for potentially secret-containing types. Though the default Hasher is SipHash which is fine when properly keyed, other hashers do not necessarily prevent preimage attacks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think documenting the caveats properly is enough? A bigint library that blocks all potential security misuses risks crippling itself a fair bit and it’s not obvious where to draw the line. I’d argue that in this case it’s expected that users know about the security hazards.

Copy link
Member

Choose a reason for hiding this comment

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

It would definitely need documentation. This library is documented as being constant-time by default and we are careful to treat each integer as potentially containing a secret, with separate *_vartime methods which act in variable-time so as to make those sort of secret accesses easier to audit.

I would also want to know what a use case is for keying a HashMap with ConstMontyForm.

@@ -145,7 +145,7 @@ impl<const LIMBS: usize> ConstantTimeEq for MontyParams<LIMBS> {

/// An integer in Montgomery form represented using `LIMBS` limbs.
/// The odd modulus is set at runtime.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here re: potentially secret-containing types

@dvdplm
Copy link
Contributor Author

dvdplm commented Aug 27, 2024

This library is documented as being constant-time by default and we are careful to treat each integer as potentially containing a secret, with separate *_vartime methods which act in variable-time so as to make those sort of secret accesses easier to audit.

Thinking about this further I now think you are absolutely right. If crypto-bigint's stance is that its integers are all potentially secrets, then its types should not be (easily) usable in hash maps or sets. Users would have to have deep knowledge of how the hasher used in their code behaves wrt CT and pre-image resistance etc (or pick a cryptographic hash, with the perf penalty).

This was a dumb idea. Closing. :)

@dvdplm dvdplm closed this Aug 27, 2024
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